Willy, Am 24.06.19 um 07:33 schrieb Willy Tarreau: > Hi Tim, > > On Sun, Jun 23, 2019 at 10:10:08PM +0200, Tim Duesterhus wrote: >> Willy, >> >> these patches are the result of running >> >> scan-build make -j4 all TARGET=linux-glibc DEBUG=-DDEBUG_STRICT=1 >> >> While it spits out a bunch of false negatives that are quite convoluted these >> patches either: >> >> 1. Make it easier for clang analyzer to understand the code by making a few >> *small* >> adjustments. The one in `proxy_parse_declare` might be questionable, >> though. >> 2. Actually fix an issue I could reproduce with a carefully crafted example >> configuration. > > 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. 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. > 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 > 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 :-) > 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? > 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. > I understand the problem but I think that fixing it with an ifdef to > detect this specific analyzer isn't the right way to do it because > we'll get it in the face again soon for any other code analyzer. I > think we should move this to an inline function marked with > __attribute__((noreturn)) instead. > > I'll have a deeper look, because I think we need some updated tools. If > compilers get smarter at purposely annoying us, we'll need to more > ostensibly shut them up when relevant by not telling them what we're > doing since they appear to have difficulties reading the code they were > made to compile in the first place... > > I'm having a few ideas that could actually all derive from the > ALREADY_CHECKED() hack. For example we could use something like this > to block funny checks and disable certain absurd warnings that made > us introduce bugs in the past : > > #define FORCE(x) ({ typeof(x) __x__ = (x); asm("" : "=rm"(__x__) : > "0"(__x__)); __x__; }) > > We could also use __builtin_trap() when defined, or __builtin_unreachable() > after an asm statement, optionally replaced by a loop when not defined. > > I may propose you a few things to try later today. Best regards Tim Düsterhus
