Page MenuHomePhabricator

[analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and primitive expansion
ClosedPublic

Authored by Szelethus on Oct 2 2018, 12:02 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Oct 2 2018, 12:02 PM
MTC added a subscriber: MTC.Oct 3 2018, 12:17 AM
Szelethus updated this revision to Diff 168334.Oct 4 2018, 11:22 AM
  • Removed the version entry from the plist file,
  • Now using TokenConcatenation to print spaces only when needed (this needed for D52988),
  • A bit more doc,
  • Reorganized ("refactored" would be too strong of a word) getExpandedMacroImpl to make it easier to read.
NoQ added inline comments.Oct 16 2018, 4:11 PM
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
727 ↗(On Diff #169862)

Whoa, that's so easy!

787–791 ↗(On Diff #169862)

Not sure, random thought: Could this work in fact be done with the static Lexer::getImmediateMacroName() helper?

test/Analysis/plist-macros-with-expansion.cpp
31 ↗(On Diff #169862)

Hmm, i think we can use CHECK-NEXT: here, in order to make sure these lines go sequentially.

Otherwise this second line may accidentally match someone else's expansion(?).

84 ↗(On Diff #169862)

Typo: arguments.

Szelethus added inline comments.Oct 29 2018, 11:25 AM
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
787–791 ↗(On Diff #169862)

I need to create a lexer to lex the arguments of function-like macros (which is in part 3), so I can't get away with it I'm afraid :/

Please add a test case where a bug path goes through a macro definition and this macro is undefed at the end of the translation unit.

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
667 ↗(On Diff #169862)

Maybe it would be worth to move these helpers to a separate file and add some documentation why we actually simulate the preprocessor rather than doing something else.

724 ↗(On Diff #169862)

Maybe could directly return ExpansionInfo?

734 ↗(On Diff #169862)

I think this local might be redundant.

753 ↗(On Diff #169862)

This might not be completely correct. We are using the preprocessor state after processing the whole translation unit rather than the preprocessor state at the expansion location. This might not cause any problems unless there is a collision between macro names and non-macro names. Also, undefs, redefs might cause troubles.

796 ↗(On Diff #169862)

Could this assertion fail due to undefs in the code?

Szelethus set the repository for this revision to rC Clang.Oct 31 2018, 8:25 AM
Szelethus planned changes to this revision.Oct 31 2018, 8:52 AM

I'll put this patch on hold while I'm investigating the issue @xazax.hun mentioned.

Mind you, again, other patches don't depend strictly on this part, I'll only need to change the for loop. Hopefully. You never know.

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
753 ↗(On Diff #169862)

Yup, you're right, I'm afraid :/

I'll investigate whether I can gather the macro definition at the spelling location with Lexer.

796 ↗(On Diff #169862)

Yes... yes it could.

Fixes according to @xazax.hun's remarks. Thanks for catching that, it was a non-trivial cornercase!

  • Now I'm acquiring the macro directive history, and choosing the appropriate macro definition in getMacroNameAndInfo
  • Rebased to the now commited first part.
Szelethus marked 10 inline comments as done.Oct 31 2018, 10:46 AM
Szelethus added inline comments.
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
667 ↗(On Diff #169862)

I'm actually a little unsure about that -- I have had similar thought, but the placement here can be justified with a simple "Where else?". But that's a little vague, I'll admit.

724 ↗(On Diff #169862)

I renamed the function to be less confusing. Did this address your concern?

753 ↗(On Diff #169862)

I'm really-really happy it didn't have to come to that.

NoQ accepted this revision.Nov 1 2018, 6:57 PM

Because i don't know much about macros, i shouldn't be blocking this review on waiting for myself to understand the code :) I guess we'll eventually figure out if something is wrong with it.

This revision is now accepted and ready to land.Nov 1 2018, 6:57 PM
xazax.hun accepted this revision.Nov 2 2018, 4:49 AM

One question and one nit otherwise looks good. Feel free to commit once those are resolved without another round of reviews.

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
805 ↗(On Diff #171962)

Can't we have troubles with getMacroInfo + undefs here? Wouldn't findDirectiveAtLoc be a safer choice here?

883 ↗(On Diff #171962)

I would remove the inline keyword here. I rarely see this in clang codebase. Let's just trust the compiler :)

Szelethus updated this revision to Diff 172459.EditedNov 2 2018, 4:28 PM
Szelethus marked an inline comment as done.

Thanks! I'll commit when I have time watch buildbots.

This revision was automatically updated to reflect the committed changes.
Szelethus reopened this revision.Nov 4 2018, 6:22 AM

Here's to losing a couple more handfuls of hair, tests break on many platforms, so I reverted it.

This revision is now accepted and ready to land.Nov 4 2018, 6:22 AM
Szelethus closed this revision.Nov 4 2018, 7:21 PM

Woohoo, it seems to be fine! ^-^

I thought the evaluation order in braced initializer lists are from left to right, but apparently not. Certainly not on windows.