Page MenuHomePhabricator

clang-tidy: check for repeated side effects in macro
ClosedPublic

Authored by danielmarjamaki on May 5 2015, 6:31 AM.

Details

Reviewers
alexfh
Summary

This is a new clang-tidy checker that warns when there is side effects in a macro argument that is repeated in the macro expansion.

I am running tests. Right now it has been tested on 7678 files in 180 projects. And there has been in total 7 warnings. As far as I see all these 7 warnings are real bugs.

Interestingly I get a couple of warnings for standard functions that are implemented as macros. As far as I see my headers are not standard-compliant:

format.c:774:28: warning: Side effects in macro argument is repeated in macro expansion [misc-macro-repeated-side-effects]
                tblt->instr[0] = toupper(*p++);
                                         ^
/usr/include/ctype.h:229:11: note: 'toupper' macro defined here
#  define toupper(c)    __tobody (c, toupper, *__ctype_toupper_loc (), (c))
          ^
tunnel.c:73:25: warning: Side effects in macro argument is repeated in macro expansion [misc-macro-repeated-side-effects]
      char *q = strchr (++p, '"');
                        ^
/usr/include/x86_64-linux-gnu/bits/string2.h:395:11: note: 'strchr' macro defined here
#  define strchr(s, c) \
  (__extension__ (__builtin_constant_p (c) && !__builtin_constant_p (s)	      \
		  && (c) == '\0'					      \
		  ? (char *) __rawmemchr (s, c)				      \
		  : __builtin_strchr (s, c)))

Diff Detail

Event Timeline

danielmarjamaki retitled this revision from to clang-tidy: check for repeated side effects in macro.
danielmarjamaki updated this object.
danielmarjamaki edited the test plan for this revision. (Show Details)
danielmarjamaki updated this object.
danielmarjamaki added a subscriber: Unknown Object (MLST).

Interestingly I get a couple of warnings for standard functions that are implemented as macros. As far as I see my headers are not standard-compliant:

hmm. now I have the feeling I was wrong.

I don't know.. but I guess that probably those are some compile-time handling only.

Now it doesn't warn about macros in system headers.

I have now tested it on 18045 files in 381 debian projects.

I saw warnings in 4 projects. In two projects I believe there are bugs. Imo the other two projects had false positives for these reasons:

  1. repeated side effects is intended.
  2. argument with side effects is repeated in macro, but in different conditional code paths, side effect is not repeated

For #1 I think the only reasonable solution is that this checker must be disabled by the user.

For #2 I think we could maybe fix some FPs in clang tidy by analysing the tokens. But it's not going to be easy and solid imho. I recommend that these users disable this warning. If we bailout when there are different conditional code paths there will be false negatives.

alexfh added a subscriber: alexfh.May 8 2015, 7:08 AM

Sorry for the delay. I was on vacation and currently digging out stuff out of my inbox. I'll try to review this thoroughly early next week. A few random comments for now.

Now it doesn't warn about macros in system headers.

It looks like the latest patch is exactly the same as the first one.

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
42

The clang:: qualifier is not needed here and below.

51

Will a range-based for loop work here?

55

ditto

test/clang-tidy/misc-repeated-side-effects-in-macro.c
14

Do you need a separate function for each test?

Also, please leave the whole warning message once and truncate all the other instances to fit into 80 columns.

Taken care of review comments. Dont warn about macros in system headers.

alexfh added inline comments.May 11 2015, 8:52 AM
clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
49

nit: Please add a trailing period.

50

What system macros did get flagged here that you wouldn't want? Combining MIN/MAX with a side effect seems like a bad idea, for example.

54

So, will a range-based for loop work here?

Also, when I send patches for review, I find it convenient to answer each reviewer's comment with "Done" or a reason why the comment is not applicable. This helps keeping track of the issues I addressed.

78

s/sideeffects/side effects/

85

I'd make the message more specific by adding at least the index of the argument that is repeated. Maybe also add the name of the parameter in the note.

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
50

For instance: toupper, strchr. I think these were false positives. :-(

For instance for strchr I assume "__builtin_constant_p (s)" does not use s at runtime and then it's safe to use s again later in the macro.

I don't see how we can teach the checker to not care about any compiletime behaviour. do you know if this is possible?

51

I don't see how I can do this without modifying the MacroInfo, I'd need a new method that returns a container with the arguments right? Maybe the MacroInfo::ArgumentList should be a container?

I would prefer to do such refactoring separately.

54

How?

Then I must modify MacroInfo right? Either the arguments must be stored in some container. Or else MacroInfo can have a method that creates a container with the arguments and returns it, I know this is not a good solution but seems easier for me to implement ;-).

I would prefer to not change MacroInfo in this patch.. but I could submit a parallell patch.

55

Impossible.

Fortunately I have a patch pending (http://reviews.llvm.org/D9079) that makes it possible to do this. If it gets the OK and I can apply it I will of course use the range-based for loop here too.

85

Yes I agree.. will do.

alexfh edited edge metadata.May 21 2015, 5:09 AM

Nice, but the results are from a different check: misc-macro-parentheses.

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
50

I can find some definitions of these macros, but I'm not sure they would be the same definitions you are talking about. It would be easier if you quoted the definitions here.

54

Yes, you'll need to add a method that returns a "container" with arguments same way as in D9079:

ArrayList<IdentifierInfo*> args() const {
  return ArrayList<IdentifierInfo*>(ArgumentList, NumArguments);
}
55

Phabricator says that you have committed that patch 10 days ago:

Closed by commit rL236975: Refactor MacroInfo so range for loops can be used to iterate its tokens. (authored by danielmarjamaki)

Here are some results:
https://drive.google.com/file/d/0BykPmWrCOxt2YTdOU0Y3M1RmdGM/view?usp=sharing

Nice, but the results are from a different check: misc-macro-parentheses.

I see.. I dont have the results anymore. I am rerunning the checker and will probably post results tomorrow.

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
49

Will be fixed in next patch

55

Yes I will of course use this. I totally agree range for loops are better.

78

Will be fixed in next patch

The script I use to test Clang on many packages:
https://drive.google.com/file/d/0BykPmWrCOxt2Zy1wVXZQWVV0LVE/view?usp=sharing

Background:
In the Cppcheck project we run Cppcheck on all debian packages continuously with a script. The results are shown here:
http://cppcheck.sourceforge.net/devinfo/daca2-report/daca2.html

I modified the script that runs Cppcheck so it runs Clang instead.

How to use it:

Create a folder:
mkdir ~/daca-clang

Run Clang on debian source code:
python daca-clang.py --folders=abc --cc_path=~/llvm/build/Debug+Asserts/bin1

--folders=abc:

Select folders to check. This will select the folders "a", "b" and "c".. these contain all packages that start with "a", "b" or "c".

--cc_path=...

Specify where clang is found

this will only run normal compilation. To run a static analyser checker, add --checker=name-of-checker. To run clang-tidy, use --clang-tidy (however this currently only runs misc-macro* checkers).

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
50

Here is the toupper macro:

#  define toupper(c)    __tobody (c, toupper, *__ctype_toupper_loc (), (c))

The c is repeated but I assume that __tobody doesnt use both argument 1 and 4 at runtime.

Here is the strchr macro:

#  define strchr(s, c) \
  (__extension__ (__builtin_constant_p (c) && !__builtin_constant_p (s)	      \
		  && (c) == '\0'					      \
		  ? (char *) __rawmemchr (s, c)				      \
		  : __builtin_strchr (s, c)))

The s is used in the condition and then in both conditional expressions. However I assume that the usage in the condition is compile-time only.

danielmarjamaki edited edge metadata.

fixed comments from previous revision.

alexfh added inline comments.Jun 8 2015, 6:53 AM
clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
75

This would also benefit from Token::isOneOf that you can move from clang/lib/Format/FormatToken.h to clang::Token.

97

You seem to assume that the macro is function-like, but you'd better check it (PP->getMacroDefinition(TII)->getMacroInfo()->isFunctionLike() + all required nullptr checks).

124

Let's use standard facilities provided by the diagnostics engine:

Check.diag(..., "side effects in the %ordinal0 macro argument ....

If you want this to be more verbose, you can combine this with a %select{} modifier to add spellings for the first few ordinals.

test/clang-tidy/misc-repeated-side-effects-in-macro.c
67

You can omit the " is repeated in macro expansion [misc-macro-repeated-side-effects]" part from all check lines but the first one to make the tests more readable.

edse added inline comments.Jun 10 2015, 6:29 AM
clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
75

Fixed (the addition of Token::IsOneOf will be added in another patch).

97

Actually the SkipParen bails if the next token is not a left parenthesis, but there may be complications if a parenthesis is following a macro without arguments.

I will implement your Idea in the next patch. It seems more roboust.

124

So it did exist, have to dig deeper next time... I replaced the Tiers-functions with %ordinal0 and I think it is satisfying.

test/clang-tidy/misc-repeated-side-effects-in-macro.c
67

will do, see next patch.

Updated patch. Fixed comments and refactored.

Thank you for the update!

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
57

s/Bail/Bail out/, otherwise it means something different. Also, "the contents" and s/Macro/the macro/ (there's no variable named Macro).

60

clang-format, please.

63

s/paranthesis/parenthesis/

64

Missing comma before "then".

84

The line is too long. Please clang-format.

87

s/NULL/nullptr/

115

Maybe "side-effects ... _are_ repeated"?

118

"macro %0 defined here" would read better (given %0 is the name of the macro).

test/clang-tidy/misc-repeated-side-effects-in-macro.c
8

I thought, "%ordinal0" would generate "1st", not "first". Did you run tests after making changes?

27

nit: missing trailing period.

34

Please use proper capitalization and punctuation in comments. Here and below.

35

Did you mean "bail out"? IMHO, "bail" means something different.

45

Space after //, capitalization, trailing period.

48

ditto

49

If the test doesn't need special formatting, please clang-format.

73

s/bailout/bail out/

took care of review comments

alexfh added inline comments.Jun 15 2015, 5:36 AM
clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
55

I find it hard to read this function due to many nested control statements. Let's split this loop in two separate checks:

if (std::find_if(MI->tokens().begin(), MI->tokens().end(), [] (const Token &T) { return T.isOneOf(...);}) != MI->tokens.end())
  return;

and

if (CountArgumentExpansions(Arg /* or ResultArgToks */, MI->tokens()) < 2)
  continue;

And move most of it to a new CountArgumentExpansions function (if you have a better name, feel free to use it).

57

nit: "content ... is" or "contents ... are"
Also, s/to complex/too complex/.

63

nit: It's more correct to use plural here: "parentheses starting at the current position."

76

s/NULL/nullptr/

114

Please use isOneOf.

118

Looks like you're calling MI->getArgumentNum(Arg) twice. Maybe store the result?

New patch for this checker. Fixed comments, refactoring.

alexfh accepted this revision.Jun 17 2015, 5:29 AM
alexfh edited edge metadata.

Looks good now with one nit.

Thank you for another awesome check!

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
70

nit: And now I'd revert the order of checks (HasSideEffects seems to be slightly cheaper?) and make this even simpler:

if (HasSideEffects(ResultArgToks) && CountArgumentExpansions(MI, Arg) >= 2) {
  Check.diag(...) ...
}
This revision is now accepted and ready to land.Jun 17 2015, 5:29 AM
alexfh added inline comments.Jun 17 2015, 5:31 AM
clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
70

... reverse the order of checks ...

Checked in with 239909.

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
55

Fixed

57

Fixed

63

I rewrote this

// If current token is a parenthesis, skip it.

maybe it can be improved further

76

fixed

114

Fixed

118

Fixed