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.
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
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).
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.
For the crash that clang doesn't detect that the code will stop there,
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.
Cheers,
Willy
