[Analyzer] GNU named variadic macros in Plister

[chrish_ericsson_atx]

Summary

Added support for GNU named variadic macros in
macro expansion for plist generation.

Fix for https://bugs.llvm.org/show_bug.cgi?id=44493

[chrish_ericsson_atx]

clang/test/Analysis/plist-macros-with-expansion.cpp

536

Seems the do {} while (0) idiom is a problem, because the plist output gets formatted differently on windows than it does on Linux. (Extra space).

[Szelethus]

The fix is great, thank you so much! The test seems to be annoying, it might be worth looking into it some other time, but that is definitely orthogonal to this patch.

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

1211–1213

That looks so much better.

clang/test/Analysis/plist-macros-with-expansion.cpp

529–545

These FileCheck lines are great, because the plist file on its own is not very readable, could you include them for the new test as well? Side note, we're already thinking that removing the plist file might be a good idea (its a mess to keep up-to-date, and doesn't test all that much).

536

Oh that is a real shame, we absolutely shouldn't do that! Can we somehow provoke a warning through some other way? (null pointer dereference)

[chrish_ericsson_atx]

Addressed feedback from @Szelethus

[chrish_ericsson_atx]

Thanks for the quick feedback, Kristof! I changed the macro to be just a decrement operator, and triggered DBZ with that. Added the checks as you suggested.

[Szelethus]

LGTM! Thanks!

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

getMacroExpansionInfo(const MacroParamMap &PrevParamMap,
 
   // Acquire the macro's name.
   Token TheTok;
   TStream.next(TheTok);
 
   std::string MacroName = PP.getSpelling(TheTok);
 
   const auto *II = PP.getIdentifierInfo(MacroName);
-  assert(II && "Failed to acquire the IndetifierInfo for the macro!");
+  assert(II && "Failed to acquire the IdentifierInfo for the macro!");
 
   const MacroInfo *MI = getMacroInfoForLocation(PP, SM, II, ExpanLoc);
   // assert(MI && "The macro must've been defined at it's expansion location!");
   //
   // We should always be able to obtain the MacroInfo in a given TU, but if
   // we're running the analyzer with CTU, the Preprocessor won't contain the
   // directive history (or anything for that matter) from another TU.
   // TODO: assert when we're not running with CTU.
getMacroExpansionInfo(const MacroParamMap &PrevParamMap,
   // actual macro arguments, or do not represent the macro argument's closing
   // parentheses, so we'll count how many parentheses aren't closed yet.
   // If ParanthesesDepth
   //   * = 0, then there are no more arguments to lex.
   //   * = 1, then if we find a tok::comma, we can start lexing the next arg.
   //   * > 1, then tok::comma is a part of the current arg.
   int ParenthesesDepth = 1;
 
-  // If we encounter __VA_ARGS__, we will lex until the closing tok::r_paren,
-  // even if we lex a tok::comma and ParanthesesDepth == 1.
-  const IdentifierInfo *__VA_ARGS__II = PP.getIdentifierInfo("__VA_ARGS__");
+  // If we encounter the variadic arg, we will lex until the closing
+  // tok::r_paren, even if we lex a tok::comma and ParanthesesDepth == 1.
+  const IdentifierInfo *VariadicParamII = PP.getIdentifierInfo("__VA_ARGS__");
+  if (MI->isGNUVarargs()) {
+    // If macro uses GNU-style variadic args, the param name is user-supplied,
+    // an not "__VA_ARGS__".  E.g.:
+    //   #define FOO(a, b, myvargs...)
+    // In this case, just use the last parameter:
+    VariadicParamII = *(MacroParams.rbegin());
+  }
 
   for (const IdentifierInfo *CurrParamII : MacroParams) {
     MacroParamMap::mapped_type ArgTokens;
 
     // One could also simply not supply a single argument to __VA_ARGS__ -- this
     // results in a preprocessor warning, but is not an error:
     //   #define VARIADIC(ptr, ...) \
     //     someVariadicTemplateFunction(__VA_ARGS__)
     //
     //   int *ptr;
     //   VARIADIC(ptr); // Note that there are no commas, this isn't just an
     //                  // empty parameter -- there are no parameters for '...'.
     // In any other case, ParenthesesDepth mustn't be 0 here.
     if (ParenthesesDepth != 0) {
 
       // Lex the first token of the next macro parameter.
       TStream.next(TheTok);
 
-      while (
-          !(ParenthesesDepth == 1 &&
-            (CurrParamII == __VA_ARGS__II ? false : TheTok.is(tok::comma)))) {
+      while (CurrParamII == VariadicParamII || ParenthesesDepth != 1 ||
+             !TheTok.is(tok::comma)) {
         assert(TheTok.isNot(tok::eof) &&
                "EOF encountered while looking for expanded macro args!");
 
         if (TheTok.is(tok::l_paren))
           ++ParenthesesDepth;
 
         if (TheTok.is(tok::r_paren))
           --ParenthesesDepth;
 
         if (ParenthesesDepth == 0)
           break;
 
         if (TheTok.is(tok::raw_identifier)) {
           PP.LookUpIdentifierInfo(TheTok);
           // This token is a variadic parameter:
           //
           //   #define PARAMS_RESOLVE_TO_VA_ARGS(i, fmt) foo(i, fmt); \
           //     i = 0;
           //   #define DISPATCH(...) \
           //     PARAMS_RESOLVE_TO_VA_ARGS(__VA_ARGS__);
           //                            // ^~~~~~~~~~~ Variadic parameter here
           //
-          //   void mulitpleParamsResolveToVA_ARGS(void) {
+          //   void multipleParamsResolveToVA_ARGS(void) {
           //     int x = 1;
           //     DISPATCH(x, "LF1M healer"); // Multiple arguments are mapped to
           //                                 // a single __VA_ARGS__ parameter.
           //     (void)(10 / x);
           //   }
           //
           // We will stumble across this while trying to expand
           // PARAMS_RESOLVE_TO_VA_ARGS. By this point, we already noted during
           // the processing of DISPATCH what __VA_ARGS__ maps to, so we'll
           // retrieve the next series of tokens from that.
-          if (TheTok.getIdentifierInfo() == __VA_ARGS__II) {
-            TStream.injectRange(PrevParamMap.at(__VA_ARGS__II));
+          if (TheTok.getIdentifierInfo() == VariadicParamII) {
+            TStream.injectRange(PrevParamMap.at(VariadicParamII));
             TStream.next(TheTok);
             continue;
           }
         }
 
         ArgTokens.push_back(TheTok);
         TStream.next(TheTok);
       }
     } else {
-      assert(CurrParamII == __VA_ARGS__II &&
+      assert(CurrParamII == VariadicParamII &&
              "No more macro arguments are found, but the current parameter "
-             "isn't __VA_ARGS__!");
+             "isn't the variadic arg!");
     }
 
     ParamMap.emplace(CurrParamII, std::move(ArgTokens));
   }
 
   assert(TheTok.is(tok::r_paren) &&
          "Expanded macro argument acquisition failed! After the end of the loop"
          " this token should be ')'!");

clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist

    <array>
     <integer>523</integer>
     <integer>524</integer>
     <integer>525</integer>
     <integer>526</integer>
    </array>
   </dict>
   </dict>
+  <dict>
+   <key>path</key>
+   <array>
+    <dict>
+     <key>kind</key><string>control</string>
+     <key>edges</key>
+      <array>
+       <dict>
+        <key>start</key>
+         <array>
+          <dict>
+           <key>line</key><integer>537</integer>
+           <key>col</key><integer>3</integer>
+           <key>file</key><integer>0</integer>
+          </dict>
+          <dict>
+           <key>line</key><integer>537</integer>
+           <key>col</key><integer>5</integer>
+           <key>file</key><integer>0</integer>
+          </dict>
+         </array>
+        <key>end</key>
+         <array>
+          <dict>
+           <key>line</key><integer>539</integer>
+           <key>col</key><integer>3</integer>
+           <key>file</key><integer>0</integer>
+          </dict>
+          <dict>
+           <key>line</key><integer>539</integer>
+           <key>col</key><integer>15</integer>
+           <key>file</key><integer>0</integer>
+          </dict>
+         </array>
+       </dict>
+      </array>
+    </dict>
+    <dict>
+     <key>kind</key><string>event</string>
+     <key>location</key>
+     <dict>
+      <key>line</key><integer>539</integer>
+      <key>col</key><integer>3</integer>
+      <key>file</key><integer>0</integer>
+     </dict>
+     <key>ranges</key>
+     <array>
+       <array>
+        <dict>
+         <key>line</key><integer>539</integer>
+         <key>col</key><integer>3</integer>
+         <key>file</key><integer>0</integer>
+        </dict>
+        <dict>
+         <key>line</key><integer>539</integer>
+         <key>col</key><integer>26</integer>
+         <key>file</key><integer>0</integer>
+        </dict>
+       </array>
+     </array>
+     <key>depth</key><integer>0</integer>
+     <key>extended_message</key>
+     <string>The value 0 is assigned to &apos;a&apos;</string>
+     <key>message</key>
+     <string>The value 0 is assigned to &apos;a&apos;</string>
+    </dict>
+    <dict>
+     <key>kind</key><string>event</string>
+     <key>location</key>
+     <dict>
+      <key>line</key><integer>540</integer>
+      <key>col</key><integer>13</integer>
+      <key>file</key><integer>0</integer>
+     </dict>
+     <key>ranges</key>
+     <array>
+       <array>
+        <dict>
+         <key>line</key><integer>540</integer>
+         <key>col</key><integer>10</integer>
+         <key>file</key><integer>0</integer>
+        </dict>
+        <dict>
+         <key>line</key><integer>540</integer>
+         <key>col</key><integer>15</integer>
+         <key>file</key><integer>0</integer>
+        </dict>
+       </array>
+     </array>
+     <key>depth</key><integer>0</integer>
+     <key>extended_message</key>
+     <string>Division by zero</string>
+     <key>message</key>
+     <string>Division by zero</string>
+    </dict>
+   </array>
+   <key>macro_expansions</key>
+   <array>
+    <dict>
+     <key>location</key>
+     <dict>
+      <key>line</key><integer>539</integer>
+      <key>col</key><integer>3</integer>
+      <key>file</key><integer>0</integer>
+     </dict>
+     <key>name</key><string>BZ44493_GNUVA</string>
+     <key>expansion</key><string>--(a);</string>
+    </dict>
+   </array>
+   <key>description</key><string>Division by zero</string>
+   <key>category</key><string>Logic error</string>
+   <key>type</key><string>Division by zero</string>
+   <key>check_name</key><string>core.DivideZero</string>
+   <!-- This hash is experimental and going to change! -->
+   <key>issue_hash_content_of_line_in_context</key><string>21c6d180d8c8c30cf730b7a7136980a9</string>
+  <key>issue_context_kind</key><string>function</string>
+  <key>issue_context</key><string>bz44493</string>
+  <key>issue_hash_function_offset</key><string>4</string>
+  <key>location</key>
+  <dict>
+   <key>line</key><integer>540</integer>
+   <key>col</key><integer>13</integer>
+   <key>file</key><integer>0</integer>
+  </dict>
+  <key>ExecutedLines</key>
+  <dict>
+   <key>0</key>
+   <array>
+    <integer>536</integer>
+    <integer>537</integer>
+    <integer>538</integer>
+    <integer>539</integer>
+    <integer>540</integer>
+   </array>
+  </dict>
+  </dict>
  </array>
  <key>files</key>
  <array>
  </array>
 </dict>
 </plist>

clang/test/Analysis/plist-macros-with-expansion.cpp

 // CHECK-NEXT: <key>expansion</key><string>variadicCFunction(x, &quot;Additional supply depots required.&quot;,  &quot;&apos;a&apos;&quot;, 10);x = 0;</string>
 
 void stringifyVA_ARGSEmpty(void) {
   int x = 1;
   STRINGIFIED_VA_ARGS(x, "Additional supply depots required.");
   (void)(10 / x); // expected-warning{{Division by zero}}
 }
 
 // FIXME: Stringify and escape __VA_ARGS__ correctly.
 // CHECK: <key>name</key><string>STRINGIFIED_VA_ARGS</string>
 // CHECK-NEXT: <key>expansion</key><string>variadicCFunction(x, &quot;Additional supply depots required.&quot;, &quot;)&quot;;x = 0;</string>
+
+// bz44493: Support GNU-style named variadic arguments in plister
+#define BZ44493_GNUVA(i, args...)  --(i);
+
+int bz44493(void) {
+  int a = 2;
+  BZ44493_GNUVA(a);
+  BZ44493_GNUVA(a, "arg2");
+  (void)(10 / a); // expected-warning{{Division by zero}}
+  return 0;
+}
+
+// CHECK: <key>name</key><string>BZ44493_GNUVA</string>
+// CHECK-NEXT: <key>expansion</key><string>--(a);</string>