This is an archive of the discontinued LLVM Phabricator instance.

[ASan] rename left/right to before/after.
ClosedPublic

Authored by fmayer on Aug 24 2022, 2:45 PM.

Details

Summary

left/right is a weird way to refer to address ordering.

Diff Detail

Event Timeline

fmayer created this revision.Aug 24 2022, 2:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2022, 2:45 PM
Herald added a subscriber: Enna1. · View Herald Transcript
fmayer requested review of this revision.Aug 24 2022, 2:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2022, 2:45 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
fmayer updated this revision to Diff 455384.Aug 24 2022, 2:47 PM

test update

kcc added a subscriber: kcc.Aug 24 2022, 2:48 PM

not arguing for or against, but I wonder how many output parsers will break.

not arguing for or against, but I wonder how many output parsers will break.

not sure about asan, for hwasan we did some quite significant changes to the output format, and no one complained at us.

I think this is a pretty minor change - we are not touching the general structure of the report, nor introducing a new error type (even though the latter happens from time to time).

kcc added a comment.Aug 24 2022, 2:53 PM

not arguing for or against, but I wonder how many output parsers will break.

not sure about asan, for hwasan we did some quite significant changes to the output format, and no one complained at us.

The number of users is different. At the very least I would check with ClusterFuzz

fmayer added a subscriber: enh.
fmayer planned changes to this revision.Aug 24 2022, 5:28 PM

Need to fix tests, sorry, messed something up.

fmayer updated this revision to Diff 455437.Aug 24 2022, 5:48 PM

fix tests

This is fine from my perspective, if we want to be extra cautious it might be worth sending an announcement to Discourse before landing this change.

fmayer added a comment.Sep 1 2022, 2:14 PM

This is fine from my perspective, if we want to be extra cautious it might be worth sending an announcement to Discourse before landing this change.

OK. Posted a notice to #sanitizers on Discord and https://discourse.llvm.org/t/psa-asan-and-hwasan-output-format-changes/65024.

MaskRay accepted this revision.Sep 3 2022, 1:52 PM
MaskRay added a subscriber: MaskRay.

Thanks for starting the post. Personally I think the rename improves clarify.

This revision is now accepted and ready to land.Sep 3 2022, 1:52 PM
eugenis accepted this revision.Sep 6 2022, 10:06 AM

LGTM

This revision was landed with ongoing or failed builds.Sep 6 2022, 1:25 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Sep 8 2022, 4:52 PM

If you want to update this, please do it everywhere.

(I don't think it's clearer and it broke a bunch of our tests, so I'm a bit grumpy about this change. Apologies.)

compiler-rt/lib/asan/asan_allocator.cpp
804

This still says left

compiler-rt/lib/asan/asan_descriptions.cpp
131

So does this.

fmayer marked an inline comment as done.Sep 9 2022, 8:48 AM

If you want to update this, please do it everywhere.

(I don't think it's clearer and it broke a bunch of our tests, so I'm a bit grumpy about this change. Apologies.)

Sorry about breaking your tests, the apologies are mine. Could you share the test that was broken? Was it only tests or also some parser? D133582 sorts out the remaining references to left/right in the asan code.

We had a bunch of gtest death tests to check that the sanitizers are working. They did a write to the left or right if an array, and checked that that a) crashed b) the crash message contained the text "left of" / "right of". Fairly easy to update, but also a bit churny, for subjectively not a lot of upside. (But if y'all think this is a better diag, that's enough of course.)

Thank you for updating the rest to make the codebase self-consistent again!

enh added a comment.Sep 9 2022, 11:50 AM

We had a bunch of gtest death tests to check that the sanitizers are working. They did a write to the left or right if an array, and checked that that a) crashed b) the crash message contained the text "left of" / "right of". Fairly easy to update, but also a bit churny, for subjectively not a lot of upside. (But if y'all think this is a better diag, that's enough of course.)

yeah, we won't know for a while, but i'm hopeful that (in my day job as "Android C/C++ crash dump bug concierge") i'll have to explain "self explanatory" (hw)asan bug reports less frequently...

it took a pretty sophisticated C programmer explicitly saying "i don't know what '0 bytes to the right' is telling me" for me to be brave enough to complain to the asan folks, even though i'd always had to translate these in my head myself. if _he_ didn't get it, the average NDK user never will.

the same person also noticed that in the shadow map we dump shadow addresses rather than "real" addresses, which i'd never noticed because i just ignore that bit as being "for experts only". one day i might be brave enough to admit that i don't know whether i'm supposed to be using it, and if so, for what :-)

fmayer marked an inline comment as done.Sep 9 2022, 11:52 AM

the same person also noticed that in the shadow map we dump shadow addresses rather than "real" addresses, which i'd never noticed because i just ignore that bit as being "for experts only". one day i might be brave enough to admit that i don't know whether i'm supposed to be using it, and if so, for what :-)

FYI this was addressed in D133391 and D133380.