Page MenuHomePhabricator

[SanitizerCoverage] Drop !associated on metadata sections
ClosedPublic

Authored by MaskRay on Feb 24 2021, 5:44 PM.

Details

Summary

In SanitizerCoverage, the metadata sections (__sancov_guards,
__sancov_cntrs, __sancov_bools) are referenced by functions. After
inlining, such a __sancov_* section can be referenced by more than one
functions, but its sh_link still refers to the original function's section.
(Note: a SHF_LINK_ORDER section referenced by a section other than its linked-to
section violates the invariant.)

If the original function's section is discarded (e.g. LTO internalization +
ld.lld --gc-sections), ld.lld may report a sh_link points to discarded section error.

This above reasoning means that !associated is not appropriate to be called by
an inlinable function. Non-interposable functions are inline candidates, so we
have to drop !associated. A __sancov_pcs is not referenced by other sections
but is expected to parallel a metadata section, so we have to make sure the two
sections are retained or discarded at the same time. A section group does the
trick. (Note: we have a module ctor, so getUniqueModuleId guarantees to
return a non-empty string, and GetOrCreateFunctionComdat guarantees to return
non-null.)

For interposable functions, we could keep using !associated, but
LTO can change the linkage to internal and allow such functions to be inlinable,
so we have to drop !associated, too. To not interfere with section
group resolution, we need to use the noduplicates variant (section group flag 0).
(This allows us to get rid of the ModuleID parameter.)
In -fno-pie and -fpie code (mostly dso_local), instrumented interposable
functions have WeakAny/LinkOnceAny linkages, which are rare. So the
section group header overload should be low.

This patch does not change the object file output for COFF (where !associated is ignored).

Diff Detail

Event Timeline

MaskRay created this revision.Feb 24 2021, 5:44 PM
MaskRay requested review of this revision.Feb 24 2021, 5:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2021, 5:44 PM

Thanks for the quick fix!

llvm/lib/Transforms/Instrumentation/Instrumentation.cpp
81–82

this comment does not match condition now

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
686–688

Why removing these lines does not change COFF behaviour ?

MaskRay updated this revision to Diff 326268.Feb 24 2021, 7:17 PM
MaskRay marked 2 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Comments.

MaskRay added inline comments.Feb 24 2021, 7:17 PM
llvm/lib/Transforms/Instrumentation/Instrumentation.cpp
81–82

Thanks. BTW: we can delete the ModuleId block entirely.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
686–688

I'll amend the description:

This patch does change the object file output for COFF (!associated is ignored).

LGTM
If no more comment I'll accept tomorrow?

llvm/lib/Transforms/Instrumentation/Instrumentation.cpp
81–82

So it was added in D51902 and replaced with NoDuplicates for COFF in D54232

morehouse added inline comments.Feb 25 2021, 9:19 AM
llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol-nocomdat.ll
1

This test now has comdats. Should we rename the test?

MaskRay updated this revision to Diff 326451.Feb 25 2021, 11:24 AM
MaskRay marked an inline comment as done.

Precommit test improvement
Rename a test

This revision is now accepted and ready to land.Feb 25 2021, 11:28 AM
rnk accepted this revision.Feb 25 2021, 11:32 AM

What you have described is generally true, and it is why static locals cannot be part of the comdat group of an inline function (inline int f() { static int gv; return ++gv; }). Groups (and !associated, I guess, although I'm not super familiar) really only work for metadata applications, where the relocation edges go from the associated data towards the externally visible things. It is very difficult to construct a comdat group with more than one externally visible symbol. See all the confusion over the D5 comdat group.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
686–688

I agree, it is redundant for COFF. The comdat group is what establishes the association links.

vitalybuka accepted this revision.Feb 25 2021, 11:45 AM
MaskRay edited the summary of this revision. (Show Details)Feb 25 2021, 11:58 AM
This revision was landed with ongoing or failed builds.Feb 25 2021, 11:59 AM
This revision was automatically updated to reflect the committed changes.
ychen added a subscriber: ychen.May 25 2021, 3:41 PM
ychen added inline comments.
llvm/lib/Transforms/Instrumentation/Instrumentation.cpp
76

Internally, I saw this failure https://github.com/llvm/llvm-project/blob/a6a57f03be40cf2c546805155624a206649092ec/llvm/lib/Linker/LinkModules.cpp#L181 that may be caused by this for a test built with -fprofile-generate -flto for ELF. According to the title and description of the patch, that seems an unintended behavior change. Just check in to see if it is obvious how to fix this. Will provide the reproducer once it is available.

MaskRay added inline comments.May 25 2021, 4:46 PM
llvm/lib/Transforms/Instrumentation/Instrumentation.cpp
76

Is it called by the legacy LTO implementation LTOCodeGenerator::addModule? I guess new LTO does not exercise the code path.

I guess the fix is to ignore deduplication feature in ModuleLinker::run ComdatsChosen. comdat nodeduplicates was added to ELF this year.