This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Prevent misuses of -analyze-function
ClosedPublic

Authored by steakhal on Feb 1 2022, 3:20 AM.

Details

Summary

Sometimes when I pass the mentioned option I forget about passing the parameter list for c++ sources.
It would be also useful newcomers to learn about this.

This patch introduces some logic checking common misuses involving -analyze-function.

Diff Detail

Event Timeline

steakhal created this revision.Feb 1 2022, 3:20 AM
steakhal requested review of this revision.Feb 1 2022, 3:20 AM
martong accepted this revision.Feb 1 2022, 9:18 AM

Very good! If we had this earlier it could have saved me some frustrating debugging.

clang/test/Analysis/analyze-function-guide.cpp
18

So we need -Dbool=_Bool trickery to be able to parse the function in C, okay. I think it would have been simpler to have fizzbuzz(int x, int y) instead.

49

We should have a matching case with -x c as well.

This revision is now accepted and ready to land.Feb 1 2022, 9:18 AM

Nice quality-of-life improvement!

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
506 ↗(On Diff #404872)

Every top-level function was skipped

steakhal marked an inline comment as done.Feb 2 2022, 1:47 AM

Thanks for the reviews

clang/test/Analysis/analyze-function-guide.cpp
18

I'm intentionally demonstrating this.
Since even in C++ mode, we need to match against _Bool.
Hopefully, this will help fellow developers for this detail.

This revision was automatically updated to reflect the committed changes.
steakhal marked 3 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2022, 2:32 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Landed with the requested changes.

NoQ added inline comments.Feb 2 2022, 1:30 PM
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
523 ↗(On Diff #405184)

🥺

NoQ added inline comments.Feb 2 2022, 1:37 PM
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
523 ↗(On Diff #405184)

You can use this for a test, should be in an .m file:

@interface MyClass
-(int) messageWithFoo: (int)foo bar: (int)bar;
@end

@implementation MyClass
-(int) messageWithFoo: (int)foo bar: (int)bar {
  return foo + bar;
}
@end

(reacts to -analyze-function="-[MyClass messageWithFoo:bar:]")

NoQ added inline comments.Feb 2 2022, 1:46 PM
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
523 ↗(On Diff #405184)

Oh wait, this is already committed. I guess I can do that myself.

steakhal reopened this revision.Feb 2 2022, 11:34 PM

It broke some bots. I had to revert it in minutes it landed. Yet to be investigated how did it break something.
Mind that I dont have any experience with objC.

This revision is now accepted and ready to land.Feb 2 2022, 11:34 PM
steakhal updated this revision to Diff 406463.Feb 7 2022, 7:43 AM
  • pin target triple to (possibly) resolve the build bot failure
  • add obj-c test case
NoQ added a comment.Feb 7 2022, 9:46 AM

Thanks!!

This revision was landed with ongoing or failed builds.Feb 8 2022, 8:28 AM
This revision was automatically updated to reflect the committed changes.
steakhal reopened this revision.Feb 8 2022, 8:44 AM

Ah, it seems like pinning the target triple was not enough.
I'm stuck at this point.

This revision is now accepted and ready to land.Feb 8 2022, 8:44 AM
NoQ added a comment.Feb 8 2022, 9:10 AM

Can you post a link to a specific buildbot failure so that we could look at it together?

Can you post a link to a specific buildbot failure so that we could look at it together?

Sure!

I received these three mails:
https://lab.llvm.org/buildbot#builders/67/builds/5824
https://lab.llvm.org/buildbot#builders/91/builds/3404
https://lab.llvm.org/buildbot#builders/98/builds/12849

This revision was automatically updated to reflect the committed changes.

I've decided to remove the offending RUN lines.
It's less than ideal, but TBH I think it doesn't really matter.