Page MenuHomePhabricator

[PassManager/Sanitizer] Enable usage of ported AddressSanitizer passes with -fsanitize=address
ClosedPublic

Authored by leonardchan on Oct 2 2018, 4:53 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Is this the right place in the pipeline to put the passes? The legacy PM inserts the asan passes at EP_OptimizerLast, why not do the same thing?

fedor.sergeev added inline comments.Oct 4 2018, 3:16 AM
lib/CodeGen/BackendUtil.cpp
1031–1038 ↗(On Diff #168054)

I dont believe CGSCC is appropriate here.

I would expect to see a simple ModuleToFunction adapter, something like this:

MPM.addPass(AddressSanitizerModulePass());
MPM.addPass(createModuleToFunctionPassAdaptor(AddressSanitizerFunctionPass())

Also, it seems that legacy runs Function sanitizer first and then Module sanitizer after it.
This sequence does it backwards. Is it intentional?

leonardchan marked an inline comment as done.

Is this the right place in the pipeline to put the passes? The legacy PM inserts the asan passes at EP_OptimizerLast, why not do the same thing?

I noticed this also and thought adding this using registerScalarOptimizerLateEPCallback was the initial correct way to add this function pass, but it seems that the vector of callbacks this gets added to (ScalarOptimizerLateEPCallbacks in buildFunctionSimplificationPipeline) doesn't get called unless some sort of optimization is requested (that is I run with -fexperimental-new-pass-manager -fsanitize=address -O1) when the sanitizer should still be applied even without optimization.

We could force this pipeline to be run if we run PB.buildPerModuleDefaultPipeline()regardless of the optimization level, but I don't know why it's only called if optimization is zero.

lib/CodeGen/BackendUtil.cpp
1031–1038 ↗(On Diff #168054)

My bad. I didn't see there was a createModuleToFunctionPassAdaptor earlier and used CGSCC since that seemed to be how other function passes were added to the module.

I also wasn't sure if the order mattered since it seems both passes can be run independently of each other and provide different instrumentation. I think @vitalybuka might be able to answer that better, Regardless, I switched them still so Module runs after Function.

You're right, my bad; I missed the fact that it's EP_OptimizerLast, which has no equivalent in the new PM. Your implementation is actually correct here.

Did you run your test in a debug build?

You're right, my bad; I missed the fact that it's EP_OptimizerLast, which has no equivalent in the new PM. Your implementation is actually correct here.

Did you run your test in a debug build?

Yes, and I tested it on release also in case there were any differences between either builds.

@philip.pfaffe @fedor.sergeev Do you have any more comments on this patch?

philip.pfaffe accepted this revision.Oct 17 2018, 4:43 AM

LGTM, thank you!

This revision is now accepted and ready to land.Oct 17 2018, 4:43 AM
This revision was automatically updated to reflect the committed changes.