Page MenuHomePhabricator

[analyzer] Fix macro names in diagnostics within bigger macros.
ClosedPublic

Authored by NoQ on Mar 7 2019, 5:33 PM.

Details

Summary

On the newly included test case the previous behavior was

Line 53: note: Assuming 'i' is equal to nested_null_split

which is meh.

I tried to make this piece of logic slightly more correct, but it's most likely still completely incorrect.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Mar 7 2019, 5:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 5:33 PM
Szelethus requested changes to this revision.Mar 8 2019, 1:42 AM

Uhh, it's such a chore to work on these things -- You can really feel that the preprocessor must've been the first thing implemented in clang. Unfortunately, you really have to counterweight it's shortcomings with excessive amount of comments. I can see an infinite loop and a lot of string comparisons, but someone without having this particular test case as a context, it's very hard to follow what's happening.

Could you please add some code examples in the comments too?

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1997–1999 ↗(On Diff #189805)

Gets the location of the immediate macro caller, one level up the stack toward the initial macro typed into the source.

Hmm, is there a guarantee that at the end of the stack is not a macro location?

clang/test/Analysis/diagnostics/macros.cpp
53 ↗(On Diff #189805)

Ah, that looks pretty :D

This revision now requires changes to proceed.Mar 8 2019, 1:42 AM
Charusso requested changes to this revision.Mar 8 2019, 7:51 AM
Charusso added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1985 ↗(On Diff #189805)

ParentName?

1996 ↗(On Diff #189805)

I was never cool in algorithms, but I think it is over-complicated. In Hungary we are about to build a new nuclear power plant so here it is really emphasized: never-ever create an infinite loop.
What about the following?:

while (LocStart.isMacroID()) {                                             
  StringRef CurrentMacroName = Lexer::getImmediateMacroNameForDiagnostics( 
      LocStart, BRC.getSourceManager(),                                    
      BRC.getASTContext().getLangOpts());                                  
  LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);  
  if (CurrentMacroName != ParentName)                                           
    MacroName = CurrentMacroName;                                          
}
2003 ↗(On Diff #189805)

There is too much duplication and it is difficult to understand what is going on. May here is the time for deduplicating?
Please note that, there is a function called getMacroName(SourceLocation, BugReporterContext &), may you would put your own helper-function above that.

NoQ updated this revision to Diff 192765.EditedMar 28 2019, 7:13 PM

An elegant solution for a more civilized age. Unfortunately, doesn't preserve the UINT32_MAX macro in the newly added test - i'll try a bit harder to preserve it. Relies on D59977 to work.

NoQ added a comment.Mar 29 2019, 10:12 AM

I think the right thing to do here is "look at the immediate macro; while it expands exactly to our original expression, look at what it is an expansion of; write down the last macro we've reached". My code now gives up whenever we stop expanding to the original expression, but it should write down the highest macro it has reached instead.

NoQ updated this revision to Diff 192906.Mar 29 2019, 2:11 PM

On second thought, dunno. In the scan-build macro preview it wouldn't show you UINT32_MAX anyway. Maybe let's keep this behavior.

Cleaned up the patch a little bit.

In D59121#1448367, @NoQ wrote:

On second thought, dunno. In the scan-build macro preview it wouldn't show you UINT32_MAX anyway. Maybe let's keep this behavior.

Cleaned up the patch a little bit.

Somehow on the Assuming ... pieces we write out the macro name, and my little code did that too. We could obtain the value but I think your first intention is the correct behaviour. Why are you not using my working example?

NoQ added a comment.Mar 29 2019, 2:23 PM

Mmm, that *is* an Assuming... piece, i.e., this is the same code, just the structure of macros is more complicated than usual.

In D59121#1448386, @NoQ wrote:

Mmm, that *is* an Assuming... piece, i.e., this is the same code, just the structure of macros is more complicated than usual.

You told me we would like to see a value when we hover over a name, which is cool. If I think about C and they do not C# over the complicated macros, I would like to C names of macros. It is up to you, but your first intention working with my code very well.

NoQ updated this revision to Diff 192917.Mar 29 2019, 2:46 PM

Also, this is kinda weird. According to my logic, we should have written Assuming 'i' is equal to 4294967295 because that's what the user will see in the macro popup. However, that's incorrect for the same reason: i is an int, while 4294967295 doesn't fit into an int, so they can never be equal. Writing Assuming 'i' is equal to UINT32_MAX is incorrect for the same reason, and the current message is the only one that's technically the truth.

Writing what exactly we're assuming is, in my opinion, more important than repeating what's literally written in the code.

Added a FIXME test to document this problem.

Charusso accepted this revision.Mar 29 2019, 3:14 PM

Nice catch!

Szelethus accepted this revision.Apr 10 2019, 3:11 PM

LGTM! Thanks!

This revision is now accepted and ready to land.Apr 10 2019, 3:11 PM
This revision was automatically updated to reflect the committed changes.