Page MenuHomePhabricator

[CrossTU] Fix plist macro expansion if macro in other file.
ClosedPublic

Authored by balazske on Jul 12 2019, 7:53 AM.

Details

Summary

When cross TU analysis is used it is possible that a macro expansion
is generated for a macro that is defined (and used) in other than
the main translation unit. To get the expansion for it the source
location in the original source file and original preprocessor
is needed.

Diff Detail

Repository
rL LLVM

Event Timeline

balazske created this revision.Jul 12 2019, 7:53 AM
Szelethus accepted this revision.Jul 12 2019, 8:22 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Jul 12 2019, 8:22 AM
balazske added a subscriber: ilya-biryukov.

@ilya-biryukov Please check if it is acceptable to use ASTUnit in PlistDiagnostic.cpp.

StaticAnalyzer/Core does not depend on clangFrontend now, you can see this by looking at lib/StaticAnalyzer/Core/CMakeLists.txt:

add_clang_library(clangStaticAnalyzerCore
...
  LINK_LIBS
  clangAST
  clangASTMatchers
  clangAnalysis
  clangBasic
  clangCrossTU
  clangLex
  clangRewrite
  )

Not a StaticAnalyzer expert, so I don't know whether it's acceptable to add this dependency to clangStaticAnalyzerCore, you'll have to find someone who owns the code to know whether this dependency is justified.
(My wild guess from looking at the names of the libraries would be that this dependency is not ok and the code should go into clangStaticAnalyzerFrontend instead. But again, not an expert here, just a guess).

But please add a dependency into LINK_LIBS inside CMakeLists.txt if you start depending on clangFrontend.
Most of these violations are found if you build in a cmake -DBUILD_SHARED_LIBS=On configuration.

martong added a subscriber: NoQ.Jul 23 2019, 6:54 AM

StaticAnalyzer/Core does not depend on clangFrontend now, you can see this by looking at lib/StaticAnalyzer/Core/CMakeLists.txt:

add_clang_library(clangStaticAnalyzerCore
...
  LINK_LIBS
  clangAST
  clangASTMatchers
  clangAnalysis
  clangBasic
  clangCrossTU
  clangLex
  clangRewrite
  )

Not a StaticAnalyzer expert, so I don't know whether it's acceptable to add this dependency to clangStaticAnalyzerCore, you'll have to find someone who owns the code to know whether this dependency is justified.
(My wild guess from looking at the names of the libraries would be that this dependency is not ok and the code should go into clangStaticAnalyzerFrontend instead. But again, not an expert here, just a guess).

But please add a dependency into LINK_LIBS inside CMakeLists.txt if you start depending on clangFrontend.
Most of these violations are found if you build in a cmake -DBUILD_SHARED_LIBS=On configuration.

@Szelethus @NoQ Could StaticAnalyzer/Core depend on clangFrontend? I am not sure if we can get the Preprocessor somewhere else ...

It is possible to use the Preprocessor instead of ASTUnit in getImportedFromSourceLocation. But this will contain less useful (even if currently not used) information. The test extension in D65064 is not possible to do if not ASTUnit is used. (Or a tuple must be used that contains Preprocessor and TranslationUnitDecl and maybe other objects that are stored in the ASTUnit.)

Maybe CrossTU should not use the Frontend library? (Now the StaticAnalyzerCore is using CrossTU and CrossTU is using Frontend, in this way StaticAnalyzerCore could use Frontend too.)

NoQ added a comment.Jul 23 2019, 1:01 PM

The preprocessor is defined in Lex, so i guess i'm curious about a more precise description of what's impossible to do without Frontend.

The Frontend is needed because "ASTUnit.h" is added now into PlistDiagnostics.cpp. The ASTUnit object is used to pass information out from getImportedFromSourceLocation. The StaticAnalyzerCore library uses CrossTU library, and CrossTU uses Frontend, so I think it may be not a problem if StaticAnalyzerCore will use directly Frontend. Otherwise the getImportedFromSourceLocation can return the Preprocessor (and ASTContext) (in a tuple) instead of an ASTUnit and use of ASTUnit is not needed. (Patch in D65064 contains a test that does work only if the TranslationUnitDecl of the imported-from AST is known, to get this value an ASTUnit or ASTContext must be returned from getImportedFromSourceLocation.)

balazske updated this revision to Diff 211689.Jul 25 2019, 1:25 AM

Added clangFrontend to CMakeLists.txt.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2019, 3:52 AM

@balazske This is causing buildbot failures - revert?

Problem should be fixed now (r367013).