Page MenuHomePhabricator

Remove 'no_sanitize_memtag'. Add 'sanitize_memtag'.
ClosedPublic

Authored by hctim on Jun 30 2022, 3:00 PM.

Details

Summary

For MTE globals, we should have clang emit the attribute for all GV's
that it creates, and then use that in the upcoming AArch64 global
tagging IR pass. We need a positive attribute for this sanitizer (rather
than implicit sanitization of all globals) because it needs to interact
with other parts of LLVM, including:

  1. Suppressing certain global optimisations (like merging),
  2. Emitting extra directives by the ASM writer, and
  3. Putting extra information in the symbol table entries.

While this does technically make the LLVM IR / bitcode format
non-backwards-compatible, nobody should have used this attribute yet,
because it's a no-op.

Diff Detail

Event Timeline

hctim created this revision.Jun 30 2022, 3:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 3:00 PM
hctim requested review of this revision.Jun 30 2022, 3:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 3:00 PM
hctim added a comment.Jul 7 2022, 1:32 PM

(friendly ping @eugenis)

eugenis added inline comments.Jul 7 2022, 3:06 PM
llvm/include/llvm/IR/GlobalValue.h
322

This paragraph explains why we need the include bit, but not the exclude bit.
Why do we need both?
IR backward compatibility?

llvm/test/Assembler/globalvariable-attributes.ll
11

can we ban both attrs on the same global?

hctim marked 2 inline comments as done.Jul 8 2022, 12:57 PM
hctim added inline comments.
llvm/include/llvm/IR/GlobalValue.h
322

no_sanitize_memtag is added by the clang frontend and is used to ignore instrumentation in the upcoming IR pass.

sanitize_memtag is to be added by the IR pass, and is used to make the asm writers / elf object writers add add necessary metadata.

Updated the comments to be a little more clear about this.

llvm/test/Assembler/globalvariable-attributes.ll
11

See Vitaly's comment at https://reviews.llvm.org/D126100?id=434618#inline-1218941

Given that no_sanitize_memtag always overrides (see GlobalValue::isTagged(), which should always be used to check for sanitization), I think it's better to keep the IR reader/writer dumb to the actual semantics.

hctim updated this revision to Diff 443332.Jul 8 2022, 12:57 PM
hctim marked 2 inline comments as done.

Small update to comment.

hctim updated this revision to Diff 443390.Jul 8 2022, 4:39 PM

After talking with Evgenii offline, and testing, clearly we don't need an exclude mask. Update the patch to remove the exclude mask, and replace it with an include mask.

Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 4:39 PM
hctim edited the summary of this revision. (Show Details)Jul 8 2022, 4:40 PM
hctim retitled this revision from Add 'sanitize_memtag' Global IR attribute to Remove 'no_sanitize_memtag'. Add 'sanitize_memtag'..Jul 8 2022, 4:40 PM
eugenis accepted this revision.Tue, Jul 12, 4:29 PM

LGTM

llvm/include/llvm/IR/GlobalValue.h
349

return hasSanitizerMetadata() && getSanitizerMetadata().Memtag

This revision is now accepted and ready to land.Tue, Jul 12, 4:29 PM
hctim updated this revision to Diff 444111.Tue, Jul 12, 4:51 PM
hctim marked an inline comment as done.

Final comments.

This revision was landed with ongoing or failed builds.Wed, Jul 13, 8:55 AM
This revision was automatically updated to reflect the committed changes.