Willy,
Am 24.06.19 um 11:07 schrieb Willy Tarreau:
> 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.
I agree here, that's why I only created patches for a subset of the
reported issues.
>>> 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.
Agreed.
>>> 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
IMO silencing is better than ignoring, because then a the report does
not need to be evaluated again when checking again. Of course I
recognize that silencing potentially comes at a cost to the human and
that one should not make any huge changes, just to please the machine.
For the changes I made I'd argue that they actually might be helpful to
the human as well to make sure that the locations in question do not
diverge. Less so for the `proxy_parse_declare` one, more for
`h2_make_htx_(request|response)` one. I find the latter incredibly
non-obvious, but that might be just my view.
May I suggest this one for the latter then (I believe it captures the
intent better than the current one):
--- a/src/h2.c
+++ b/src/h2.c
@@ -689,7 +689,7 @@ int h2_make_htx_request(struct http_hdr *list,
struct htx *htx, unsigned int *ms
goto fail;
/* Let's dump the request now if not yet emitted. */
- if (!(fields & H2_PHDR_FND_NONE)) {
+ if (!sl) {
sl = h2_prepare_htx_reqline(fields, phdr_val, htx, msgf);
if (!sl)
goto fail;
@@ -913,7 +913,7 @@ int h2_make_htx_response(struct http_hdr *list,
struct htx *htx, unsigned int *m
goto fail;
/* Let's dump the request now if not yet emitted. */
- if (!(fields & H2_PHDR_FND_NONE)) {
+ if (!sl) {
sl = h2_prepare_htx_stsline(fields, phdr_val, htx, msgf);
if (!sl)
goto fail;
> 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!
>
Fair enough. I'm not going to argue about my patches back and forth,
that could go hours without bringing any useful result. Please merge the
patches you like from this series and disregard the others (#4 and #5
have already been merged by William and Christopher). And maybe consider
my suggestion in this email for #9, I honestly believe that it makes the
code clearer for both the human and the machine.
Best regards
Tim Düsterhus
