This is an archive of the discontinued LLVM Phabricator instance.

[bazel] Don't alwayslink clang-tidy libraries
ClosedPublic

Authored by aaronmondal on Mar 3 2023, 10:57 AM.

Details

Summary

These libraries are only ever used in clang-tidy itself, so there is no
need to unconditionally keep all symbols.

Diff Detail

Event Timeline

aaronmondal created this revision.Mar 3 2023, 10:57 AM
Herald added a project: Restricted Project. · View Herald Transcript
aaronmondal requested review of this revision.Mar 3 2023, 10:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2023, 10:57 AM
aaronmondal added reviewers: GMNGeoffrey, jathu, Restricted Project.Mar 3 2023, 10:59 AM

I would've thought it was necessary to alwayslink them because maybe they use some kind of auto-registration system into clang-tidy, so without alwayslink your clang-tidy binary wouldn't link any of them in? But I could be wrong, maybe they're explicitly referenced in the clang-tidy binary?

I would've thought it was necessary to alwayslink them because maybe they use some kind of auto-registration system into clang-tidy, so without alwayslink your clang-tidy binary wouldn't link any of them in?

I think the reason alwayslink was in here was because https://reviews.llvm.org/D143804 also took https://github.com/eomii/rules_ll/commit/678b1e4698aca2c8d5767c95e5245e60921754e into consideration, which originally copied the general structure from https://github.com/llvm/llvm-project/blob/main/utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl.

As far as I can remember this was just an oversight in the original implementation. I wasn't aware of the implications of alwayslink until @GMNGeoffrey mentioned it in https://reviews.llvm.org/D143804. But the clang-tidy "modules" don't seem to work like plugins and a Bazel-built clang-tidy executable seems to work fine without this flag.

But I could be wrong, maybe they're explicitly referenced in the clang-tidy binary?

I think this is what the ClangTidyForceLinker.h file (also included by ClangTidyMain.cpp) does.

I would've thought it was necessary to alwayslink them because maybe they use some kind of auto-registration system into clang-tidy, so without alwayslink your clang-tidy binary wouldn't link any of them in? But I could be wrong, maybe they're explicitly referenced in the clang-tidy binary?

There is no always-link in cmake as far as I know, so we should not need any in Bazel either in the LLVM project hopefully.

There is no always-link in cmake as far as I know, so we should not need any in Bazel either in the LLVM project hopefully.

It seems that only mlir actually needs this and there are custom build files for that.

The only actual usage of the cc_plugin_library which explicitly exposes alwayslink is for the libclang target, but its left to default (False) there. I think it should be fine to remove alwayslink from the llvm and clang build files entirely.

I would've thought it was necessary to alwayslink them because maybe they use some kind of auto-registration system into clang-tidy, so without alwayslink your clang-tidy binary wouldn't link any of them in? But I could be wrong, maybe they're explicitly referenced in the clang-tidy binary?

There is no always-link in cmake as far as I know, so we should not need any in Bazel either in the LLVM project hopefully.

Ah, fair enough. It was just a vague guess at why this might've been included in the first place. If it doesn't apply - no worries.

GMNGeoffrey accepted this revision.Mar 6 2023, 11:23 AM
This revision is now accepted and ready to land.Mar 6 2023, 11:23 AM

Hmmm actually our internal version of these build files also uses alwayslink with the comment "Registration of the clang-tidy checker modules relies on the linker, so we need to enforce linking". That comment was written in 2014 though, so things may have changed. Let me try seeing what tests fail if I remove that internally.

Hmmm actually our internal version of these build files also uses alwayslink with the comment "Registration of the clang-tidy checker modules relies on the linker, so we need to enforce linking". That comment was written in 2014 though, so things may have changed. Let me try seeing what tests fail if I remove that internally.

I tried removing it internally and nothing fails, so I think this is vestigial. Let's kill it!

This revision was automatically updated to reflect the committed changes.