search

Ericsson / clang

Cross Translation Unit analysis capability for Clang Static Analyzer. (Fork of official clang at http://llvm.org/git/clang)
http://clang.llvm.org/
Other
15 stars 10 forks source link

Handle CTU diagnostic macro expansions better. #682

Closed balazske closed 5 years ago

balazske commented 5 years ago

If a macro is to be expanded during diagnostic report creation the preprocessor for the original source file (where the macro was imported from) is used instead of the preprocessor for the actual source file (that do not contain correct macro informations). (If the macro is in imported code.)

Szelethus commented 5 years ago

Wow, I would never have guessed it's possible to retrieve the Preprocessor for another AST!

I glanced over the patch and I like what I'm seeing, though it would be great to test some actual macro expansions, something like this:

https://github.com/Ericsson/clang/blob/ctu-clang7/test/Analysis/plist-macros-with-expansion.cpp

balazske commented 5 years ago

Added a test that fails without the fix with the "unsortable locations found" error. The test fails too if the original solution in isBeforeInTranslationUnit is used.

Szelethus commented 5 years ago

Do you refer to this patch?

https://reviews.llvm.org/D59934

balazske commented 5 years ago

Yes, that fix is not correct because it do not find macros in other TU (that has different SourceManager and different macro history that is not imported at AST import).

Szelethus commented 5 years ago

Hmm, I see. Then which patch does this depend on? What does failure of the lit tests mean (segfault, assertion failure, incorrect output)?

martong commented 5 years ago

@Xazax-hun Adding you as a reviewer, because this involves changes in CTU files too and I'd like you to get a glimpse on this before we upstream.

martong commented 5 years ago

run tests (this comment is intended for the jenkins bot)

balazske commented 5 years ago

If the fix is omitted the test fails with:

Unsortable locations found
UNREACHABLE executed at llvm/tools/clang/lib/Basic/SourceManager.cpp:2040!

If the other fix (in isBeforeInTranslationUnit) is used we get incorrect output: Some macros are not expanded (that are defined in the to-be-imported file only) (I think it is possible to get wrongly expanded macros if there is a macro with same name in both files).

balazske commented 5 years ago

The test fails because there is still a problem: Only source locations are handled that are in the "main" source file in a TU but not those that are in headers.

balazske commented 5 years ago

First part: https://reviews.llvm.org/D64554

balazske commented 5 years ago

https://reviews.llvm.org/D64635 https://reviews.llvm.org/D64638