This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Port Sancov
ClosedPublic

Authored by leonardchan on Jun 4 2019, 7:35 PM.

Details

Summary

This patch contains a port of SanitizerCoverage to the new pass manager. This one's a bit hefty.

Changes:

  • Split SanitizerCoverageModule into 2 SanitizerCoverage for passing over functions and ModuleSanitizerCoverage for passing over modules.
  • ModuleSanitizerCoverage exists for adding 2 module level calls to initialization functions but only if there's a function that was instrumented by sancov.
  • Added legacy and new PM wrapper classes that own instances of the 2 new classes.
  • Update llvm tests and add clang tests,

Diff Detail

Event Timeline

leonardchan created this revision.Jun 4 2019, 7:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2019, 7:35 PM
chandlerc requested changes to this revision.Jun 10 2019, 5:36 PM

I would just change this to have the module pass loop over the functions -- that seems like it'll be much cleaner.

As it is, I'm not seeing where the loop actually happens. But rather than trying to figure that out, just seems better to turn it into an explicit loop. That way you can get rid of all the check analysis overhead I think.

This revision now requires changes to proceed.Jun 10 2019, 5:36 PM
leonardchan edited the summary of this revision. (Show Details)

I would just change this to have the module pass loop over the functions -- that seems like it'll be much cleaner.

As it is, I'm not seeing where the loop actually happens. But rather than trying to figure that out, just seems better to turn it into an explicit loop. That way you can get rid of all the check analysis overhead I think.

Done. This simplifies the patch a lot more.

chandlerc requested changes to this revision.Jun 12 2019, 8:19 PM

This was a lot easier for me to understand too, thanks. Somewhat minor code changes below.

clang/lib/CodeGen/BackendUtil.cpp
1231–1232

Is there an existing function pass pipeline we can put this in, maybe by using an extension point? That would make it much faster to run I suspect.

llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h
6–7

Need to use the new file header.

15

I feel like we usually have whitespace here too?

25

Not really a wrapper, just the pass itself...

SanitizerCoverage is an implementation detail that the reader here doesn't really know about.

38

Same comment as above.

llvm/lib/Passes/PassRegistry.def
248

Consistency would suggest just sancov here? I don't feel strongly though.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
228–229

This should be in the legacy pass below? (actually, I tihnk it already is, so this can likely be removed)

262–269
if (!llvm::any_of(M, [](const Function &F) { return can InstrumentWithSancov(F); }))
  return false;
442

I completely misread this the first time through thinking you used a common object for state....

Maybe change the constructor to accept a function to make it more obvious that this is intended to be built per-function?

This revision now requires changes to proceed.Jun 12 2019, 8:19 PM
leonardchan marked 9 inline comments as done.
leonardchan added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
1231–1232

I tried this earlier, but when I use registerOptimizerLastEPCallback() before the default module pipeline is made, I end up getting this linker error for -O2 runs:

fuzzer.c:(.text.sancov.module_ctor_8bit_counters[sancov.module_ctor_8bit_counters]+0x11): undefined reference to `__start___sancov_pcs'
fuzzer.c:(.text.sancov.module_ctor_8bit_counters[sancov.module_ctor_8bit_counters]+0x16): undefined reference to `__stop___sancov_pcs'

I don't understand how I get this because the symbols are declared in the IR dump as:

@__start___sancov_pcs = external hidden global i64*
@__stop___sancov_pcs = external hidden global i64*

for both the optimized cases if I use the adaptor or EP. I'm still linking with the same compiler-rt in each case.

The only difference I think of using the adaptor after buildPerModuleDefaultPipeline() vs using the extension point callback before is that the order in which the pass will be called changes, although I still don't see how it could lead to this undefined reference if I still link against compiler-rt.

Additionally, the only difference between the IR between using the adaptor vs the EP is a few extra globals/declarations seem to be missing when using the EP, which makes me think another pass that ran after this one removed them, although these don't seem to be related to the error.

llvm/lib/Passes/PassRegistry.def
248

Ah, so the reason why I don't use sancov here is because in the tests when using `-passes='function(sancov)', I get the following error:

/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-b133876586/bin/opt: unknown function pass '/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-b133876586/bin/sancov'

I think this is because lit tries to substitute sancov with the one under bin/sancov in my build dir. I'd be up for changing it if there's a way around this.

chandlerc added inline comments.Jun 18 2019, 7:47 PM
clang/lib/CodeGen/BackendUtil.cpp
1231–1232

Hmm...

Maybe those missing globals / declarations are relevant. Maybe they are necessary to get the linking against the sancov runtime to work correctly?

You could try adding these globals / declarations to the 'used' set so they don't get removed by later passes:
https://llvm.org/docs/LangRef.html#the-llvm-compiler-used-global-variable

Otherwise, I have no idea, maybe makes sense to loop in the sanitizer folks explicitly for help debugging this...

llvm/lib/Passes/PassRegistry.def
248

That's... really entertaining.

I suspect the tool should really be llvm-sancov to avoid this kind of thing.

But anyways, i'm OK with this or calling it something else. There is probably a way to hide a thing from lit's substitution as well, I just don't know what it is....

leonardchan marked 3 inline comments as done.
leonardchan added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
1231–1232

Ah ok. I did not know about this global before. So it seems that the problem was related to the __sancov_pcs section being optimized out. Adding __sancov_gen_ to llvm.compiler.used fixes this.

I did a little extra digging and I think it's ConstantMergePass or some other pass that works with this that seems to cause this.

The used thing still seems like there is an underlynig bug here. See below.

(Also a tiny nit, but that isn't the interesting part.)

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
206

Maybe call these getFooBarImpl so that you don't have to rely on name lookup trickery below to refer to them unambiguously?

754

There is a GlobalsToAppendToCompilerUsed vector that we append Array to below (line 759). Why isn't that sufficient instead of this?

leonardchan marked 5 inline comments as done.
leonardchan added inline comments.
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
754

Ah, I didn't actually see this for some reason. The reason why it wasn't working before was bc I left the code that called

if (TargetTriple.isOSBinFormatMachO())
  appendToUsed(M, GlobalsToAppendToUsed);
appendToCompilerUsed(M, GlobalsToAppendToCompilerUsed);

in initializeModule, so this never got appended. Fixed by moving this to a finalizeModule which runs after the function runs end.

chandlerc accepted this revision.Jul 11 2019, 2:39 PM

LGTM, thanks so much for sticking through this, I know it was ... nontrivial!

This revision is now accepted and ready to land.Jul 11 2019, 2:39 PM
This revision was automatically updated to reflect the committed changes.
abrachet marked an inline comment as done.Jul 12 2019, 3:37 AM
abrachet added a subscriber: abrachet.
abrachet added inline comments.
llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
1022–1026 ↗(On Diff #209358)

buildbots are complaining about warnings from these being unused.

leonardchan marked an inline comment as done.Jul 12 2019, 11:09 AM
leonardchan added inline comments.
llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
1022–1026 ↗(On Diff #209358)

Thanks for notifying. Removed them in r365931.