This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][PlistMacroExpansion] Part 1.: New expand-macros flag
ClosedPublic

Authored by Szelethus on Oct 1 2018, 12:34 PM.

Details

Summary

This is the first part of the implementation of the inclusion of macro expansions into the plist output.

This approach is very much different to what HTMLRewrite does -- the motivation behind this was that

  • I really wanted to avoid const_cast,
  • This patch aims to only expand macros relevant to the bugpath, so there's no point in lexing everything.

Sadly, it seems like I couldn't get away with a solution not re-implementing parts of the preprocessor -- macro arguments are expanded manually and somewhat painfully, but I am very confident about it's stability and correctness.

How I'm planning to do this:

  • A new analyzer option was added to turn macro expansions on.
  • BugReporter will compact the bugpath when that flag is enabled, thus creating PathDiagnosticMacroPieces. (fun fact: they were never created for plists before!)
  • (Implemented in a separate patch) When a macro is emitted,
    1. a lexer is created to acquire the macro's name and arguments.
    2. The actual definition of the macro can be acquired from the Preprocessor.
      1. Should one of the tokens of the definition the parameter of the current macro, it has to be replaced manually with the information we gathered by lexing.
      2. Should one of them be be a macro, it will be expanded recursively. The information about parameters is passed on.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Oct 1 2018, 12:34 PM

We need your all-seeing hawk eye!

Szelethus edited the summary of this revision. (Show Details)Oct 1 2018, 1:23 PM
Szelethus edited the summary of this revision. (Show Details)
Szelethus edited the summary of this revision. (Show Details)Oct 1 2018, 2:56 PM
whisperity added a subscriber: gamesh411.EditedOct 2 2018, 1:38 AM

Your code looks good, just minor comments going on inline.
Trying to think of more cases to test for, in case someone generously misuses FLMs, as seen here in an example if you scroll down a bit: https://gcc.gnu.org/onlinedocs/cpp/Macro-Arguments.html#Macro-Arguments
Maybe one or two tests cases for this should be added, but I believe all corners are covered other than this.

I also very much like the idea of only lexxing out relevans macros.

Could you please run some tests on our usuals and upload a few cases (or a plist-to-html of your test file analysed) so we can see how nicely these relevant macros are handled?

(The whole thing, however, is generally disgusting. I'd've expected the Preprocessor to readily contain a lot of stuff that's going on here.)

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
689 ↗(On Diff #167803)

Returns with true

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
261 ↗(On Diff #167803)

move move move like in your other self-defined type below

292 ↗(On Diff #167803)

the location of what?

298 ↗(On Diff #167803)

the ranges of what?

668 ↗(On Diff #167803)

You have two records named "MacroArgsInfo" and "MacroNameAndArgsInfo", this is confusing. A different name should be for this class here... maybe the later used MacroArgMap is good, and then the struct's field should be renamed ArgMap or just Args.

680 ↗(On Diff #167803)

move string argument into local member (See https://www.bfilipek.com/2018/08/init-string-member.html, it's fascinating and me and @gamesh411 just found this again yesterday night.)

711–712 ↗(On Diff #167803)

Indentation of these lines is wrong.

723 ↗(On Diff #167803)

"this is a" or "macro is function-like"

725 ↗(On Diff #167803)

its

726 ↗(On Diff #167803)

What's a "\"super\" macro"? Maybe "outer" (or "outermost") is the word you wanted to use here?

737 ↗(On Diff #167803)

Unnecessary ; at the end?

756 ↗(On Diff #167803)

&& "Ill-formed input: macro args were never closed!"

765 ↗(On Diff #167803)

Token& so copies are explicitly not created?

796 ↗(On Diff #167803)

Okay, this looks nasty. I get that pointer offsetting is commutative, but... this is nasty.

Also, what's the difference between using .data() and .begin()? Lexer's this overload takes three const char* params. Maybe this extra variable here is useless and you could just pass BufferInfo.data() + LocInfo.second to the constructor below.

796 ↗(On Diff #167803)

Or we need a better name or a comment here.

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

Why do we use the HTML entity tag ' instead of '? Is the PList output expanded this much?

Szelethus updated this revision to Diff 167924.Oct 2 2018, 4:54 AM
  • Fixes according to @whisperity's comments
  • New test cases for
    • commas in brackets,
    • commas in braces,
    • macro arguments left empty.

(The whole thing, however, is generally disgusting. I'd've expected the Preprocessor to readily contain a lot of stuff that's going on here.)

Don't even get me started.

Szelethus marked 13 inline comments as done.Oct 2 2018, 4:58 AM
Szelethus added inline comments.
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
292 ↗(On Diff #167803)

This is fairly obvious from the context, and is used throughout the whole file, so I'm leaving this one as is.

737 ↗(On Diff #167803)

It is actually necessary ^-^

765 ↗(On Diff #167803)

I've seen Token being copied all over the place in other files, but after looking at it's implementation, const ref should be better.

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

Yup, unfortunately.

Szelethus marked 8 inline comments as done.Oct 2 2018, 4:59 AM
donat.nagy added a subscriber: donat.nagy.EditedOct 2 2018, 6:41 AM

What would happen if this checker encounters a variadic macro (introduced to the standard in in C++11, see syntax (3) and (4) here) or the # and ## preprocessor operators?

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
755 ↗(On Diff #167924)

I fear that this does not handle nested parentheses correctly; e.g. in a case like MACRO_ONE(foo, MACRO_TWO(bar, baz), spam), it does not skip , spam).

Szelethus updated this revision to Diff 167966.Oct 2 2018, 10:02 AM
  • Prettified the RUN: lines,
  • Fixed a crash with variadic macros,
  • Added TODOs to handle # and ##,
  • Added an expected plist output.

I initially justified the size of this patch due to how intertwined everything is, but given that there's still some work left to do, I might end up splitting it up.

What would happen if this checker encounters a variadic macro (introduced to the standard in in C++11, see syntax (3) and (4) here) or the # and ## preprocessor operators?

Spot on observation, but it'll probably be the task of a followup patch.

Szelethus added inline comments.Oct 2 2018, 10:04 AM
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
755 ↗(On Diff #167924)

And rightfully so, nice catch!

Szelethus updated this revision to Diff 167992.Oct 2 2018, 11:57 AM
Szelethus retitled this revision from [analyzer][WIP] Add macro expansions to the plist output to [analyzer][PlistMacroExpansion] Part 1.: New expand-macros flag.
Szelethus edited the summary of this revision. (Show Details)
Szelethus removed reviewers: xazax.hun, dcoughlin.
Szelethus set the repository for this revision to rC Clang.

I initially justified the size of this patch due to how intertwined everything is, but given that there's still some work left to do, I might end up splitting it up.

I wouldn't enjoy reviewing a patch of this size very much, so I decided to split this up into 3 parts. This one will only contain the new flag.

whisperity added inline comments.Oct 3 2018, 1:25 AM
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
6 ↗(On Diff #167992)

You are diffing against a hardcoded output file which contains a git hash! Won't this break at the next commit?!

Szelethus marked 3 inline comments as done.Oct 3 2018, 4:20 AM
Szelethus added inline comments.
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
6 ↗(On Diff #167992)

I'm actually using %diff_plist!

// Check the actual plist output.
//   RUN: cat %t.plist | %diff_plist \
//   RUN:   %S/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist

Here's the patch that implemented it (should jump to the config file): D52036#change-mOw5EmE2OiVA

Szelethus updated this revision to Diff 168320.Oct 4 2018, 9:47 AM
Szelethus marked an inline comment as done.
xazax.hun added inline comments.Oct 9 2018, 2:16 AM
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
6 ↗(On Diff #167992)

Even if it will not break the build I would prefer to remove the non-official fork/mirror links from the test file.

whisperity added inline comments.Oct 9 2018, 3:12 AM
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
346 ↗(On Diff #168320)

Same here, as @xazax.hun mentioned. (Damn when I make a checker I'll be careful to ensure my paths don't contain swearwords... 😜 )

Maybe we should carefully analyse what is exactly needed from the plist and throw the rest, just to make the whole file smaller. Plists are pretty bloaty already...

Removed the version entry from the plist file.

NoQ added inline comments.Oct 16 2018, 3:28 PM
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
469 ↗(On Diff #167803)

Should we say something about plists in the option name?

test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
44–54 ↗(On Diff #169858)

Because we're adding an element of an <array> rather than a key of a <dict>, I'm not entirely sure this is backwards compatible. Clients may crash if they iterate over the path array and encounter an unexpected element kind. Is it going to be bad for your use case if we put expansions into a separate array alongside the path array?

Szelethus marked 3 inline comments as done.Oct 18 2018, 8:56 AM
Szelethus added inline comments.
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
469 ↗(On Diff #167803)

Sure, but should my AnalyzerOptions refactoring effort go through first, I intend emit warnings if flags aren't set correctly (eg. the output isn't set to plist but this flag is enabled). Should probably rename "serialize-stats" too then.

http://lists.llvm.org/pipermail/cfe-dev/2018-October/059842.html

test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
44–54 ↗(On Diff #169858)

It shouldn't be :)

Szelethus updated this revision to Diff 170509.EditedOct 22 2018, 3:20 PM

Added a new macro_expansions key on the same level at path and notes, under which the macro expansions are listed, as suggested by @NoQ.

There were a couple ways to make this happen. I could've changed how macro pieces are made in BugReporter, so that they are created but the path isn't compacted, but then the BugReporter's implementation would be complicated by doing specific things for specific output formats. I instead decided to manually flatten macro pieces in PlistDiagnostics.

Since FlushDiagnosticsImpl was already super long, I decided to move both the logic of this patch and the logic for notes into a separate function.

Szelethus marked 2 inline comments as done.Oct 22 2018, 3:21 PM
NoQ added inline comments.Oct 25 2018, 2:25 PM
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
471 ↗(On Diff #170509)

Yup, np, let's wait until it lands then.

test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
17–18 ↗(On Diff #170509)

They look empty. I don't think this is intended.

NoQ accepted this revision.Oct 25 2018, 2:26 PM
NoQ added inline comments.
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
17–18 ↗(On Diff #170509)

Oh, wait, right, those are not implemented yet!

This revision is now accepted and ready to land.Oct 25 2018, 2:26 PM
Szelethus planned changes to this revision.EditedOct 26 2018, 7:24 AM

@xazax.hun observed that the way diagnostics looks like is this:

diagnostics
  report 1
    notes
    macro_expansions
    path
    executed_lines
  report 2
    ...

Buuuut, if I didn't insist on this structure, but rather print macros at the end of the report, we wouldn't need all this hackery (converting an immutable container into a mutable one, and then actually modifying the path). This however does imply that macro pieces have to be collected, which would imply the need for adding yet another parameter to every single function in this file. I think the time has come to collect them in a class, so I'll put this patch on hold.

Mind you, the rest of the patches don't contain logic that depends on this patch.

Changes according to my last comment.

This revision is now accepted and ready to land.Oct 29 2018, 10:16 AM
xazax.hun accepted this revision.Oct 31 2018, 6:50 AM

LGTM! Thanks, I think it is much easier to understand what is going on this way.

Szelethus marked 2 inline comments as done.Oct 31 2018, 7:51 AM
Szelethus added inline comments.
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
471 ↗(On Diff #170509)

I'll commit as is, because D53277 is blocked by D53276, and I'd very much like to get this project out of the way as soon as possible.

This revision was automatically updated to reflect the committed changes.