[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

[Balázs Benics via Phabricator via cfe-commits]

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

This preprocessor expansion stuff is definitely not my expertise, nvm here is my review.

However, I observed some strange things happening in the test-cases, that's why I //request changes//.
I hope that I messed something up at the evaluation of your tests, but please have a closer look at them.

There were a few nits, but nothing serious.
I also liked the previous discussion about the reusable components, and I understand your decision going this way. I'm fine with that.
Keep up your good work. We need this.



================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:883
 
+/// Wrapper around a Lexer object that can lex tokens one-by-one. Optionally,
+/// one can "inject" a range of tokens into the stream, in which case the next
----------------
`Optionally` -> `Additionally`?


================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:892-894
+    std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(ExpanLoc);
+    const llvm::MemoryBuffer *MB = SM.getBuffer(LocInfo.first);
+    const char *MacroNameTokenPos = MB->getBufferStart() + LocInfo.second;
----------------
It should be more readable if you use `tie`. That way you can give names to the parts.


================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:896-898
+    RawLexer.reset(new Lexer(SM.getLocForStartOfFile(LocInfo.first), LangOpts,
+                             MB->getBufferStart(), MacroNameTokenPos,
+                             MB->getBufferEnd()));
----------------
I'm always puzzled if I see a naked `new`.
Couldn't we use the assignment operator and `std::make_unique` here?


================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:901
+
+  void next(Token &Result) {
+    if (CurrTokenIt == TokenRange.end()) {
----------------
I don't like output parameters.
If we really need them, we should at least have a suspicious name.
Unfortunately, I can not come up with anything useful :|


================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1150-1161
   // Acquire the macro's arguments at the expansion point.
   //
   // The rough idea here is to lex from the first left parentheses to the last
   // right parentheses, and map the macro's parameter to what they will be
   // expanded to. A macro argument may contain several token (like '3 + 4'), so
   // we'll lex until we find a tok::comma or tok::r_paren, at which point we
   // start lexing the next argument or finish.
----------------
By //lexing// one might think we use the actual lexer. Should we change this comment?


================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1338-1339
 void TokenPrinter::printToken(const Token &Tok) {
+  // TODO: Handle the case where hash and hashhash occurs right before
+  // __VA_ARGS__.
+
----------------
What does //hashhash// mean? I might lack some context though :D


================
Comment at: clang/test/Analysis/plist-macros-with-expansion.cpp:481
+  i = 0;
+#define DISPATCH(...) PARAMS_RESOLVE_TO_VA_ARGS(__VA_ARGS__);
+
----------------
You don't need an ending semicolon here. It will be already there at the expansion location.
This way you introduce an empty expression after the macro expansion.
The same happens in all the other cases as well.


================
Comment at: clang/test/Analysis/plist-macros-with-expansion.cpp:484-486
+  int x = 1;
+  DISPATCH(x, "LF1M healer");
+  (void)(10 / x); // expected-warning{{Division by zero}}
----------------
Should we really abuse the division by zero checker here?
Can't we just use an ExprInspection call here?
Maybe it requires a specific BugPath visitor, and that is why we do it this way?


================
Comment at: clang/test/Analysis/plist-macros-with-expansion.cpp:511
+// CHECK: <key>name</key><string>CONCAT_VA_ARGS</string>
+// CHECK-NEXT: <key>expansion</key><string>variadicCFunction(x, "You need to construct",);x = 0;</string>
+
----------------
How did that comma appear there?
https://godbolt.org/z/4En3E5


================
Comment at: clang/test/Analysis/plist-macros-with-expansion.cpp:517-524
+void stringifyVA_ARGS(void) {
+  int x = 1;
+  STRINGIFIED_VA_ARGS(x, "Additional supply depots required.", 'a', 10);
+  (void)(10 / x); // expected-warning{{Division by zero}}
+}
+
+// CHECK: <key>name</key><string>STRINGIFIED_VA_ARGS</string>
----------------
This test case is also bad.
https://godbolt.org/z/89s48K


================
Comment at: clang/test/Analysis/plist-macros-with-expansion.cpp:526-533
+void stringifyVA_ARGSEmpty(void) {
+  int x = 1;
+  STRINGIFIED_VA_ARGS(x, "Additional supply depots required.");
+  (void)(10 / x); // expected-warning{{Division by zero}}
+}
+
+// CHECK: <key>name</key><string>STRINGIFIED_VA_ARGS</string>
----------------
Also. https://godbolt.org/z/8c3dxP


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86135/new/

https://reviews.llvm.org/D86135