[RISC-V][HWASAN] Don't explicitly load GOT entry to call hwasan mismatch routine

[smd]

Summary

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

Depends On D131343

[smd]

Update test

[luismarques]

Sorry, could you please explain why this required a manually written check? It's not obvious to me.

[smd]

In D132994#3885764, @luismarques wrote:

Sorry, could you please explain why this required a manually written check? It's not obvious to me.

I tried to figure out how to use update_llc_test_checks.py properly, but unfortunately I was not able to make it expand @llvm.hwasan.check.memaccess.shortgranules, so I had to add those checks manually.
If you have any suggestions how to fix this test in a proper way, they'll be more than welcome!

Btw this patch and previous one obviously should be merged and pushed together.'

Thanks!

[luismarques]

I looked at this more carefully. I think the CHECKs for the .section were already not being updated automatically (not sure if that was the case when they were originally added), so you didn't change that (when I originally skimmed the patch I thought that was new). I'm not sure the two UTC_ARGS are actually needed.

The changes to the test itself seem fine, please do merge with the previous patch, as you indicated.

[smd]

Addressing comments

[smd]

@luismarques @jrtc27 Do you think it's ok if I commit this patch?
Thanks

[luismarques]

As far as I can tell from looking at the relevant psABI discussions and GNU patches, this overall approach is OK.

Thinking about the specifics of the patch, what happens if you separately compile and assemble this? Does it lose the STO_RISCV_VARIANT_CC?

[smd]

In D132994#3911904, @luismarques wrote:

As far as I can tell from looking at the relevant psABI discussions and GNU patches, this overall approach is OK.

Thinking about the specifics of the patch, what happens if you separately compile and assemble this? Does it lose the STO_RISCV_VARIANT_CC?

Hi Luis,

Sorry for the delayed reply.
Regarding your question from what I can tell STO_RISCV_VARIANT_CC is present in the object file if it's compiled separately, the flag is present on the symbol:

clang -fsanitize=hwaddress -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions global.c -c -o /tmp/globals.o
...
llvm-readobj -a /tmp/globals.o
...
  Symbol {
    Name: __hwasan_tag_mismatch_v2 (650)
    Value: 0x0
    Size: 0
    Binding: Global (0x1)
    Type: None (0x0)
    Other [ (0x80)
      STO_RISCV_VARIANT_CC (0x80)
    ]
    Section: Undefined (0x0)
  }

And I expect it's also supported by the linkers by quickly looking into existing patches.

[jrtc27]

It seems we're missing a emitDirectiveVariantCC equivalent to AArch64's emitDirectiveVariantPCS, which is what should be used here I believe otherwise Luis is right, the assembly doesn't mark the symbol as a variant CC one, so round-tripping through assembly loses the flag.

What you showed is still compiling directly from C to an object file with the integrated assembler; you'd need to either use -no-integrated-as or use -S and then assemble the .s file.

[smd]

In D132994#3923439, @jrtc27 wrote:

It seems we're missing a emitDirectiveVariantCC equivalent to AArch64's emitDirectiveVariantPCS, which is what should be used here I believe otherwise Luis is right, the assembly doesn't mark the symbol as a variant CC one, so round-tripping through assembly loses the flag.

What you showed is still compiling directly from C to an object file with the integrated assembler; you'd need to either use -no-integrated-as or use -S and then assemble the .s file.

Indeed asm file lacks .variant_cc annotation.
I doubt supporting emitDirectiveVariantCC should be a part of this particular patch, so what's the best way to handle this situation?

[smd]

I think one of possible solutions for the problem mentioned above might be the following:

• mark __hwasan_tag_mismatch_v2 as .variant_cc directly in asm file
• add support for reading/dumping .variant_cc as emitDirectiveVariantPCS does
• remove setting ELF::STO_RISCV_VARIANT_CC for __hwasan_tag_mismatch_v2 in RISCVAsmPrinter.cpp

Supposedly (2) should be implemented/committed as a separate patch before this one. It looks like it's being done as part of https://reviews.llvm.org/D103435
I might have my own patch for .variant_cc support. Does it make sense to publish it for the review, since the D103435 has been sitting there for over a year?

@jrtc27 @luismarques what do you think of that?

[luismarques]

In D132994#3927328, @smd wrote:

I think one of possible solutions for the problem mentioned above might be the following:

• mark __hwasan_tag_mismatch_v2 as .variant_cc directly in asm file
• add support for reading/dumping .variant_cc as emitDirectiveVariantPCS does
• remove setting ELF::STO_RISCV_VARIANT_CC for __hwasan_tag_mismatch_v2 in RISCVAsmPrinter.cpp

Supposedly (2) should be implemented/committed as a separate patch before this one. It looks like it's being done as part of https://reviews.llvm.org/D103435
I might have my own patch for .variant_cc support. Does it make sense to publish it for the review, since the D103435 has been sitting there for over a year?

@jrtc27 @luismarques what do you think of that?

It looks like it D103435 has started picking up activity again. You can always publish your patch with a description that references D103435, and possibly point out the ways in which the patches are different. For instance, sometimes if it's just a subset of another patch that can help that subset be merged earlier, without being blocked on other issues. In any case, perhaps now D103435 will move faster.

I suggest rebasing this patch on an implementation of (2), either D103435 or your alternative, as that seems like the proper solution. If those patches were to continue being blocked for a while I don't have a problem with (1), but I imagine other people might disagree.

[smd]

In D132994#3943554, @luismarques wrote:

In D132994#3927328, @smd wrote:

I think one of possible solutions for the problem mentioned above might be the following:

• mark __hwasan_tag_mismatch_v2 as .variant_cc directly in asm file
• add support for reading/dumping .variant_cc as emitDirectiveVariantPCS does
• remove setting ELF::STO_RISCV_VARIANT_CC for __hwasan_tag_mismatch_v2 in RISCVAsmPrinter.cpp

Supposedly (2) should be implemented/committed as a separate patch before this one. It looks like it's being done as part of https://reviews.llvm.org/D103435
I might have my own patch for .variant_cc support. Does it make sense to publish it for the review, since the D103435 has been sitting there for over a year?

@jrtc27 @luismarques what do you think of that?

It looks like it D103435 has started picking up activity again. You can always publish your patch with a description that references D103435, and possibly point out the ways in which the patches are different. For instance, sometimes if it's just a subset of another patch that can help that subset be merged earlier, without being blocked on other issues. In any case, perhaps now D103435 will move faster.

I suggest rebasing this patch on an implementation of (2), either D103435 or your alternative, as that seems like the proper solution. If those patches were to continue being blocked for a while I don't have a problem with (1), but I imagine other people might disagree.

Hi Luis,

I think all 3 of those steps might be necessary: it's impossible to do (1) without having changes from (2).
And (3) could be done after having (1) done. I doubt anyone else would use __hwasan_tag_mismatch_v2 except for this particular case, so marking it with .variant_cc might not hurt.
It looks like D138352 might do the trick and bring .variant_cc support pretty soon.

Other possible solution I see is to push this patch as is and later fix the issues you've raised after .variant_cc patch is commited.

[smd]

Rebase and properly emit .variant_cc

[smd]

With D139414 being pushed to upstream, .variant_cc symbol is now properly emitted by hwasan code:

clang -fsanitize=hwaddress -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions src/llvm-project/compiler-rt/test/hwasan/TestCases/global.c -S -c -o /tmp/globals.s | grep variant_cc /tmp/globals.s
        .variant_cc     __hwasan_tag_mismatch_v2

[smd]

ping

[luismarques]

LGTM.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

275–276

Nit: missing period.

277–278

Nit. You can use auto for the variable since the type is obvious from the cast.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

 void RISCVAsmPrinter::EmitHwasanMemaccessSymbols(Module &M) {
   if (HwasanMemaccessSymbols.empty())
     return;
 
   assert(TM.getTargetTriple().isOSBinFormatELF());
 
   MCSymbol *HwasanTagMismatchV2Sym =
       OutContext.getOrCreateSymbol("__hwasan_tag_mismatch_v2");
+  // Annotate symbol as one having incompatible calling convention, so
+  // run-time linkers can instead eagerly bind this function.
+  auto &RTS =
+      static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer());
+  RTS.emitDirectiveVariantCC(*HwasanTagMismatchV2Sym);
 
   const MCSymbolRefExpr *HwasanTagMismatchV2Ref =
       MCSymbolRefExpr::create(HwasanTagMismatchV2Sym, OutContext);
+  auto Expr = RISCVMCExpr::create(HwasanTagMismatchV2Ref,
+                                  RISCVMCExpr::VK_RISCV_CALL, OutContext);
 
   for (auto &P : HwasanMemaccessSymbols) {
     unsigned Reg = std::get<0>(P.first);
     uint32_t AccessInfo = std::get<1>(P.first);
-    const MCSymbolRefExpr *HwasanTagMismatchRef = HwasanTagMismatchV2Ref;
     MCSymbol *Sym = P.second;
 
     unsigned Size =
         1 << ((AccessInfo >> HWASanAccessInfo::AccessSizeShift) & 0xf);
     OutStreamer->switchSection(OutContext.getELFSection(
         ".text.hot", ELF::SHT_PROGBITS,
         ELF::SHF_EXECINSTR | ELF::SHF_ALLOC | ELF::SHF_GROUP, 0, Sym->getName(),
         /*IsComdat=*/true));
if (Reg != RISCV::X10)
                                    *STI);
     OutStreamer->emitInstruction(
         MCInstBuilder(RISCV::ADDI)
             .addReg(RISCV::X11)
             .addReg(RISCV::X0)
             .addImm(AccessInfo & HWASanAccessInfo::RuntimeMask),
         *STI);
 
-    // Intentionally load the GOT entry and branch to it, rather than possibly
-    // late binding the function, which may clobber the registers before we have
-    // a chance to save them.
-    RISCVMCExpr::VariantKind VKHi;
-    unsigned SecondOpcode;
-    if (OutContext.getObjectFileInfo()->isPositionIndependent()) {
-      SecondOpcode = RISCV::LD;
-      VKHi = RISCVMCExpr::VK_RISCV_GOT_HI;
-    } else {
-      SecondOpcode = RISCV::ADDI;
-      VKHi = RISCVMCExpr::VK_RISCV_PCREL_HI;
-    }
-    auto ExprHi = RISCVMCExpr::create(HwasanTagMismatchRef, VKHi, OutContext);
-
-    MCSymbol *TmpLabel =
-        OutContext.createTempSymbol("pcrel_hi", /* AlwaysAddSuffix */ true);
-    OutStreamer->emitLabel(TmpLabel);
-    const MCExpr *ExprLo =
-        RISCVMCExpr::create(MCSymbolRefExpr::create(TmpLabel, OutContext),
-                            RISCVMCExpr::VK_RISCV_PCREL_LO, OutContext);
-
-    OutStreamer->emitInstruction(
-        MCInstBuilder(RISCV::AUIPC).addReg(RISCV::X6).addExpr(ExprHi), *STI);
-    OutStreamer->emitInstruction(MCInstBuilder(SecondOpcode)
-                                     .addReg(RISCV::X6)
-                                     .addReg(RISCV::X6)
-                                     .addExpr(ExprLo),
-                                 *STI);
-
-    OutStreamer->emitInstruction(MCInstBuilder(RISCV::JALR)
-                                     .addReg(RISCV::X0)
-                                     .addReg(RISCV::X6)
-                                     .addImm(0),
+    OutStreamer->emitInstruction(MCInstBuilder(RISCV::PseudoCALL).addExpr(Expr),
                                  *STI);
   }
 }

llvm/test/CodeGen/RISCV/hwasan-check-memaccess.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=riscv64 < %s | FileCheck --check-prefixes=CHECK,NOPIC %s
-; RUN: llc -mtriple=riscv64 --relocation-model=pic < %s | FileCheck --check-prefixes=CHECK,PIC %s
+; RUN: llc -mtriple=riscv64 < %s | FileCheck %s
+; RUN: llc -mtriple=riscv64 --relocation-model=pic < %s | FileCheck %s
 
 define ptr @f2(ptr %x0, ptr %x1) {
 ; CHECK-LABEL: f2:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    addi sp, sp, -16
 ; CHECK-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NEXT:    sd ra, 8(sp) # 8-byte Folded Spill
 ; CHECK-NEXT:    .cfi_offset ra, -8

 ; CHECK-NEXT: beq     t1, t2, .Ltmp1
 ; CHECK-NEXT: .Ltmp2:
 ; CHECK-NEXT: addi    sp, sp, -256
 ; CHECK-NEXT: sd      a0, 80(sp)
 ; CHECK-NEXT: sd      a1, 88(sp)
 ; CHECK-NEXT: sd      s0, 64(sp)
 ; CHECK-NEXT: sd      ra, 8(sp)
 ; CHECK-NEXT: li      a1, 2
-; CHECK-NEXT: .Lpcrel_hi0:
-; NOPIC-NEXT: auipc   t1, %pcrel_hi(__hwasan_tag_mismatch_v2)
-; NOPIC-NEXT: addi    t1, t1, %pcrel_lo(.Lpcrel_hi0)
-; PIC-NEXT: auipc   t1, %got_pcrel_hi(__hwasan_tag_mismatch_v2)
-; PIC-NEXT: ld      t1, %pcrel_lo(.Lpcrel_hi0)(t1)
-; CHECK-NEXT: jr      t1
-;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
-; NOPIC: {{.*}}
-; PIC: {{.*}}
+; CHECK-NEXT: call    __hwasan_tag_mismatch_v2