[PATCH] D128950: Add 'sanitize_memtag' Global IR attribute

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 2 inline comments as done.
hctim added inline comments.


================
Comment at: llvm/include/llvm/IR/GlobalValue.h:321
+    // optimisations, whereas ASan/HWASan simply rewrites the global variable
+    // and accesses, and doesn't need to interact with other parts of LLVM.
+    //
----------------
eugenis wrote:
> This paragraph explains why we need the include bit, but not the exclude bit.
> Why do we need both?
> IR backward compatibility?
`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.


================
Comment at: llvm/test/Assembler/globalvariable-attributes.ll:13
+ at g10 = global i32 2, sanitize_memtag, align 4
+ at g11 = global i32 2, no_sanitize_memtag, sanitize_memtag, align 4
+ at g12 = global i32 2, no_sanitize_address, no_sanitize_hwaddress, sanitize_memtag, align 4
----------------
eugenis wrote:
> can we ban both attrs on the same global?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128950/new/

https://reviews.llvm.org/D128950