This is an archive of the discontinued LLVM Phabricator instance.

[HWASan] Update use-after-scope test to use ARM64.
ClosedPublic

Authored by fmayer on Mar 10 2022, 10:57 AM.

Diff Detail

Event Timeline

fmayer created this revision.Mar 10 2022, 10:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 10:57 AM
fmayer requested review of this revision.Mar 10 2022, 10:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 10:57 AM
hctim added a comment.Mar 10 2022, 3:36 PM

This test is really hard to validate changes to. I mean, it's basically "$COMPILER produces $BLOB", right? I'm not really sure what are the interesting parts I'm supposed to be looking for.

Is it possible to reduce the test to check the actual conditions?

Like, for standard_lifetime, we have lifetime start -> shadow setup (some minimal UID like __hwasan_generate_tag) -> lifetime end -> shadow teardown (some minimal UID like memset-zero).

This test is really hard to validate changes to. I mean, it's basically "$COMPILER produces $BLOB", right? I'm not really sure what are the interesting parts I'm supposed to be looking for.

Is it possible to reduce the test to check the actual conditions?

Like, for standard_lifetime, we have lifetime start -> shadow setup (some minimal UID like __hwasan_generate_tag) -> lifetime end -> shadow teardown (some minimal UID like memset-zero).

This is a auto-generated golden test: ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py

I think there is a value in that, to make sure any changes don't exceed the intended scope. There are more focused tests as well in this directory.

hctim added a comment.Mar 10 2022, 6:42 PM

This is a auto-generated golden test: ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py

I think there is a value in that, to make sure any changes don't exceed the intended scope. There are more focused tests as well in this directory.

Maybe I'm just biased against golden-file tests like this because they're almost impossible to review (apart from "I trust the author"), and they increase churn significantly when one wants to make instrumentation changes. But, I do agree there's some value in the "didn't mess things up" check.

If that's all we're aiming for, would it make sense to have use-after-scope-arm64.ll and use-after-scope-x64.ll?

This is a auto-generated golden test: ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py

I think there is a value in that, to make sure any changes don't exceed the intended scope. There are more focused tests as well in this directory.

Maybe I'm just biased against golden-file tests like this because they're almost impossible to review (apart from "I trust the author"), and they increase churn significantly when one wants to make instrumentation changes. But, I do agree there's some value in the "didn't mess things up" check.

If that's all we're aiming for, would it make sense to have use-after-scope-arm64.ll and use-after-scope-x64.ll?

Or we just put x86 and aarch64 into the same file, by using -target. That will make the file very large (2x the expectation comments), but makes sure they don't diverge. I would prefer that. WDYT?

hctim added a comment.Mar 15 2022, 1:48 PM

Or we just put x86 and aarch64 into the same file, by using -target. That will make the file very large (2x the expectation comments), but makes sure they don't diverge. I would prefer that. WDYT?

Sure.

fmayer updated this revision to Diff 415579.Mar 15 2022, 2:14 PM

keep x86

fmayer updated this revision to Diff 415580.Mar 15 2022, 2:15 PM

commit message update

hctim accepted this revision.Mar 16 2022, 10:28 AM
This revision is now accepted and ready to land.Mar 16 2022, 10:28 AM
This revision was landed with ongoing or failed builds.Mar 16 2022, 10:31 AM
This revision was automatically updated to reflect the committed changes.