This is an archive of the discontinued LLVM Phabricator instance.

Run Coverage pass before other *San passes under new pass manager
ClosedPublic

Authored by aeubanks on May 10 2020, 6:31 PM.

Details

Summary

This fixes compiler-rt/test/msan/coverage-levels.cpp under the new pass manager (final check-msan test!).
Under the old pass manager, the coverage pass would run before the MSan pass. The opposite happened under the new pass manager. The MSan pass adds extra basic blocks, changing the number of coverage callbacks.

Diff Detail

Event Timeline

aeubanks created this revision.May 10 2020, 6:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2020, 6:31 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
This revision is now accepted and ready to land.May 11 2020, 11:34 AM
vitalybuka accepted this revision.May 11 2020, 12:36 PM
This revision was automatically updated to reflect the committed changes.

This patch seems to lead to the undefined symbol error we're seeing when building fuchsia:

ld.lld: error: relocation refers to a discarded section: __sancov_guards
>>> defined in user-x64-sancov.shlib/obj/third_party/ulib/scudo/scudo.string_utils.cc.o
>>> referenced by vector.h:58 (../../zircon/third_party/ulib/scudo/vector.h:58)
>>>               user-x64-sancov.shlib/obj/third_party/ulib/scudo/scudo.string_utils.cc.o:(scudo::ScopedString::append(char const*, __va_list_tag*))
>>> referenced by vector.h:58 (../../zircon/third_party/ulib/scudo/vector.h:58)
>>>               user-x64-sancov.shlib/obj/third_party/ulib/scudo/scudo.string_utils.cc.o:(scudo::Printf(char const*, ...))

Checking to see if this is could be an us problem or something this patch introduced.

For reference: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8880544155798188656/+/steps/build/0/steps/build_fuchsia/0/steps/ninja/0/steps/zircon/0/logs/raw_io.output_failure_raw_summary_/0

Just to followup on this: We believe the root cause of the error we're running into is that some sancov guards in one comdat are being referenced by symbols in another comdat, so due to linking order, the other comdat in question is referencing a sancov guard that was in a discarded comdat group. I posted full details of this on llvm-dev. We believe we're running into this error now because with this patch, inlining occurs after the sanitizers are run.

Based on some discussion in http://lists.llvm.org/pipermail/llvm-dev/2020-May/141558.html, it seems appropriate that the solution for this is to just run sanitizers after inlining, so we would avoid this error that's brought about by inlining after sanitizer runs. Since this has been breaking us for a few days, and unless other folks don't mind, I'm thinking it might be appropriate to temporarily revert this until there's a fix for either

(1) running sanitizers after inlining or
(2) changing which comdat groups the sancov guards get placed in (such that guards in one group aren't referenced by functions defined in another group)

I'm not sure how long it would take to implement either solution, so I think temporarily reverting might be ok here.

Just to followup on this: We believe the root cause of the error we're running into is that some sancov guards in one comdat are being referenced by symbols in another comdat, so due to linking order, the other comdat in question is referencing a sancov guard that was in a discarded comdat group. I posted full details of this on llvm-dev. We believe we're running into this error now because with this patch, inlining occurs after the sanitizers are run.

Based on some discussion in http://lists.llvm.org/pipermail/llvm-dev/2020-May/141558.html, it seems appropriate that the solution for this is to just run sanitizers after inlining, so we would avoid this error that's brought about by inlining after sanitizer runs. Since this has been breaking us for a few days, and unless other folks don't mind, I'm thinking it might be appropriate to temporarily revert this until there's a fix for either

Maybe the fix is to use registerScalarOptimizerLateEPCallback() instead of registerPipelineStartEPCallback(). But I'm not sure how to test that, could you try that and lmk if that fixes the issue?

(1) running sanitizers after inlining or
(2) changing which comdat groups the sancov guards get placed in (such that guards in one group aren't referenced by functions defined in another group)

I'm not sure how long it would take to implement either solution, so I think temporarily reverting might be ok here.

Sure, feel free to revert.

Just to followup on this: We believe the root cause of the error we're running into is that some sancov guards in one comdat are being referenced by symbols in another comdat, so due to linking order, the other comdat in question is referencing a sancov guard that was in a discarded comdat group. I posted full details of this on llvm-dev. We believe we're running into this error now because with this patch, inlining occurs after the sanitizers are run.

Based on some discussion in http://lists.llvm.org/pipermail/llvm-dev/2020-May/141558.html, it seems appropriate that the solution for this is to just run sanitizers after inlining, so we would avoid this error that's brought about by inlining after sanitizer runs. Since this has been breaking us for a few days, and unless other folks don't mind, I'm thinking it might be appropriate to temporarily revert this until there's a fix for either

Maybe the fix is to use registerScalarOptimizerLateEPCallback() instead of registerPipelineStartEPCallback(). But I'm not sure how to test that, could you try that and lmk if that fixes the issue?

Thanks. Will try this out later.

(1) running sanitizers after inlining or
(2) changing which comdat groups the sancov guards get placed in (such that guards in one group aren't referenced by functions defined in another group)

I'm not sure how long it would take to implement either solution, so I think temporarily reverting might be ok here.

Sure, feel free to revert.

Reverted in e9802aa4221ba3857041c2328639ce2aac0ace67.