Page MenuHomePhabricator

[clang-tidy] Share the forced linking code between clang-tidy tool and plugin
ClosedPublic

Authored by yvvan on Dec 12 2018, 1:40 AM.

Diff Detail

Event Timeline

yvvan created this revision.Dec 12 2018, 1:40 AM
  1. Please always upload all patches with full context.
  2. There are two places where this pattern exists - this file, and tool/ClangTidyMain.cpp. It clearly leads to such issues, Can this be reworked to be just one file that is simply included in both places?
yvvan planned changes to this revision.Dec 12 2018, 1:51 AM
  1. Please always upload all patches with full context.
  2. There are two places where this pattern exists - this file, and tool/ClangTidyMain.cpp. It clearly leads to such issues, Can this be reworked to be just one file that is simply included in both places?
  1. Usually I do that but this time used tortoisesvn to generate the patch...
  2. Makes sense.
yvvan updated this revision to Diff 177829.Dec 12 2018, 2:22 AM
yvvan retitled this revision from Add missing bugprone checks to clang-tidy plugin to Share the forced linking code between clang-tidy tool and plugin.
yvvan edited the summary of this revision. (Show Details)
lebedev.ri added inline comments.Dec 12 2018, 2:27 AM
clang-tidy/ClangTidyForceLinker.h
2

Standard prologue missing

yvvan updated this revision to Diff 177832.Dec 12 2018, 3:04 AM
yvvan marked an inline comment as done.

Add standard prologue to the new header

lebedev.ri accepted this revision.Dec 12 2018, 3:14 AM

Thanks, LG.
You probably may want to wait a bit (a day?) in case others want to comment.

This revision is now accepted and ready to land.Dec 12 2018, 3:14 AM

FYI, CMake target property INTERFACE_SOURCES is designed to make this easy.

For each module you would generate a file containing

extern volatile int ${MODULE_NAME}ModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED ${MODULE_NAME}ModuleAnchorDestination =
    ${MODULE_NAME}ModuleAnchorSource;

and then put that generated file in the INTERFACE_SOURCES of each module.

target_sources(${MODULE_NAME} INTERFACE ${THE_GENERATED_FILE}.cpp)

Then, you don't need to maintain it in C++ like this. It is DRY because the target_link_libraries entry for the library is what causes the symbol to be used.

JonasToth retitled this revision from Share the forced linking code between clang-tidy tool and plugin to [clang-tidy] Share the forced linking code between clang-tidy tool and plugin.Dec 12 2018, 3:24 AM
JonasToth added reviewers: aaron.ballman, hokein.
JonasToth set the repository for this revision to rCTE Clang Tools Extra.
JonasToth added a project: Restricted Project.

FYI, CMake target property INTERFACE_SOURCES is designed to make this easy.

For each module you would generate a file containing

extern volatile int ${MODULE_NAME}ModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED ${MODULE_NAME}ModuleAnchorDestination =
    ${MODULE_NAME}ModuleAnchorSource;

and then put that generated file in the INTERFACE_SOURCES of each module.

target_sources(${MODULE_NAME} INTERFACE ${THE_GENERATED_FILE}.cpp)

Then, you don't need to maintain it in C++ like this. It is DRY because the target_link_libraries entry for the library is what causes the symbol to be used.

That sounds like a better solution, but i think the current pattern is common in LLVM, maybe @aaron.ballman or @alexfh could share their opinion on this one?

Can you say where else it is common in LLVM? I'm curious. Maybe those places could be changed too.

Can you say where else it is common in LLVM? I'm curious. Maybe those places could be changed too.

My "is common" is not quantified, I have seen it before :)

From grepping "extern volatile":

  • clang-tidy a lot
  • clang/lib/Tooling/CompilationDatabase.cpp:398
  • clang/lib/Tooling/Execution.cpp:100
  • clang/tools/libclang/CIndex.cpp:8681
  • clang/tools/libclang/CIndex.cpp:8675

Thats probably all places or is same idiom implementable with something else?

One thing: Could you please check the utility-scripts in clang-tidy that create new checks, if they need adjustments? Not sure if we have something in there that relies on the old structure.

yvvan added a comment.Dec 13 2018, 6:17 AM

One thing: Could you please check the utility-scripts in clang-tidy that create new checks, if they need adjustments? Not sure if we have something in there that relies on the old structure.

As far as I see these scripts do not touch 'tool' or 'plugin' folders at all, only the specific module, docs and tests.

JonasToth accepted this revision.Dec 13 2018, 6:31 AM

LGTM, chatted with steveire on IRC: blocking this for a better cmake based solution makes no sense. So your free to commit :)

This revision was automatically updated to reflect the committed changes.
yvvan marked an inline comment as done.Dec 13 2018, 12:00 PM
yvvan added inline comments.
clang-tidy/plugin/ClangTidyPlugin.cpp
11

It seems it had to go after #include "clang/Config/config.h" to have MPI checks enabled...

lebedev.ri added inline comments.Dec 13 2018, 12:04 PM
clang-tidy/plugin/ClangTidyPlugin.cpp
11

The includes should be auto-sorted by clang-format, so if that header needs some headers,
then it should include them, not hope they will already be included.

yvvan marked an inline comment as done.Dec 13 2018, 12:07 PM
yvvan added inline comments.
clang-tidy/plugin/ClangTidyPlugin.cpp
11

True. Will fix tomorrow asap.

dyung added a subscriber: dyung.Dec 13 2018, 6:19 PM

I have reverted this in r349121 as it has been causing the PS4 bots to be red for hours now.

dyung marked an inline comment as done.Dec 13 2018, 6:20 PM
dyung added inline comments.
clang-tidy/ClangTidyForceLinker.h
11

Is there a reason that this header file does not use the standard header guards?

11

Sorry, this should not have been marked as done.

yvvan marked an inline comment as done.Dec 13 2018, 11:26 PM
yvvan added inline comments.
clang-tidy/ClangTidyForceLinker.h
11

I'm adding them together with the include.

yvvan marked an inline comment as done.Dec 13 2018, 11:42 PM
yvvan added inline comments.
clang-tidy/plugin/ClangTidyPlugin.cpp
11

Damn, "config.h" is special and can only be included once.