[cfe-dev] [External] : Re: Misleading macro evaluation warning

[Brad Moody via cfe-dev]

Thanks for the info! I'd be happy to port my current implementation to a clang-tidy checker if that would be a better fit.

Nathan, to offer a bit more detail, the current implementation is very much tuned toward finding logic bugs that are likely to affect program behavior and to avoid warning on patterns that evaluate in an unexpected order but give the expected result anyway, like the TWO + 2 example.  There's a large room for debate as to which patterns we report or don't, and the right tuning for a clang warning or clang-tidy check is likely to be different than my current implementation.  I think what I have right now is probably over-tuned toward avoiding warning on possible-but-less-likely bugs.  I'll be happy to accept feedback and tweak the check as necessary.

I've attached a clang -verify test file for the current implementation that covers many interesting cases.  I have three different warning types for evaluation purposes - the warnings containing '(hard)' are the most serious.  The warnings containing '(latent)' and '(cast)' are for patterns that are less likely to cause a problem - in the final implementation these would not be reported but they're useful to see while I'm evaluating the warnings on large codebases.

Regards,
Brad
________________________________
From: Andi-Bogdan Postelnicu <andi at mozilla.com>
Sent: Sunday, February 14, 2021 4:46 PM
To: Nathan James <n.james93 at hotmail.co.uk>
Cc: Brad Moody <brad.moody at oracle.com>; Clang Dev <cfe-dev at lists.llvm.org>
Subject: [External] : Re: [cfe-dev] Misleading macro evaluation warning

Hello,

Nathan thanks for bringing clang-tidy into discussion. I would highly prefer to have this as a clang-tidy checker due to the ease flexibility that clang-tooling is provided.
Also if this will land to clang-tidy there could be also room to have a fixhint.

Thanks,
Andi

> On 14 Feb 2021, at 07:06, Nathan James via cfe-dev <cfe-dev at lists.llvm.org> wrote:
>
> Hi Brad,
>
> Just for the record clang-tidy has a check, bugprone-macro-parentheses.
> This check operates just on the preprocessor stage. It will flag macro
> replacements where they expand to expressions which look like they
> would be safer wrapped in parens. For your example this check wouldn't
> flag the `TWO * 2` line, instead it flags `#define TWO 1 + 1`, This
> obviously has some issues especially if that macro comes from a header
> the user doesn't control.
>
> I'm split on whether this belongs in clang as a warning or as a clang-
> tidy check. Where ever this warning goes some requirements for
> upstreaming are:
> - Create tests.
> - Add diagnostic flags to control the warning(clang only).
> - Test the warning on large code bases, llvm itself is the defacto
> target to test against here. It'd be interesting to see how it handles
> nested macro expansions, thats where false positives or false negatives
> are likely to show up.
> - *Optionally offer some fix-its to either suppress the warning or
> change the code to what was likely intended. Maybe wrap parens around
> the expression to silence the warning, or wrap parens around the macro
> token for intended behaviour.
>
> There's also some potential corner cases to consider:
>  #define TWO 1 + 1
>  auto X = TWO + 2; // Should this warn??
>  auto Y = FloatVariable + TWO; // What about this, wrapping TWO in
> parens can affect the result.
>  auto Z = TemplatedVar + TWO; // This should probably always warn.
>
>
> ~Nathan James
>
>> On Sun, 2021-02-14 at 02:39 +0000, Brad Moody via cfe-dev wrote:
>> Hi all,
>>
>> I have an implementation for a new clang compiler warning that I'd
>> like to upstream and I'm looking for guidance on how to get the
>> process started.
>>
>> The warning reports expressions inside macro expansions that evaluate
>> in an unexpected way due to missing parens, either around a macro
>> argument name or around the entire macro body. This is a super common
>> mistake that has probably been made by anyone who has written a C
>> macro in their life, but I'm not aware of any compilers or other
>> tools that help catch these bugs.
>>
>> An example:
>>    #define TWO 1 + 1
>>    int f() {
>>        return TWO * 2; // Actually returns 3
>>    }
>>
>> Clang output:
>>    a.c:1:15: warning: misleading evaluation of expression in
>> expansion of macro TWO due to missing parentheses
>>    #define TWO 1 + 1
>>                  ^
>>    a.c:3:16: note: expression has higher precedence than expression
>> inside macro body
>>        return TWO * 2;
>>                   ^
>>    a.c:3:12: note: low precedence expression is hidden by expansion
>> of macro TWO
>>        return TWO * 2;
>>               ^
>>    1 warning generated.
>>
>> We've been using this warning internally for about a year now, but
>> mostly on closed code so unfortunately I can't show those results
>> here. I'm in the process of gathering results on open-source code now
>> for evaluation purposes.
>>
>> So, what should be my next steps?
>>
>> Thanks,
>> Brad Moody
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> https://urldefense.com/v3/__https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev__;!!GqivPVa7Brio!KdVFc67UAK5P4yYIwTCJerEacXkvhYxywa3ABBbZcG-ir4qxPLWZLbHCZVg6X07m$
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://urldefense.com/v3/__https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev__;!!GqivPVa7Brio!KdVFc67UAK5P4yYIwTCJerEacXkvhYxywa3ABBbZcG-ir4qxPLWZLbHCZVg6X07m$
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210214/883717fa/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test.c
Type: text/x-csrc
Size: 17250 bytes
Desc: test.c
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210214/883717fa/attachment-0001.c>