This is an archive of the discontinued LLVM Phabricator instance.

[SanitizerCoverage] Create comdat for global arrays.
ClosedPublic

Authored by morehouse on Sep 10 2018, 5:58 PM.

Event Timeline

morehouse created this revision.Sep 10 2018, 5:58 PM
eugenis accepted this revision.Sep 10 2018, 6:14 PM

LGTM
FTR, http://llvm.org/docs/LangRef.html#associated-metadata mentions that comdat is "recommended".
It would be nice to factor this out into a utility function (create associated global for a global) and use it in asan, too.

This revision is now accepted and ready to land.Sep 10 2018, 6:14 PM

Btw what if getUniqueModuleId returns ""? It may happens for files without external symbols.
I think we need to fall back to non-GCed counters.

morehouse updated this revision to Diff 164787.Sep 10 2018, 7:00 PM
  • Fallback to counter suffix if getUniqueModuleId fails.
morehouse updated this revision to Diff 164888.Sep 11 2018, 8:21 AM
  • Add refactoring TODO.
This revision was automatically updated to reflect the committed changes.
morehouse reopened this revision.Sep 11 2018, 12:44 PM
This revision is now accepted and ready to land.Sep 11 2018, 12:44 PM
  • Only set comdats for ELF.
  • Fallback to using module ID when getUniqueModuleId fails.
  • Rebase properly.
This revision was automatically updated to reflect the committed changes.
eugenis added inline comments.Sep 11 2018, 1:53 PM
llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
582 ↗(On Diff #164958)

I don't think this is correct - getModuleIdentifier() is even less unique then getUniqueModuleId, and comdat names for local linkage functions really have to be unique.

Imagine two files with the same relative path, no exported symbols, defining local functions with the same name. We don't want them to share a comdat.

I think this case needs to fallback to non-comdat non-associated instrumentation (the one that suppressed linker GC).

morehouse added inline comments.Sep 11 2018, 2:12 PM
llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
582 ↗(On Diff #164958)

Ah, I see. Will revert.

Hopefully suppressing GC for that case won't cause any regressions (e.g., https://github.com/google/sanitizers/issues/971).

morehouse added inline comments.Sep 11 2018, 3:39 PM
llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
582 ↗(On Diff #164958)

Hmm... That approach gives me link errors again:

/usr/bin/ld: __sancov_pcs has both ordered [`__sancov_pcs[SSL_COMP_get_name]' in BUILD/libssl.a(ssl_ciph.o)] and unordered [`__sancov_pcs' in BUILD/libssl.a(ssl_ciph.o)] sections

I guess the linker doesn't like having some arrays in comdats and some not in comdats within the same section.

eugenis added inline comments.Sep 11 2018, 4:34 PM
llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
582 ↗(On Diff #164958)

Never seen this error before. Does the linker treat names with a dot as ordered by the part of the name that follows the dot? What if you replace the dot with something else?

morehouse added inline comments.Sep 11 2018, 4:47 PM
llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
582 ↗(On Diff #164958)

I don't think there's any dots in the comdat name or the section name... In the case above the section name is __sancov_pcs and the comdat name is SSL_COMP_get_name.

morehouse reopened this revision.Sep 11 2018, 5:16 PM
This revision is now accepted and ready to land.Sep 11 2018, 5:16 PM
morehouse updated this revision to Diff 165001.Sep 11 2018, 5:16 PM
  • Do not set comdat for local-linkage functions without a unique module ID

The error was actually caused by setting associated metadata for some arrays and not others. By always setting the metadata, we can avoid the issue.

@eugenis: PTAL

eugenis added inline comments.Sep 13 2018, 2:19 PM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
574

Don't call getUniqueModuleId for every function in a module, it's not exactly O(1).

morehouse updated this revision to Diff 165381.Sep 13 2018, 2:29 PM
morehouse marked an inline comment as done.
  • Call getUniqueModuleId once per module.
eugenis accepted this revision.Sep 13 2018, 2:39 PM

LGTM, I'm almost 100% sure this is safe, but I don't remember all the linker quirks around associated sections now.

This revision was automatically updated to reflect the committed changes.