Page MenuHomePhabricator

[analyzer] Create MacroExpansionContext member in AnalysisConsumer
ClosedPublic

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

Details

Summary

Adds a MacroExpansionContext member to the AnalysisConsumer class.
Tracks macro expansions only if the ShouldDisplayMacroExpansions is set.
Passes a reference down the pipeline letting AnalysisConsumers query macro
expansions during bugreport construction.

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 [NFC] Create MacroExpansionContext member in AnalysisConsumer and pass down to the diagnostics consumers to [RFC][analyzer] Create MacroExpansionContext member in AnalysisConsumer and pass down to the diagnostics consumers.Dec 14 2020, 8:27 AM

Looks quite straight-forward other than that new prototype.

clang/include/clang/Analysis/PathDiagnostic.h
911 ↗(On Diff #311596)

Why do we need this prototype here?

xazax.hun added inline comments.Dec 15 2020, 5:21 AM
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
129

This will always record macro related information right? Some of the diagnostic consumers might not ever touch macro expansions. I think it would be great to only record expansions when we actually will end up using them. Alternatively, a benchmark proving the runtime of recording negligible on macro heavy code (boost maybe?) might be sufficient.

Thanks for the review.

clang/include/clang/Analysis/PathDiagnostic.h
911 ↗(On Diff #311596)

Thanks. I probably accidentally let it there :D I will remove this.
I had a hard time figuring out where and how to modify the signature of this.

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
129

To be fair, we already have an analyzer config exactly for this.
If I would introduce a function like registerForPreprocessor(PP), the constructor would be freed up to be essentially a noop.
I think I will follow this path.
This approach would fit nicely in the final patch, where I want to put them in a map. Default constructability would be a nice property.

steakhal updated this revision to Diff 316126.Jan 12 2021, 9:24 AM
steakhal marked 2 inline comments as done.

Updates:

  • New the construction of MacroExpansionContext won't hook the Preprocessor in the constructor. Hooking is done via the registerForPreprocessor(PP) member function.
  • Hooking the PP now depends on the ShouldDisplayMacroExpansions analyzer option.
  • Both getExpandedText and getOriginalText returns Optional<StringRef>. Semantics and comments changed accordingly. If the ShouldDisplayMacroExpansions analyzer flag is not set, both of these functions always return None.
  • Removed the accidental createHTMLSingleFileDiagnosticConsumer prototype in PathDiagnostic.h.
  • New unittest case added NoneForNonExpansionLocations.
  • The rest of the tests were uplifted to unwrap the Optional to accommodate the new API.
  • The last assertion of the RedefUndef unittest case was changed to match the new behavior for non-expansion locations.
steakhal updated this revision to Diff 318482.Jan 22 2021, 4:02 AM
steakhal retitled this revision from [RFC][analyzer] Create MacroExpansionContext member in AnalysisConsumer and pass down to the diagnostics consumers to [analyzer] Create MacroExpansionContext member in AnalysisConsumer.
steakhal edited the summary of this revision. (Show Details)

Nothing changed besides moving some parts to the parent revision.

  • registerForPreprocessor member function
  • Optional<StringRef> return type
  • NoneForNonExpansionLocations unittest-case
martong accepted this revision.Feb 1 2021, 6:15 AM

Still looks good.
(The parent is an NFC and only this patch changes the behavior (and only if the cmdline flag is there), right? So, I'd expect the big impact from this patch.)

This revision is now accepted and ready to land.Feb 1 2021, 6:15 AM
Szelethus accepted this revision.Feb 9 2021, 7:14 PM

My no1. thought is that I wish the new functionality you're implementing was in the interface of the Preprocessor. I found, and still find it so hard to believe that you couldn't just retrieve this information -- your projects seems to plug this gaping hole in the design. I respect that this is an opt-in functionality for now though, and I guess we could make this a default feature with relative ease.

The actual patch is straightforward, LGTM!

My no1. thought is that I wish the new functionality you're implementing was in the interface of the Preprocessor. I found, and still find it so hard to believe that you couldn't just retrieve this information

It is hard to believe, but it is the case.

your projects seems to plug this gaping hole in the design.

I honestly think that it is the supposed way of doing this. The chained preprocessor callbacks and the token watcher fit perfectly into this picture.

I respect that this is an opt-in functionality for now though, and I guess we could make this a default feature with relative ease.

We should carefully evaluate the impact and stability of this before making it happen.

steakhal updated this revision to Diff 323024.Feb 11 2021, 8:02 AM

Nothing changed.
Rebased on top of 2407eb08a5748bc2613e95fa449fc1cae6f4ff8f.