This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Use the MacroExpansionContext for macro expansions in plists
ClosedPublic

Authored by steakhal on Dec 14 2020, 8:19 AM.

Details

Summary

Removes the obsolete ad-hoc macro expansions during bugreport constructions.
It will skip the macro expansion if the expansion happened in an imported TU.

Also removes the expected plist file, while expanding matching context for
the tests.
Adds a previously crashing plist-macros-with-expansion.c testfile.
Temporarily marks plist-macros-with-expansion-ctu.c to XFAIL.

Diff Detail

Event Timeline

steakhal created this revision.Dec 14 2020, 8:19 AM
steakhal requested review of this revision.Dec 14 2020, 8:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2020, 8:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steakhal retitled this revision from [RFC] Use the MacroExpansionContext for macro expansions in plists to [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists.Dec 14 2020, 8:27 AM
martong added inline comments.Dec 15 2020, 4:05 AM
clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
0

I wonder how much added value do we have with these huge and clumsy plist files... We already have the concise unittests, which are quite self explanatory. I'd just simply delete these plist files.

martong added inline comments.Dec 15 2020, 4:09 AM
clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
0

Perhaps in the test cpp file we should just execute a FileCheck for the expansions. We are totally not interested to check the whole contents of the plist, we are interested only to see if there were expansions.

xazax.hun accepted this revision.Dec 15 2020, 5:23 AM

Yay! Deleting code is always nice.

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

We do need some plist tests to ensure that the correct plist format is emitted. How much of those do we need might be up for debate.

This revision is now accepted and ready to land.Dec 15 2020, 5:23 AM

Internally we analyzed 16 projects with this patch-stack. No reports were changed!
Now we don't crash on macro expansions in our test set, yey.

I've checked a few expansions and seemed to be readable enough.

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

It's certainly a pain to keep all the locations in sync with the code.

steakhal added inline comments.Dec 16 2020, 3:45 AM
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
828–836

I don't like that I will ruin the CTU expansions - even temporarily.
I will probably commit this patch together with the 4th one, where I will use the same macro expansion mechanism for CTU too.
So these two patches are for 'logical' separation.

NoQ added inline comments.Dec 17 2020, 6:58 PM
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
20–21

Will these two includes be ultimately removed? Like, even in the CTU case?

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

I'm absolutely in favor of not testing the entire plist files whenever possible. Just keep some minimal context available so that to not accidentally match the wrong test. Yes we obviously need some tests for the entire plist files but we already have a lot of that.

steakhal planned changes to this revision.Dec 23 2020, 4:47 AM
steakhal added inline comments.
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
20–21

I think PlistDiagnostics can depend on CTU.
ASTUnit is a different thing though. I think I just accidentally left that include.

I will remove the #include "clang/Frontend/ASTUnit.h" line.


What do you mean by the 'CTU case'?

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

Ok, I will remove these files.

NoQ added inline comments.Dec 23 2020, 8:16 PM
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
20–21

I think PlistDiagnostics can depend on CTU.

So, like, my problem is that i want to move all PathDiagnosticConsumers (with implementations) into libAnalysis so that other tools could use it but libAnalysis can't link-time-depend on libCrossTU because that'd be a circular dependency. Basically libAnalysis is a tiny library that contains analyses whereas libCrossTU is a high-level library that manages entire clang invocations. Unless we rethink our library hierarchies entirely, i believe we're stuck with this constraint.

Removing the dependency on libFrontend is insufficient because libCrossTU depends on libFrontend anyway.

If you make MacroExpansionContext abstract and put the concrete implementation in libCrossTU, thus replacing my (currently reverted) attempt in D92432 to break the dependency, that'd fix the problem entirely. Otherwise i'll probably wait for your work to land and do this myself anyway.

What do you mean by the 'CTU case'?

Nothing really (: It was an attempt to emotionally highlight the importance of not having a link-time dependency on libCrossTU even while handling the CTU execution path, according to whatever definition of that you were alluding to in the review summary.

steakhal updated this revision to Diff 316139.Jan 12 2021, 9:40 AM
steakhal marked 4 inline comments as done.

Updates:

  • Rebase stack.
  • Add FileCheck context for plist-macros-with-expansion.cpp.
  • Remove the Inputs/expected-plists/plist-macros-with-expansion.cpp.plist file.
  • Create an extra test file, testing C related stuff, such as Implicit function declaration in plist-macros-with-expansion.c.
  • Return None for CTU macro expansions instead of an arbitrary string.
This revision is now accepted and ready to land.Jan 12 2021, 9:40 AM

Updates:

  • Rebased.

Unfortunately, I could not come up with a proper CTU implementation.
It seems that when we load the AST/dump, no preprocessor events are replayed.
Without those events, my PPCallbacks implementation and tokenwatcher would not record anything, drawing macro expansion useless for CTU.

How should I continue to get this working with CTU?
@xazax.hun @martong @balazske
Previously we had some sort of macro expansions (and crashes), after these patches we would not anymore :(

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
20–21

After you land D92432, these two includes could be substituted by clang/Analysis/CrossTUAnalysisHelper.h.

How should I continue to get this working with CTU?

We have two CTU modes. One loading the dump and the other loading from AST source file. I assume that you refer to the former? Could you validate that this solution works for the latter?
If we cannot make CTU fully functional, is it possible to at least make it work for the current TU (as opposed to functions inlined from another TU)?

While it would be great to have a fully working solution, not having macro expansions in a single relatively narrow scenario might worth the trade-off to get rid of the related bugs and throw out a half-baked reimplementation of the preprocessor.

If we really want to make it work for the CTU mode with dumps, we could dump the macro expansions to a separate file together with the AST and read it back for the error reporting.

How should I continue to get this working with CTU?

We have two CTU modes. One loading the dump and the other loading from AST source file. I assume that you refer to the former?

Yes, I was referring to that.

Could you validate that this solution works for the latter?

Sure, I suspect that will have the required PP events since the Preprocessor is parsing source code :D
I'm going to check this just to be sure.

If we cannot make CTU fully functional, is it possible to at least make it work for the current TU (as opposed to functions inlined from another TU)?

This element of the patch stack implements exactly that.

While it would be great to have a fully working solution, not having macro expansions in a single relatively narrow scenario might worth the trade-off to get rid of the related bugs and throw out a half-baked reimplementation of the preprocessor.

Even in the CTU case, the main TU is still parsed via the main Preprocessor - hence we will track the macro expansions accordingly.
I would definitely favor this 'option' instead of dropping this patch stack.
I mean, it really fixes a bunch of crashes and expands macros much more accurately especially for the corner-cases.

If we really want to make it work for the CTU mode with dumps, we could dump the macro expansions to a separate file together with the AST and read it back for the error reporting.

If we chose this path, the local getExpandedMacro function needs to be simplified. That would help Artem's dependency issue.

Could you validate that this solution works for the latter [ctu-on-demand]?

Sure, I suspect that will have the required PP events since the Preprocessor is parsing source code :D
I'm going to check this just to be sure.

On-demand parsing does indeed populate the required PP events.
The 4-th element of this patch stack will aim to implement macro expansions for that scenario.

If we really want to make it work for the CTU mode with dumps, we could dump the macro expansions to a separate file together with the AST and read it back for the error reporting.

I don't know. Theoretically yes, but I'm not sure how much of a breaking change would it be.
Do we really promise ABI compatibility for AST dumps? If we support it to some extent, then we would have a problem - which would mean that we can not target clang-12 anymore with this patch stack.

Do we really promise ABI compatibility for AST dumps? If we support it to some extent, then we would have a problem - which would mean that we can not target clang-12 anymore with this patch stack.

I don't think so. As far as I remember we only support consuming AST dumps that were generated with the same version of the compiler. When a compiler is upgraded the user is expected to regenerate all the dumps.

It seems quite a challenge to hook the Preprocessor for all possible configurations for every CompilerInvocation.
The underlying machinery is somewhat complex and spaghetti to me.

Here is what I suggest:
For now, this expansion is better than the previous was. Macro expansions will work for the main TU even in any CTU configuration. For the imported TUs, it will just not expand any macros.
This is a regression in some way, but we should live with that until we implement it completely.
I think the users would prefer a non-crashing partial implementation to a crashing one.

If you think it's an appropriate, I will update the CTU tests to expect no macro expansion in any imported TU.
And also remove the XFAIL.
This way we could still target clang-12.

Do you think it's acceptable?
@xazax.hun @NoQ

Do you think it's acceptable?
@xazax.hun @NoQ

It is fine by me. I believe, currently is Ericsson the biggest contributor and user of the CTU functionality. So in case they are onboard with your plans, that should be OK.

It seems quite a challenge to hook the Preprocessor for all possible configurations for every CompilerInvocation.
The underlying machinery is somewhat complex and spaghetti to me.

Here is what I suggest:
For now, this expansion is better than the previous was. Macro expansions will work for the main TU even in any CTU configuration. For the imported TUs, it will just not expand any macros.
This is a regression in some way, but we should live with that until we implement it completely.
I think the users would prefer a non-crashing partial implementation to a crashing one.

If you think it's an appropriate, I will update the CTU tests to expect no macro expansion in any imported TU.
And also remove the XFAIL.
This way we could still target clang-12.

Do you think it's acceptable?
@xazax.hun @NoQ

Sounds good to me!

steakhal retitled this revision from [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists to [analyzer] Use the MacroExpansionContext for macro expansions in plists.Jan 22 2021, 4:03 AM
steakhal edited the summary of this revision. (Show Details)
Szelethus accepted this revision.EditedFeb 9 2021, 7:24 PM

Happy to see this mess go. It was obvious even after the first few hurdles that it wouldn't work out in the long term.

edit: I have no immediate comments regarding CTU.

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

Nice.

steakhal marked an inline comment as done.Feb 10 2021, 2:56 AM

Yey, thanks @Szelethus.