-
Notifications
You must be signed in to change notification settings - Fork 68
feat: Support running Clang Tools in-process via getActionType #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…cally load plugins during the compilation process.
…esses; modified the invocation method of the CodeStyleChecker plugin for lit test cases and adjusted the corresponding lit tests.
This is fantastic, thank you so much for returning to this! 🙏🏻 🏅 Please see my comments inline - nothing major, just some minor suggestions to make this excellent contributions a bit more consistent with clang-tutor :) |
Thank you for your acknowledgment, but I don't seem to find your inline comments at https://github.com/banach-space/clang-tutor/pull/36/files. Could you please advise how I should proceed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your acknowledgment, but I don't seem to find your inline comments at https://github.com/banach-space/clang-tutor/pull/36/files. Could you please advise how I should proceed?
Apologies, I forgot to submit these last night.
// expected-no-diagnostics | ||
class SomeClass { | ||
public: | ||
operator bool(); | ||
operator int(); | ||
operator char(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets keep these tests :)
// expected-warning@+1 {{Function names should start with lower-case letter}} | ||
void ClangTutorFuncBad(); | ||
|
||
void clangTutorFuncOK(); | ||
|
||
struct ClangTutorStruct { | ||
// expected-warning@+1 {{Function names should start with lower-case letter}} | ||
void ClangTutorMemberMethodBad(); | ||
void clangTutorMemberMethodOK(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this duplicating tests from CodeStyleCheckerFunction.cpp?
// RUN: clang -Xclang -load -Xclang %shlibdir/libCodeStyleChecker%shlibext -Xclang -add-plugin -Xclang CSC -c %s -o %t.o | ||
// RUN: test -f %t.o | ||
// RUN: rm %t.o | ||
|
||
// RUN: clang -fplugin=%shlibdir/libCodeStyleChecker%shlibext -c %s -o %t.o | ||
// RUN: test -f %t.o | ||
// RUN: rm %t.o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these additional test lines? This question applies to all test files that you have updated :)
@@ -1,11 +1,22 @@ | |||
// RUN: clang -cc1 -verify -load %shlibdir/libCodeStyleChecker%shlibext -plugin CSC %s 2>&1 | |||
// RUN: clang -cc1 -verify -load %shlibdir/libCodeStyleChecker%shlibext -add-plugin CSC %s 2>&1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep -plugin
instead of -add-plugin
?
PluginASTAction::ActionType getActionType() override { | ||
#ifndef TARGET_CLANG_TOOL | ||
return AddBeforeMainAction; | ||
#else | ||
return CmdlineAfterMainAction; | ||
#endif | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TARGET_CLANG_TOOL
-> CT_CSC_AS_TOOL
(for consistency with the other CMake vars in clang-tutor)
Also, could you document this method? A link to Clang docs would be helpful. Also, what is the difference between AddBeforeMainAction
and CmdlineAfterMainAction
? Is it possible to test the difference? As in, to write a test that we demonstrate the difference between the two?
@@ -40,4 +40,9 @@ foreach( tool ${CLANG_TUTOR_TOOLS} ) | |||
${tool} | |||
"clangTooling" | |||
) | |||
|
|||
# ct action type should be CmdlineAfterMainAction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to expand this comment with some explanation that will say why ct action type should be CmdlineAfterMainAction
. Does this make sense?
Optimized the description of run&compile in the documentation. Co-authored-by: Andrzej Warzyński <[email protected]>
Related Issue:
Issue #33; PR #34
Motivation
When performing static analysis on large-scale C++ projects, a common requirement is to inject analysis tools into the standard compilation pipeline without interrupting it (i.e., to still produce an executable after the analysis). As discussed in #33, the current implementation makes it difficult to run the analysis and complete the full compilation to generate the final binary simultaneously.
Solution
After researching the LLVM/Clang plugin architecture, I found that we can override the virtual method
getActionType()
inPluginASTAction
to modify the plugin's default behavior.By returning
ActionType::AddBeforeMainAction
, our customASTAction
is executed before the compiler's main action (e.g., codegen). This allows the analysis tool to be seamlessly integrated as a standard part of the compilation process, resolving the issue described above.History & Fixes
llvm-lit
testing framework (and being occupied with my graduation ).I'm happy to be contributing to this great tutor again!