[Toybox] [PATCH] Silence a couple of warnings.

[enh]

On Sun, Feb 5, 2017 at 12:06 PM, Rob Landley <rob at landley.net> wrote:
>> Personally I'd have made xstrdup take const char* like strdup(3), but
>> I'm assuming that patch would be rejected...
>
> Applied that bit. (The android stuff's yours, patch it how you like.)
>
> As for the rest of the codebase, if you start adding "const" to stuff
> you wind up doing regular patches adjusting "const" to silence warnings,
> the only winning move is not to play.
>
> I've still never seen const result in the optimizer doing a better job:
> it doesn't put stuff in rodata and string constants already _are_. And
> if you say "oh, it's not to produce better code, it's just a warning to
> bad programmers who can't keep track of allocation context and yet
> somehow people still want to use their C code" the fact it doesn't
> auto-annotate read-only string constants with "const" because you'd be
> assigning them to non-const char pointers all the time and everything
> would be warnings means there's a pretty darn fundamental design flaw in
> this entire idea, isn't there?
>
> Maybe gcc 6 will start using it to optimize, but isn't the point of the
> --whole-tree stuff that it can figure _out_ object lifetimes and where
> data is and isn't modified? If a better optimizer needs manual hints the
> current one isn't using, define "better". And trying to predict the
> future is infrastructure in search of a user again.
>
> Not a fan of const.
>
> On 02/03/2017 06:17 PM, enh wrote:
>>    // It's fine if this fails (generally because we're not root), but gcc no
>> -  // longer lets a (void) typecast silence the "unused result" warning, so...
>> -  if (fchown(fd, statbuf.st_uid, statbuf.st_gid));
>> +  // longer lets a (void) typecast silence the "unused result" warning, and
>> +  // clang requires the ; be on a new line for obviousness, so...
>> +  if (fchown(fd, statbuf.st_uid, statbuf.st_gid))
>> +    ;
>
> So a line that exists in the first place to silence a dubious gcc
> warning (which is wrong in this instance) needs to be modified to
> silence a dubious clang warning (which is wrong in this instance), and
> clang thought "significant whitespace" was a good solution to the
> problem. (I blame C++.)
>
> Does putting "fd = fd" before the ; fix the problem, or does it warn
> about "statement obviously put there to be optimized away"?

not with the most recent clang i have available, no.

i've sent a patch to use the "unused" trick that Google/Android have
been using everywhere for some time.

> The NDK I've
> got is still missing some stuff... Hmmm, lemme see if I can build just
> this one file with it though:
>
>> $ /opt/android/x86_64/bin/clang -c lib/lib.c -I .
>> In file included from lib/lib.c:6:
>> In file included from ./toys.h:70:
>> ./lib/lib.h:133:42: error: expected ')'
>> pid_t xpopen(char **argv, int *pipe, int stdout);
>>                                          ^
>> /opt/android/x86_64/bin/../sysroot/usr/include/stdio.h:83:17: note: expanded
>>       from macro 'stdout'
>> #define stdout (&__sF[1])
>
> Hmmm... Huh,
> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdio.h.html
> _does_ say they're supposed to be macros. Ok, my bad, I'll fix it.
>
> (Um... how did that ever work for you? You did build this with
> clang+bionic in AOSP, right?)

the platform build targets the current platform release, where we have
stdout as both a symbol and a macro. only older releases _only_ have
BSD-style __sF.

>> lib/lib.c:1039:49: warning: adding 'int' to a string does not append to the
>>       string [-Wstring-plus-int]
>>   for (i=0; i<16; i++) out+=sprintf(out, "-%02x"+!(0x550&(1<<i)), uuid[i]);
>>                                          ~~~~~~~^~~~~~~~~~~~~~~~
>
> Um, yes? Because that's now how C works, and we're programming in C?
> Why does it have a warning about the kind of mistake you shouldn't make
> twice once you understand how the language works?

you'd be surprised how much SoC vendor code won't build because it's
genuinely broken in ways like this. i personally have never seen a new
warning added to clang that didn't find real bugs in shipped SoC
vendor code. even ones you wouldn't believe, like folks who think that
you can use + to concatenate C strings.

plus, as you saw with the recent "mount -a" operator precedence bug,
even those of us who do know what we're doing make mistakes. (which is
why most of us are happy paying the `const` entry fee...)

> This is -Wtraining-wheels, why is it on by default? Ah the full toybox
> build already has an -fwhatiswrongwithyou to suppress that.
>
>> lib/lib.c:1133:11: warning: implicit declaration of function 'getgrgid_r' is
>>       invalid in C99 [-Wimplicit-function-declaration]
>>   errno = getgrgid_r(gid, &list->gr, sizeof(*list)+(char *)list,
>
> According to
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getgrgid_r.html
> you include grp.h to get that, which toys.h is doing?

as noted above (and on a previous thread), it looks like you're
targeting some older version of the platform, presumably one that
didn't have getgrgid_r.

one nice thing about the unified headers in r14 is that -- even though
they don't solve the problem of targeting older releases -- they do
make it very clear what is/isn't available at any given API level.
(when we can drop GCC support entirely, we'll be able to have better
error messages too, telling you that `f` isn't available until API
level whatever.)

> Right, what I _didn't_ see is a warning about the fd = fd; so I'll
> assume that's an acceptable fix that doesn't add a dangling ; on its own
> line deviating from the rest of the code style. (Significant whitespace.
> Sigh.)
>
> Rob
>
> P.S. I want some sort of (i_know) typecast to make the compiler
> consistently shut up about "assignment in an if statement" and so on.
> The "may be used uninitialized, but isn't" nonsense even had its own
> linux kernel macros at one point, but it's hard to grep for
> "int i = i;" after the fact to see if the broken compiler still needs
> that to stop warning about something you sat down and thought through
> and no, it can't happen. There are no UNIFORM workarounds for this
> nonsense that you can go through and clean out later...
>
> P.P.S. I'd probably accept a patch upgrading the error handling, but I
> refuse to be coerced into it by compilers objecting to valid C in
> different ways every 3 months.

here's what we've been using for years:

template <typename... T>
void UNUSED(const T&...) {
}

the patch i just sent you is a gnu99 equivalent. since (afaik) there's
only one instance in toybox so far, it doesn't seem worth trying to
factor it out yet.

-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.