[PATCH] D132995: [RISC-V][HWASAN] Support tagging global variables for RISC-V HWASAN

[Luís Marques via Phabricator via llvm-commits]

luismarques added a comment.

Is the tagging actually incompatible with the code model or only with the address materialization instructions that we use as part of the implementation of the code model? Does the tagging actually change the location of globals? I was assuming the location was the same, and the tag was just changing how we encoded that location in a numerical value. My understanding of the code model specifications is that they only care about the actual address ranges. Although the RISC-V psABI gives examples of instruction sequences my interpretation of that is that it's non-normative. So if the location doesn't actually change in principle you could materialize those with a different instruction sequence and still claim you comply with the code model, is that not the case? That only impacts the comments in the patch, I'm not opposed to the approach of using the GOT.

The overall patch LGTM.



================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:400-402
+  // In case of HWASAN being used ang tagging global variables enabled
+  // they should be accessed via GOT, since tagged address of global
+  // could not fit existing code models. This also applies for no-pic mode.
----------------
Nit: (spellchecking only)
```
// When HWASAN is used and tagging of global variables is enabled
// they should be accessed via the GOT, since the tagged address of a global
// is incompatible with existing code models. This also applies to non-pic mode.
```

See my broader patch review comment about the semantics.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:3859-3861
+  // In case of HWASAN being used ang tagging global variables enabled
+  // they should be accessed via GOT, since tagged address of global
+  // could not fit existing code models. This also applies for no-pic mode.
----------------
Ditto.


================
Comment at: llvm/test/CodeGen/RISCV/tagged-globals.ll:8-9
+
+ at global = external global i32
+ at globalint = internal global i32 0
+declare void @func()
----------------
Nit: `globalint` makes it sound it's an `integer`, can you tweak that somehow?


================
Comment at: llvm/test/CodeGen/RISCV/tagged-globals.ll:8-9
+
+ at global = external global i32
+ at globalint = internal global i32 0
+declare void @func()
----------------
luismarques wrote:
> Nit: `globalint` makes it sound it's an `integer`, can you tweak that somehow?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132995