[RISC-V][HWASAN] Support tagging global variables for RISC-V HWASAN

[smd]

Summary

[8/11] patch series to port HWASAN for riscv64

Depends On D132994

[kito-cheng]

llvm/test/CodeGen/RISCV/tagged-globals.ll

9

Maybe add one more testcase to show @global = internal global i32 0 will always access via GOT with +tagged-globals even --relocation-model=pic?

[smd]

Addressing comments

[smd]

llvm/test/CodeGen/RISCV/tagged-globals.ll

9

Done, thanks

[smd]

@jrtc27 gentle ping

[vitalybuka]

LGTM, but I guess it's more for RISCV folks to review

[smd]

Hi @jrtc27, @luismarques, @MaskRay, @craig.topper, @asb,

Could you please help with reviewing this patch.
Thanks

[smd]

@jrtc27, @asb, @luismarques, @MaskRay, @craig.topper
Gengle ping, could you please help with reviewing this patch?
Thanks

[luismarques]

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.

llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp

458–460

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.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

4085–4090

Ditto.

llvm/test/CodeGen/RISCV/tagged-globals.ll

9–10

Nit: globalint makes it sound it's an integer, can you tweak that somehow?

9–10

[smd]

Does the tagging actually change the location of globals?

No, it doesn't.

I was assuming the location was the same, and the tag was just changing how we encoded that location in a numerical value.

Yes, you're right.

My understanding of the code model specifications is that they only care about the actual address ranges.
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?

I guess that you're right. However from what I can tell, this would require to make linker to distinguish different kind of addresses of variables. I guess that's why ARM has a separate symbol for such tags.

That only impacts the comments in the patch, I'm not opposed to the approach of using the GOT.

I'll fix the comments as you suggested, thanks.

[smd]

Addressing comments

llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp

458–460

fixed, thanks

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

4085–4090

fixed, thanks

llvm/test/CodeGen/RISCV/tagged-globals.ll

9–10

fixed, thanks

[jrtc27]

llvm/test/CodeGen/RISCV/tagged-globals.ll

5–7

These shouldn't be here

[smd]

Addressing comments

[smd]

llvm/test/CodeGen/RISCV/tagged-globals.ll

5–7

fixed, thanks

[smd]

@jrtc27
Hi, could you please check that your request has been addressed?
Thanks

[smd]

In D132995#3923440, @smd wrote:

@jrtc27
Hi, could you please check that your request has been addressed?
Thanks

@jrtc27 ping

[smd]

Hi @jrtc27,
If you don't have any objections, I'd like to push this patch this weekend.
Thanks

llvm/lib/Target/RISCV/RISCV.td

 // built with this feature is not ABI-compatible with code built without this
 // feature, if atomic variables are exposed across the ABI boundary.
 def FeatureForcedAtomics : SubtargetFeature<
     "forced-atomics", "HasForcedAtomics", "true",
     "Assume that lock-free native-width atomics are available">;
 def HasAtomicLdSt
     : Predicate<"Subtarget->hasStdExtA() || Subtarget->hasForcedAtomics()">;
 
+def FeatureTaggedGlobals : SubtargetFeature<"tagged-globals",
+    "AllowTaggedGlobals",
+    "true", "Use an instruction sequence for taking the address of a global "
+    "that allows a memory tag in the upper address bits">;
+
 //===----------------------------------------------------------------------===//
 // Named operands for CSR instructions.
 //===----------------------------------------------------------------------===//
 
 include "RISCVSystemOperands.td"
 
 //===----------------------------------------------------------------------===//
 // Registers, calling conventions, instruction descriptions.

llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp

return expandAuipcInstPair(MBB, MBBI, NextMBBI, RISCVII::MO_PCREL_HI,
                              RISCV::ADDI);
 }
 
 bool RISCVPreRAExpandPseudo::expandLoadAddress(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
     MachineBasicBlock::iterator &NextMBBI) {
   MachineFunction *MF = MBB.getParent();
 
-  assert(MF->getTarget().isPositionIndependent());
   const auto &STI = MF->getSubtarget<RISCVSubtarget>();
+  // 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.
+  assert(MF->getTarget().isPositionIndependent() || STI.allowTaggedGlobals());
   unsigned SecondOpcode = STI.is64Bit() ? RISCV::LD : RISCV::LW;
   return expandAuipcInstPair(MBB, MBBI, NextMBBI, RISCVII::MO_GOT_HI,
                              SecondOpcode);
 }
 
 bool RISCVPreRAExpandPseudo::expandLoadTLSIEAddress(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
     MachineBasicBlock::iterator &NextMBBI) {

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

 }
 
 template <class NodeTy>
 SDValue RISCVTargetLowering::getAddr(NodeTy *N, SelectionDAG &DAG,
                                      bool IsLocal) const {
   SDLoc DL(N);
   EVT Ty = getPointerTy(DAG.getDataLayout());
 
-  if (isPositionIndependent()) {
+  // 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.
+  if (isPositionIndependent() || Subtarget.allowTaggedGlobals()) {
     SDValue Addr = getTargetNode(N, DL, Ty, DAG, 0);
-    if (IsLocal)
+    if (IsLocal && !Subtarget.allowTaggedGlobals())
       // Use PC-relative addressing to access the symbol. This generates the
       // pattern (PseudoLLA sym), which expands to (addi (auipc %pcrel_hi(sym))
       // %pcrel_lo(auipc)).
       return DAG.getNode(RISCVISD::LLA, DL, Ty, Addr);
 
     // Use PC-relative addressing to access the GOT for this symbol, then load
     // the address from the GOT. This generates the pattern (PseudoLA sym),
     // which expands to (ld (addi (auipc %got_pcrel_hi(sym)) %pcrel_lo(auipc))).

llvm/lib/Target/RISCV/RISCVSubtarget.h

private:
   bool EnableLinkerRelax = false;
   bool EnableRVCHintInstrs = true;
   bool EnableDefaultUnroll = true;
   bool EnableSaveRestore = false;
   bool EnableUnalignedScalarMem = false;
   bool HasShortForwardBranchOpt = false;
   bool HasLUIADDIFusion = false;
   bool HasForcedAtomics = false;
+  bool AllowTaggedGlobals = false;
   unsigned XLen = 32;
   unsigned ZvlLen = 0;
   MVT XLenVT = MVT::i32;
   uint8_t MaxInterleaveFactor = 2;
   RISCVABI::ABI TargetABI = RISCVABI::ABI_Unknown;
   std::bitset<RISCV::NUM_TARGET_REGS> UserReservedRegister;
   RISCVFrameLowering FrameLowering;
   RISCVInstrInfo InstrInfo;
public:
   bool enableLinkerRelax() const { return EnableLinkerRelax; }
   bool enableRVCHintInstrs() const { return EnableRVCHintInstrs; }
   bool enableDefaultUnroll() const { return EnableDefaultUnroll; }
   bool enableSaveRestore() const { return EnableSaveRestore; }
   bool hasShortForwardBranchOpt() const { return HasShortForwardBranchOpt; }
   bool enableUnalignedScalarMem() const { return EnableUnalignedScalarMem; }
   bool hasLUIADDIFusion() const { return HasLUIADDIFusion; }
   bool hasForcedAtomics() const { return HasForcedAtomics; }
+  bool allowTaggedGlobals() const { return AllowTaggedGlobals; }
   MVT getXLenVT() const { return XLenVT; }
   unsigned getXLen() const { return XLen; }
   unsigned getFLen() const {
     if (HasStdExtD)
       return 64;
 
     if (HasStdExtF)
       return 32;

llvm/test/CodeGen/RISCV/tagged-globals.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --no-generate-body-for-unused-prefixes
+; RUN: llc --relocation-model=pic < %s | FileCheck %s
+; RUN: llc --relocation-model=static < %s | FileCheck %s
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "riscv64-unknown-linux"
+
+@global_ext = external global i32
+@global_int = internal global i32 0
+declare void @func()
+
+define i32* @global_addr() #0 {
+; CHECK-LABEL: global_addr:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:  .Lpcrel_hi0:
+; CHECK-NEXT:    auipc a0, %got_pcrel_hi(global_ext)
+; CHECK-NEXT:    ld a0, %pcrel_lo(.Lpcrel_hi0)(a0)
+; CHECK-NEXT:    ret
+  ret i32* @global_ext
+}
+
+define i32 @global_load() #0 {
+; CHECK-LABEL: global_load:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:  .Lpcrel_hi1:
+; CHECK-NEXT:    auipc a0, %got_pcrel_hi(global_ext)
+; CHECK-NEXT:    ld a0, %pcrel_lo(.Lpcrel_hi1)(a0)
+; CHECK-NEXT:    lw a0, 0(a0)
+; CHECK-NEXT:    ret
+  %load = load i32, i32* @global_ext
+  ret i32 %load
+}
+
+define void @global_store() #0 {
+; CHECK-LABEL: global_store:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:  .Lpcrel_hi2:
+; CHECK-NEXT:    auipc a0, %got_pcrel_hi(global_ext)
+; CHECK-NEXT:    ld a0, %pcrel_lo(.Lpcrel_hi2)(a0)
+; CHECK-NEXT:    sw zero, 0(a0)
+; CHECK-NEXT:    ret
+  store i32 0, i32* @global_ext
+  ret void
+}
+
+define i32* @global_int_addr() #0 {
+; CHECK-LABEL: global_int_addr:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:  .Lpcrel_hi3:
+; CHECK-NEXT:    auipc a0, %got_pcrel_hi(global_int)
+; CHECK-NEXT:    ld a0, %pcrel_lo(.Lpcrel_hi3)(a0)
+; CHECK-NEXT:    ret
+  ret i32* @global_int
+}
+
+define i32 @global_int_load() #0 {
+; CHECK-LABEL: global_int_load:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:  .Lpcrel_hi4:
+; CHECK-NEXT:    auipc a0, %got_pcrel_hi(global_int)
+; CHECK-NEXT:    ld a0, %pcrel_lo(.Lpcrel_hi4)(a0)
+; CHECK-NEXT:    lw a0, 0(a0)
+; CHECK-NEXT:    ret
+  %load = load i32, i32* @global_int
+  ret i32 %load
+}
+
+define void @global_int_store() #0 {
+; CHECK-LABEL: global_int_store:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:  .Lpcrel_hi5:
+; CHECK-NEXT:    auipc a0, %got_pcrel_hi(global_int)
+; CHECK-NEXT:    ld a0, %pcrel_lo(.Lpcrel_hi5)(a0)
+; CHECK-NEXT:    sw zero, 0(a0)
+; CHECK-NEXT:    ret
+  store i32 0, i32* @global_int
+  ret void
+}
+
+define void ()* @func_addr() #0 {
+; CHECK-LABEL: func_addr:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:  .Lpcrel_hi6:
+; CHECK-NEXT:    auipc a0, %got_pcrel_hi(func)
+; CHECK-NEXT:    ld a0, %pcrel_lo(.Lpcrel_hi6)(a0)
+; CHECK-NEXT:    ret
+  ret void ()* @func
+}
+
+attributes #0 = { "target-features"="+tagged-globals" }