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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D86135#2235473 <https://reviews.llvm.org/D86135#2235473>, @Szelethus wrote:

> In D86135#2233611 <https://reviews.llvm.org/D86135#2233611>, @martong wrote:
>
>>> The fundamental problem is, we simply can't ask Preprocessor what a macro expands into without hacking really hard.
>>
>> Can you summarize what is the exact problem (or give a link to a discussion, etc)? Is it an architectural problem in Clang itself?
>
> I phrased myself poorly. The Preprocessor can tell what a macro usage immediately expands into (otherwise this project would have been practically impossible to implement), what it struggles to get done is show what a macro usage in the source code turns into in the preprocessed translation unit. As is often the case, macros may be nested into one another:
>
>   #define MACRO2(ptr) ptr = 0
>   #define MACRO1(x) MACRO2(x)
>   
>   int *a = get();
>   MACRO1(a); // Step 1. Expand MACRO1.
>              //        a.) Retrieve the tokens defined for MACRO1.
>              //        b.) Resolve parameter x to argument a.
>              // Step 2. Expand MACRO2...
>   *a = 5;
>
> From this code snippet, should we be interested in what `MACRO1(a)` expands into, `Preprocessor` can pretty much only deliver on point 1a.) with any convenience. The problem here is that it was simply never engineered to be used outside of the preprocessing stage, so much so, that by the time I worked on this project about a decade after `Preprocessor`'s inception, I still had to turn super trivial methods const. Other than that, the discussions I linked in the summary is pretty much all I can offer.
>
>> Could we somehow refactor Clang and the Preprocessor to be usable for us? I mean LLVM and Clang has the mindset to build reusable components, the Preprocessor (and the Sema) should be one of them too, not just the AST.
>
> Working with the `Preprocessor` seems incredibly tedious and error prone -- mind that this is almost literally the first thing we implemented in Clang, and you can tell. Also, its performance critical, and any meaningful performance impact may need strong justification. While it would be beneficial to have the knowledge I'm crafting being integrated into the actual class, its hard to justify all the effort it would take to do so, especially now this project is so far along anyways. If I has to start from scratch, I would try to explore other approaches, but might still end up just doing what I did here.
>
> In D86135#2233695 <https://reviews.llvm.org/D86135#2233695>, @xazax.hun wrote:
>
>> Is this related to https://bugs.llvm.org/show_bug.cgi?id=44493?
>
> To some extent, but this patch unfortunately doesn't solve that bug. The problem I think is that named variadic macros aren't supported at all.

Thanks, for the detailed explanation, makes it easier to understand the reasons!



================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1227
+          if (TheTok.getIdentifierInfo() == __VA_ARGS__II) {
+            TStream.injectRange(PrevParamMap.at(__VA_ARGS__II));
+            TStream.next(TheTok);
----------------
Why do we have to push back the tokens in case of __VA_ARGS? And what is in PrevParamMap here. Is it possible that `at` can fail here? Perhaps an example could make this hunk way easier to understand. To be honest, this hunk is a mystique for me in this form. 


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

https://reviews.llvm.org/D86135