This is an archive of the discontinued LLVM Phabricator instance.

[MTE] Add AArch64GlobalsTagging Pass
ClosedPublic

Authored by hctim on Sep 6 2022, 6:05 PM.

Details

Summary

Adds an IR pass for -fsanitize=memtag-globals. This pass goes over the
tag-capable global variables, and replaces them with a tagged global
variable of the same contents. This new global variable will have its
size and alignment adjusted if neccesary so that they're both a multiple
of the tag granule size (16 bytes).

Global merge must also be suppressed for tagged globals, as each global
variable must have a unique tag. This can possibly be relaxed in future;
globals that are identical in size, alignment, and content can possibly
be merged. The major problem comes from tail- or head-merging, which if
left unchecked, could have partially-overlapping global variables with
different memory tags, leading to crashes at runtime.

Diff Detail

Event Timeline

hctim created this revision.Sep 6 2022, 6:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2022, 6:05 PM
hctim requested review of this revision.Sep 6 2022, 6:05 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 6 2022, 6:05 PM
fmayer added inline comments.Sep 6 2022, 6:19 PM
llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
61

not necessarily in this CL: do we want some attribute to turn off instrumentation?

99

can we use the SmallVector here?

109

what does this do?

hctim updated this revision to Diff 458472.Sep 7 2022, 9:02 AM
hctim marked 3 inline comments as done.

Update from Florian's comments.

llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
61

memtag sanitization is not-implicit (i.e. it's opt-in rather than opt-out): https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/GlobalValue.h#L327, so a turn-off attribute should be unnecessary

109

dead code, thanks, removed

tschuett added inline comments.
llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
94

If the 16 is the size of the granule, then it deserves to be named constant.

hctim updated this revision to Diff 458487.Sep 7 2022, 9:51 AM
hctim marked an inline comment as done.

Update inlined constants to use named constant.

fmayer added inline comments.Sep 7 2022, 11:04 AM
llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
26

why don't we put this into the llvm namespace rather than using?

tschuett added inline comments.Sep 7 2022, 11:10 AM
llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
30

I believe you are going too far with the anonymous namespace. There is no need for static functions in anonymous namespaces.

https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

hctim updated this revision to Diff 458536.Sep 7 2022, 12:41 PM
hctim marked 2 inline comments as done.

Some namespace moving and shaking.

llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
26

common for code in this folder, INITIALIZE_PASS_BEGIN (below) errors out not being in the llvm namespace, but also warns about redundantly using llvm:: in the macro expansion if you explicitly use the namespace...

30

moved everything but the class out of the namespace

94

thanks, done

fmayer accepted this revision.Sep 7 2022, 12:42 PM
This revision is now accepted and ready to land.Sep 7 2022, 12:42 PM

Change description says that the new pass "marks them as tagged". That's not what is happening.

llvm/lib/CodeGen/GlobalMerge.cpp
656

This comment is not convincing. The change description is right that some globals can be merged, copy that here, or extend the comment a little to explain why and under what assumptions.

llvm/lib/MC/ELFObjectWriter.cpp
1344 ↗(On Diff #458536)

Isn't a bigger reason that we need to know the address tag to put on the pointer, and that requires a symbol, not a section reference?

hctim updated this revision to Diff 479452.Dec 1 2022, 3:25 PM
hctim marked 2 inline comments as done.

Rebase now that the parent is submitted, clean up an old comment. New commit message also incoming.

llvm/lib/CodeGen/GlobalMerge.cpp
656

Thanks, extended the comment.

llvm/lib/MC/ELFObjectWriter.cpp
1344 ↗(On Diff #458536)

removed this comment here, as this was moved into the parent patch https://reviews.llvm.org/D131863 and the comment was updated there. this comment references the old st_other-based implementation, whereas now it's needed for SHT_AARCH64_MEMTAG_GLOBALS_STATIC.

hctim edited the summary of this revision. (Show Details)Dec 1 2022, 3:34 PM
piggynl added a subscriber: piggynl.Dec 6 2022, 6:17 AM
hctim updated this revision to Diff 486093.Jan 3 2023, 2:35 PM

Rebase.

eugenis accepted this revision.Jan 10 2023, 1:27 PM

LGTM

llvm/lib/CodeGen/GlobalMerge.cpp
655

typo: "merge"

llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
75

unused variable

hctim updated this revision to Diff 493642.Jan 31 2023, 9:04 AM
hctim marked 2 inline comments as done.

Rebase for submit.

This revision was landed with ongoing or failed builds.Jan 31 2023, 9:25 AM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Jan 31 2023, 12:03 PM

Hi @hctim, the test you added is failing on buildbots where AArch64 is not built, can you either update the test to not require that backend, or mark the test as requiring it?

https://lab.llvm.org/buildbot/#/builders/139/builds/35253

error: unable to create target: 'No available targets are compatible with triple "aarch64-unknown-linux-android31"'
1 error generated.

Hi @hctim, the test you added is failing on buildbots where AArch64 is not built, can you either update the test to not require that backend, or mark the test as requiring it?

https://lab.llvm.org/buildbot/#/builders/139/builds/35253

error: unable to create target: 'No available targets are compatible with triple "aarch64-unknown-linux-android31"'
1 error generated.

Thanks for the heads up. I've reverted and will test on my end before relanding.