Misleading macro evaluation warning

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Misleading macro evaluation warning

Deep Majumder via cfe-dev
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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Misleading macro evaluation warning

Deep Majumder via cfe-dev
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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Misleading macro evaluation warning

Deep Majumder via cfe-dev
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 <[hidden email]> 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
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [External] : Re: Misleading macro evaluation warning

Deep Majumder 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 <[hidden email]>
Sent: Sunday, February 14, 2021 4:46 PM
To: Nathan James <[hidden email]>
Cc: Brad Moody <[hidden email]>; Clang Dev <[hidden email]>
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 <[hidden email]> 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
>> [hidden email]
>> https://urldefense.com/v3/__https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev__;!!GqivPVa7Brio!KdVFc67UAK5P4yYIwTCJerEacXkvhYxywa3ABBbZcG-ir4qxPLWZLbHCZVg6X07m$
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://urldefense.com/v3/__https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev__;!!GqivPVa7Brio!KdVFc67UAK5P4yYIwTCJerEacXkvhYxywa3ABBbZcG-ir4qxPLWZLbHCZVg6X07m$

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

test.c (23K) Download Attachment