This is an archive of the discontinued LLVM Phabricator instance.

Clean up unnecessary dependency
ClosedPublic

Authored by dangyi on Mar 11 2021, 4:03 PM.

Details

Summary

The LINK_COMPONENTS dependency between DebugInfoCodeView and DebugInfoMSF is unnecessary. Breaking them would allow a more fine-controlled distribution.

Diff Detail

Event Timeline

dangyi created this revision.Mar 11 2021, 4:03 PM
dangyi requested review of this revision.Mar 11 2021, 4:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2021, 4:03 PM
RKSimon added a subscriber: RKSimon.

Please can you ensure the diffs include context (git diff -U9999)

The llvm/lib/DebugInfo/CodeView/CMakeLists.txt change is fine. The change llvm/lib/MC/CMakeLists.txt is not.

Deleting a dependency needs to make sure the relevant components still (1) include what they use and (2) link what they use (the dependency for the direct include should be explicit specified).

(2) can be checked by making sure both -DBUILD_SHARED_LIBS=on and -DBUILD_SHARED_LIBS=off configurations still work. -DBUILD_SHARED_LIBS=on on Linux enables -Wl,-z,defs which requires every
shared object to have full direct dependencies specified (gold and ld.lld behavior; not in GNU ld). This makes for a more reliable build.

(1) is tricky to inspect. Bazel layering_check (-fmodule-name=X -fmodules-strict-decluse) can ensure an error: module X does not depend on a module exporting 'string.h' if a header file not mentioned by modulemap files is included.
MC actually includes DebugInfo/CodeView headers so the removal is not justified. Unfortunately the CMake -DLLVM_ENABLE_MODULES=on build does not specify -fmodule-name=X -fmodules-strict-decluse so this is difficult to enforce.

dangyi updated this revision to Diff 330314.Mar 12 2021, 11:24 AM

Thank you for your detailed explanation. I thought LINK_COMPONENTS merely means, well, link dependency. I'll update the patch.

dangyi updated this revision to Diff 330340.Mar 12 2021, 12:05 PM
dangyi retitled this revision from Clean up unnecessary dependencies to Clean up unnecessary dependency.
dangyi edited the summary of this revision. (Show Details)
MaskRay accepted this revision.Mar 12 2021, 2:51 PM

Thank you for your detailed explanation. I thought LINK_COMPONENTS merely means, well, link dependency. I'll update the patch.

If somehow an inline function in a DebugInfo/CodeView header is moved to the associated .cpp file, a virtual function with non-vague-linkage vtable is needed, etc, the developer may have to add back the dependency.

If you don't have commit access, please provide your name and email https://llvm.org/docs/Phabricator.html#committing-someone-s-change-from-phabricator

This revision is now accepted and ready to land.Mar 12 2021, 2:51 PM
This revision was landed with ongoing or failed builds.Mar 15 2021, 4:29 PM
This revision was automatically updated to reflect the committed changes.