This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][CTU] API for CTU macro expansions
ClosedPublic

Authored by steakhal on Jan 14 2021, 4:36 AM.

Details

Summary

Removes CrossTranslationUnitContext::getImportedFromSourceLocation
Removes the corresponding unit-test segment.

Introduces the CrossTranslationUnitContext::getMacroExpansionContextForSourceLocation
which will return the macro expansion context for an imported TU. Also adds a
few implementation FIXME notes where applicable, since this feature is
not implemented yet. This fact is also noted as Doxygen comments.

Uplifts a few CTU LIT test to match the current incomplete behavior.

It is a regression to some extent since now we don't expand any
macros in imported TUs. At least we don't crash anymore.

Note that the introduced function is already covered by LIT tests.
Eg.: Analysis/plist-macros-with-expansion-ctu.c

Diff Detail

Event Timeline

steakhal created this revision.Jan 14 2021, 4:36 AM

Probably the CrossTranslationUnitContext::ImportedFileIDs can be removed. The FileIDImportHandler in ASTImporter was used only for this (if I know correctly) too but it can be useful if a similar mechanism is needed later, but it remains there as "dead code" if not removed.

Probably the CrossTranslationUnitContext::ImportedFileIDs can be removed. The FileIDImportHandler in ASTImporter was used only for this (if I know correctly) too but it can be useful if a similar mechanism is needed later, but it remains there as "dead code" if not removed.

Excellent catch!

steakhal updated this revision to Diff 316861.Jan 15 2021, 12:57 AM
steakhal edited the summary of this revision. (Show Details)
steakhal set the repository for this revision to rG LLVM Github Monorepo.

Update: resolve @balazske's comment D94673#2498165.

  • remove ASTImporter::FileIDImportHandler etc.
  • updates the revision's summary accordingly
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
vsavchenko added inline comments.Jan 15 2021, 1:04 AM
clang/include/clang/CrossTU/CrossTranslationUnit.h
186–187

I think it's missing "location" here

188–189

I don't think that this piece of information should be here, it took me a bit to get it. Maybe it can be a separate \note?

steakhal updated this revision to Diff 316873.Jan 15 2021, 1:42 AM
steakhal marked 2 inline comments as done.

Updated the comment of CrossTranslationUnitContext::getMacroExpansionContextForSourceLocation.

steakhal added inline comments.Jan 15 2021, 1:45 AM
clang/include/clang/CrossTU/CrossTranslationUnit.h
191

is

steakhal edited the summary of this revision. (Show Details)Jan 22 2021, 4:04 AM
steakhal updated this revision to Diff 323025.Feb 11 2021, 8:06 AM

Actually, somehow I messed up something previously. Now, the diff contains the latest version.
Sorry for the inconvenience!

Rebased on top of 2407eb08a5748bc2613e95fa449fc1cae6f4ff8f.

Szelethus accepted this revision.Feb 16 2021, 8:17 AM

LGTM! You may wanna wait for someone more knowledgeable about CTU to give it another accept.

This revision is now accepted and ready to land.Feb 16 2021, 8:17 AM
balazske accepted this revision.Feb 17 2021, 3:43 AM
balazske added inline comments.
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
835

I am not sure if getExpandedText will handle a source location that is not in the same TU. Probably the previous source location mapping mechanism (that is now removed) is needed additionally, so that here the original source location and the original MacroExpansionContext is available. Probably this can be done in other way, the MacroExpansionContext could handle this mapping. (A single MacroExpansionContext could handle every imported source location at least for on-demand parsing.)

steakhal added inline comments.Feb 17 2021, 6:36 AM
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
835

Yes. For a complete CTU implementation, it seems inevitable to be able to acquire the proper MacroExpansionContext for an imported source-location. So the previous mapping will be likely back once we implement this functionality completely.

I think we can not implement it via having a single MacroExpansionContext due to layering violations.
I wanted this to be part of the Analysis library, but if it was concerned about CTU importing and stuff it would reside inside the cross_tu library.
I might be wrong about this though.