Page MenuHomePhabricator

[Sanitizer] Fix failing sanitizer tests
ClosedPublic

Authored by yln on Feb 5 2021, 10:06 PM.

Details

Summary

The new pass manager was enabled by default [1].

The commit message states the following relevant differences:

  • The inliner works slightly differently
  • -O1 does some amount of inlining

These tests are affected because they specify -O1 and then check the
reported stack trace.

[1] https://reviews.llvm.org/D95380

Diff Detail

Event Timeline

yln requested review of this revision.Feb 5 2021, 10:06 PM
yln created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2021, 10:06 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
jroelofs accepted this revision.Feb 6 2021, 11:18 AM
jroelofs added a subscriber: jroelofs.

LGTM

This revision is now accepted and ready to land.Feb 6 2021, 11:18 AM
This revision was landed with ongoing or failed builds.Feb 8 2021, 9:59 AM
This revision was automatically updated to reflect the committed changes.
yln added a subscriber: kcc.Feb 8 2021, 11:36 AM

@kubamracek pointed out that the optimization level in these tests might be of significance: ensuring that things work correctly with optimizations enabled.

Kostya usually has the last word for these kind of issues. @kcc Would you like me to revert this change and fix the tests in a different way?

  • Adapt stack traces
  • Try to prevent inlining via noinline attributes
  • Other way?
kcc added a comment.Feb 8 2021, 4:00 PM

yea, I am afraid that removing -O1 weakens our ability to find subtle bugs in how sanitizers work with the optimized code.
After all, most of the uses for the sanitizers are with -O1 and higher, so by testing with -O0 we are hiding potential problems.
I think the best is to prevent inlining (noinline attribute, or a command line if available)