On Mon, Jun 24, 2019 at 10:03:29AM +0200, Tim Düsterhus wrote:
> > Well, some there seem to be real bugs, but others are more a matter of
> > style than correctness and I disagree with a bogus compiler dictating
> > what coding style rules we must respect. Because that compiler is bogus.
>
> To be clear: This is not a compiler. It's a static analyzer. clang !=
> clang analyzer.
OK maybe but the point remains, we're relying on a tool complaining about
*style* and not about code correctness. History has proven multiple times
that whenever we fall into that trap, we end up with :
- confusing code which is less and less maintainable over time,
due to the extra checks that lead to nothing and that become very
difficult to adjust.
- new bugs due to the extra complexity involved in certain checks
or ways to write code.
- regressions in stable branch by failure to adapt the mangled code
to work properly there.
Yes I'm in favor of fixing bogus code or to adapt fragile constructs,
but not to work around compiler (or analyzer) bugs or stupidities if
they add wrong assumptions, confusion, or suspicion in the code.
> There were a *much* larger number of mistakes reported. I picked the
> ones that either were valid (BUG/MINOR) or that were easily silenced
> (MINOR) and I carefully read the code to make sure that they are valid.
OK but one more reason then to drop any other tendencious ones if it
doesn't even achieve a 100% cleaning!
> > If it emits an error when using sizeof(int) instead of sizeof(*foo) where
> > foo is an pointer to unsigned int, it is bogus. I challenge you to present
> > me a relevant architecture where unsigned changes the allocated size!r
>
> For C11 it is guaranteed that there is none according to this:
> https://stackoverflow.com/a/13169473/782822
Not surprised at all :-)
> > Please note that I *do* prefer the sizeof(*foo) for readability and long
> > term correctness, it's just that we must not claim a bug in the code when
> > the bug is in the compiler itself (or whatever claims there is a bug).
>
> Feel free to re-tag that one as MINOR :-)
In my opinion it's a cleanup : it doesn't change the emitted code at all,
and makes the code more robust against changes, and saves one round trip
to the include file for the casual reader trying to figure what caused a
bug to happen.
> > Also I refuse to use BUG_ON() to silence the compiler. The sole purpose
> > of BUG_ON() is to help the *developer*. It should be seen as proven
> > documentation that helps figuring branches when analysing a bug. While
> > the assertion in a comment may have become wrong over time, the assertion
> > in a BUG_ON() is provably true.
>
> I agree with any *large* changes to make the analyzer happy. In the two
> patches I made it was a minimal change to make. The proxy_parse_declare
> probably could also be changed to:
>
> > --- a/src/proxy.c
> > +++ b/src/proxy.c
> > @@ -498,7 +498,8 @@ static int proxy_parse_declare(char **args, int
> > section, struct proxy *curpx,
> > hdr->index = curpx->nb_req_cap++;
> > curpx->req_cap = hdr;
> > }
> > - if (strcmp(args[2], "response") == 0) {
> > + else {
> > + BUG_ON(strcmp(args[2], "response") != 0);
> > hdr->next = curpx->rsp_cap;
> > hdr->index = curpx->nb_rsp_cap++;
> > curpx->rsp_cap = hdr;
>
> which might be acceptable?
No because that BUG_ON() above is redundant, just put an "else if (strcmp...)"
if you want but I really fail to see the problem there and we're not going
to add confusion in the code to please a shitty compiler or analyzer which
finds bugs where they are not. What you're doing above is mangling the code
to *temporarily* shut up the analyzer. It has nothing to do with addressing
any real issue. Either the analyzer is right and the bug is somewhere else
and must be fixed, or it's wrong and we'd rather ignore it. Otherwise we
can as well use this awesome analyzer and try to fix its reports :
$ for i in src/*.c; do for j in $(seq 1 $((RANDOM%5+1))); do echo
"$i:$((RANDOM%2000+1)): variable is used uninitialized in this
function";done;done
Don't get me wrong, I'm not dismissing everything such analysers can find,
I'm saying that these tools should be used as a help an not as a goal. Each
report needs to be carefully evaluated. In the strcmp() example above,
nothing in your patch addresses a real issue since the code is just a
repetition of something done 5 lines above. If the analyzer is wrong there,
be it!
> > For the crash that clang doesn't detect that the code will stop there,
>
> Just to make sure: clang *analyzer*. It's nothing an off-the-shelf clang
> will report.
One more reason for not sticking too hard to such wrong reports then!
Thanks,
Willy
