[PATCH] Fix /bin/sh compilation with CFLAGS += -DDEBUG > 1

[Garrett Cooper]

On Tue, Oct 12, 2010 at 5:30 AM, John Baldwin <jhb at freebsd.org> wrote:
> On Tuesday, October 12, 2010 6:47:49 am Garrett Cooper wrote:
>> Hi,
>>     It looks like the format strings are broken on 64-bit archs in
>> /bin/sh's TRACE functionality (can be enabled by uncommenting -DDEBUG
>> > 1 in bin/sh/Makefile). The attached patch fixes this functionality
>> again so one can trace sh's calls with TRACE, which may or may be
>> helpful to those debugging /bin/sh.
>>     Tested build and execution on amd64; tested build on i386.
>
> I don't think the Makefile bits are needed, you can just use
> 'make DEBUG_FLAGS="-g -DDEBUG=2"' instead.

    Ok... I was just trying to make it tunable, but sure :)!

> Also, if you plan on using -g you should generally set DEBUG_FLAGS anyway so
> binaries are not stripped.

    Ok, something new to note...

> The use of things like PRIoMAX is not done in FreeBSD as it is ugly.  You can
> use things like '%t' to print ptrdiff_t types instead.  So for example, for
> the first hunk, I would change the type of 'startloc' to ptrdiff_t and use
> this:
>
>        TRACE(("evalbackq: size=%td: \"%.*s\"\n",
>                (dest - stackblock()) - startloc,
>                (int)((dest - stackblock()) - startloc),
>                stackblock() + startloc));
>
> Also, in your change here, you used %j to print a size_t.  That will break on
> i386.

    It didn't actually break the build, but it's fine because I didn't
test runtime (and I might have broken that)... I'll go with defacto
standard as per printf(3).

> You should use %z to print size_t's, but even better is to just use %t
> to print a ptrdiff_t (which is the type that holds the difference of two
> pointers).

    Ok. The overall temperature of using PRI* from POSIX seems like
it's undesirable; is it just POSIX cruft that FreeBSD conforms to in
theory only and doesn't really use in practice, or is there an example
of real practical application where it's used in the sourcebase?

> The various changes in jobs.c should use '%td' as well rather than (int)
> casts.

    Ok. Tested build and runtime on amd64 and tested build-only with i386.

Thanks!
-Garrett
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-bin-sh-DEBUG-functionality.diff
Type: application/octet-stream
Size: 3299 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20101012/55c41063/fix-bin-sh-DEBUG-functionality.obj