[RISC-V][HWASAN] Add support for lowering HWASAN intrinsic for RISC-V

[smd]

Summary

[6/12] patch series to port HWASAN for riscv64

Depends On D131574

[craig.topper]

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

264

Why do we need to create STI when there's already an STI as a member of this class?

[smd]

Addressing comments

[smd]

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

264

Fixed, thanks

[vitalybuka]

Please split RISCVAsmParser/Printer and HWAddressSanitizer into separate patches

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

17

If we change RISCVAsmPrinter, I assume we need to update RISCVAsmParser.cpp and add corresponding tests

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

919 ↗

(On Diff #450706)

we need a few new tests for instumentation

[smd]

Addressing comments

[smd]

In D131343#3711357, @vitalybuka wrote:

Please split RISCVAsmParser/Printer and HWAddressSanitizer into separate patches

Fixed, thanks.

[vitalybuka]

RISCVAsmPrinter needs a test for changes in this patch.

[smd]

In D131343#3719618, @vitalybuka wrote:

RISCVAsmPrinter needs a test for changes in this patch.

The requested tests have been added as part of D131344.
Thanks.

[vitalybuka]

In D131343#3722882, @smd wrote:

In D131343#3719618, @vitalybuka wrote:

RISCVAsmPrinter needs a test for changes in this patch.

The requested tests have been added as part of D131344.
Thanks.

llvm/test/CodeGen/RISCV/hwasan-check-memaccess.ll ?
the test essentially validates LowerHWASAN_CHECK_MEMACCESS, and does not relies on the rest of D131344
So the test needs to be moved from D131344 into this patch.

With moved test LGTM in general.
But I have no experience with RISCV, so I didn't look into details of emitted code.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

120

This "if" looks to random here?
File structure is very different from AArch64 version, so not sure if this is the best place for the code.

[smd]

@vitalybuka

llvm/test/CodeGen/RISCV/hwasan-check-memaccess.ll ?
the test essentially validates LowerHWASAN_CHECK_MEMACCESS, and does not relies on the rest of D131344
So the test needs to be moved from D131344 into this patch.

I believe the same applies to D131575 and basic.ll, where changes to instrumentMemAccessInline are tested?
If so, would you mind me moving hwasan-check-memaccess.ll to this patch and the rest of .ll tests to D131575?
Thanks

[vitalybuka]

In D131343#3726862, @smd wrote:

@vitalybuka

llvm/test/CodeGen/RISCV/hwasan-check-memaccess.ll ?
the test essentially validates LowerHWASAN_CHECK_MEMACCESS, and does not relies on the rest of D131344
So the test needs to be moved from D131344 into this patch.

I believe the same applies to D131575 and basic.ll, where changes to instrumentMemAccessInline are tested?
If so, would you mind me moving hwasan-check-memaccess.ll to this patch and the rest of .ll tests to D131575?
Thanks

In D131343#3726862, @smd wrote:

@vitalybuka

llvm/test/CodeGen/RISCV/hwasan-check-memaccess.ll ?
the test essentially validates LowerHWASAN_CHECK_MEMACCESS, and does not relies on the rest of D131344
So the test needs to be moved from D131344 into this patch.

I believe the same applies to D131575 and basic.ll, where changes to instrumentMemAccessInline are tested?
If so, would you mind me moving hwasan-check-memaccess.ll to this patch and the rest of .ll tests to D131575?
Thanks

Sure. The logic is simple:

1. Split functional changes into smaller patches: AsmPrint, Instrumentation, RT ...
2. Each patch contains tests corresponding to changed functionality

So
test/CodeGen/RISCV/ goes into D131343 (CodeGen patch)
test/Instrumentation/HWAddressSanitizer/RISCV/ into D131575 (Instrumentation patch)

D131344 should keep only compiler-rt/test/hwasan/TestCases/ as they look irrelevant to CodeGen and Instrumentation

[smd]

Addressing comments

[jrtc27]

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

120

lowerRISCVMachineInstrToMCInst seems like the right place for it. That's where PATCHABLE_FUNCTION_ENTER gets handled. Given that this node is a copy of AArch64's and every architecture would presumably use the same thing I do wonder if it should be under TargetOpcode, too...

451

This is what STO_RISCV_VARIANT_CC is for

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

1 ↗

(On Diff #453089)

Use update_llc_test_checks.py. Also test PIC too if you have PIC-specific code.

3 ↗

(On Diff #453089)

Redundant, you have -mtriple in the command line

[smd]

Addressing comments

[smd]

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

451

If you don't mind, I'd like address that in a separate patch, as I have to go through the discussion on this matter. Thanks

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

1 ↗

(On Diff #453089)

Fixed, thanks

3 ↗

(On Diff #453089)

Fixed, thanks

[jrtc27]

This was committed without addressing all the feedback you were provided

[jrtc27]

And if you’re committing a significant chunk of code to a backend I don’t think you should be doing so without getting approval from someone who regularly works on the backend

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

23 ↗

(On Diff #456205)

Surely you need UTC_ARGS
: —disable/—enable around this so it persists across reruns of update_llc_test_checks.py?

58 ↗

(On Diff #456205)

I do not understand what this comment is saying nor what the following lines are for

[smd]

@jrtc27 sorry for the mess

In D131343#3754228, @jrtc27 wrote:

This was committed without addressing all the feedback you were provided

What's the best way to fix it: would you like me to revert the commit or address the issues in next patches?

In D131343#3754229, @jrtc27 wrote:

And if you’re committing a significant chunk of code to a backend I don’t think you should be doing so without getting approval from someone who regularly works on the backend

Would you mind me adding you as a reviewer for further patches?

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

58 ↗

(On Diff #456205)

tbh, me neither, but this comment was added by update_llc_test_checks.py

Surely you need UTC_ARGS

Let me try this and see if it changes anything

[smd]

@jrtc27 I've created several reviews(D13299{3,4,5}) to address the issues that you mentioned. Could you please take a look? Thanks

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

 //===-- RISCVAsmPrinter.cpp - RISCV LLVM assembly writer ------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains a printer that converts from our internal representation
 // of machine-dependent LLVM code to the RISCV assembly language.
 //
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/RISCVInstPrinter.h"
 #include "MCTargetDesc/RISCVMCExpr.h"
 #include "MCTargetDesc/RISCVTargetStreamer.h"
 #include "RISCV.h"
 #include "RISCVTargetMachine.h"
 #include "TargetInfo/RISCVTargetInfo.h"
 #include "llvm/ADT/Statistic.h"
+#include "llvm/BinaryFormat/ELF.h"
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "llvm/CodeGen/MachineConstantPool.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCInstBuilder.h"
+#include "llvm/MC/MCObjectFileInfo.h"
+#include "llvm/MC/MCSectionELF.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Transforms/Instrumentation/HWAddressSanitizer.h"
+
 using namespace llvm;
 
 #define DEBUG_TYPE "asm-printer"
 
 STATISTIC(RISCVNumInstrsCompressed,
           "Number of RISC-V Compressed instructions emitted");
 
 namespace {
bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                        const char *ExtraCode, raw_ostream &OS) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
                              const char *ExtraCode, raw_ostream &OS) override;
 
   void EmitToStreamer(MCStreamer &S, const MCInst &Inst);
   bool emitPseudoExpansionLowering(MCStreamer &OutStreamer,
                                    const MachineInstr *MI);
 
+  typedef std::tuple<unsigned, uint32_t> HwasanMemaccessTuple;
+  std::map<HwasanMemaccessTuple, MCSymbol *> HwasanMemaccessSymbols;
+  void LowerHWASAN_CHECK_MEMACCESS(const MachineInstr &MI);
+  void EmitHwasanMemaccessSymbols(Module &M);
+
   // Wrapper needed for tblgenned pseudo lowering.
   bool lowerOperand(const MachineOperand &MO, MCOperand &MCOp) const {
     return lowerRISCVMachineOperandToMCOperand(MO, MCOp, *this);
   }
 
   void emitStartOfAsmFile(Module &M) override;
   void emitEndOfAsmFile(Module &M) override;
 
void RISCVAsmPrinter::emitInstruction(const MachineInstr *MI) {
   RISCV_MC::verifyInstructionPredicates(MI->getOpcode(),
                                         getSubtargetInfo().getFeatureBits());
 
   // Do any auto-generated pseudo lowerings.
   if (emitPseudoExpansionLowering(*OutStreamer, MI))
     return;
 
   MCInst TmpInst;
+
+  if (MI->getOpcode() == RISCV::HWASAN_CHECK_MEMACCESS_SHORTGRANULES) {
+    LowerHWASAN_CHECK_MEMACCESS(*MI);
+    return;
+  }
+
   if (!lowerRISCVMachineInstrToMCInst(MI, TmpInst, *this))
     EmitToStreamer(*OutStreamer, TmpInst);
 }
 
 bool RISCVAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                                       const char *ExtraCode, raw_ostream &OS) {
   // First try the generic code, which knows about modifiers like 'c' and 'n'.
   if (!AsmPrinter::PrintAsmOperand(MI, OpNo, ExtraCode, OS))
     return false;

 }
 
 void RISCVAsmPrinter::emitEndOfAsmFile(Module &M) {
   RISCVTargetStreamer &RTS =
       static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer());
 
   if (TM.getTargetTriple().isOSBinFormatELF())
     RTS.finishAttributeSection();
+  EmitHwasanMemaccessSymbols(M);
 }
 
 void RISCVAsmPrinter::emitAttributes() {
   RISCVTargetStreamer &RTS =
       static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer());
   RTS.emitTargetAttributes(*MCSTI);
 }
 
 void RISCVAsmPrinter::emitFunctionEntryLabel() {
   AsmPrinter::emitFunctionEntryLabel();
   RISCVTargetStreamer &RTS =
       static_cast<RISCVTargetStreamer &>(*OutStreamer->getTargetStreamer());
   RTS.setTargetABI(STI->getTargetABI());
 }
 
 // Force static initialization.
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVAsmPrinter() {
   RegisterAsmPrinter<RISCVAsmPrinter> X(getTheRISCV32Target());
   RegisterAsmPrinter<RISCVAsmPrinter> Y(getTheRISCV64Target());
 }
+
+void RISCVAsmPrinter::LowerHWASAN_CHECK_MEMACCESS(const MachineInstr &MI) {
+  Register Reg = MI.getOperand(0).getReg();
+  uint32_t AccessInfo = MI.getOperand(1).getImm();
+  MCSymbol *&Sym =
+      HwasanMemaccessSymbols[HwasanMemaccessTuple(Reg, AccessInfo)];
+  if (!Sym) {
+    // FIXME: Make this work on non-ELF.
+    if (!TM.getTargetTriple().isOSBinFormatELF())
+      report_fatal_error("llvm.hwasan.check.memaccess only supported on ELF");
+
+    std::string SymName = "__hwasan_check_x" + utostr(Reg - RISCV::X0) + "_" +
+                          utostr(AccessInfo) + "_short";
+    Sym = OutContext.getOrCreateSymbol(SymName);
+  }
+  auto Res = MCSymbolRefExpr::create(Sym, MCSymbolRefExpr::VK_None, OutContext);
+  auto Expr = RISCVMCExpr::create(Res, RISCVMCExpr::VK_RISCV_CALL, OutContext);
+
+  EmitToStreamer(*OutStreamer, MCInstBuilder(RISCV::PseudoCALL).addExpr(Expr));
+}
+
+void RISCVAsmPrinter::EmitHwasanMemaccessSymbols(Module &M) {
+  if (HwasanMemaccessSymbols.empty())
+    return;
+
+  const Triple &TT = TM.getTargetTriple();
+  assert(TT.isOSBinFormatELF());
+
+  MCSymbol *HwasanTagMismatchV2Sym =
+      OutContext.getOrCreateSymbol("__hwasan_tag_mismatch_v2");
+
+  const MCSymbolRefExpr *HwasanTagMismatchV2Ref =
+      MCSymbolRefExpr::create(HwasanTagMismatchV2Sym, 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));
+
+    OutStreamer->emitSymbolAttribute(Sym, MCSA_ELF_TypeFunction);
+    OutStreamer->emitSymbolAttribute(Sym, MCSA_Weak);
+    OutStreamer->emitSymbolAttribute(Sym, MCSA_Hidden);
+    OutStreamer->emitLabel(Sym);
+
+    // Extract shadow offset from ptr
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::SLLI).addReg(RISCV::X6).addReg(Reg).addImm(8),
+        *STI);
+    OutStreamer->emitInstruction(MCInstBuilder(RISCV::SRLI)
+                                     .addReg(RISCV::X6)
+                                     .addReg(RISCV::X6)
+                                     .addImm(12),
+                                 *STI);
+    // load shadow tag in X6, X5 contains shadow base
+    OutStreamer->emitInstruction(MCInstBuilder(RISCV::ADD)
+                                     .addReg(RISCV::X6)
+                                     .addReg(RISCV::X5)
+                                     .addReg(RISCV::X6),
+                                 *STI);
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::LBU).addReg(RISCV::X6).addReg(RISCV::X6).addImm(0),
+        *STI);
+    // Extract tag from X5 and compare it with loaded tag from shadow
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::SRLI).addReg(RISCV::X7).addReg(Reg).addImm(56),
+        *STI);
+    MCSymbol *HandleMismatchOrPartialSym = OutContext.createTempSymbol();
+    // X7 contains tag from memory, while X6 contains tag from the pointer
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::BNE)
+            .addReg(RISCV::X7)
+            .addReg(RISCV::X6)
+            .addExpr(MCSymbolRefExpr::create(HandleMismatchOrPartialSym,
+                                             OutContext)),
+        *STI);
+    MCSymbol *ReturnSym = OutContext.createTempSymbol();
+    OutStreamer->emitLabel(ReturnSym);
+    OutStreamer->emitInstruction(MCInstBuilder(RISCV::JALR)
+                                     .addReg(RISCV::X0)
+                                     .addReg(RISCV::X1)
+                                     .addImm(0),
+                                 *STI);
+    OutStreamer->emitLabel(HandleMismatchOrPartialSym);
+
+    OutStreamer->emitInstruction(MCInstBuilder(RISCV::ADDI)
+                                     .addReg(RISCV::X28)
+                                     .addReg(RISCV::X0)
+                                     .addImm(16),
+                                 *STI);
+    MCSymbol *HandleMismatchSym = OutContext.createTempSymbol();
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::BGEU)
+            .addReg(RISCV::X6)
+            .addReg(RISCV::X28)
+            .addExpr(MCSymbolRefExpr::create(HandleMismatchSym, OutContext)),
+        *STI);
+
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::ANDI).addReg(RISCV::X28).addReg(Reg).addImm(0xF),
+        *STI);
+
+    if (Size != 1)
+      OutStreamer->emitInstruction(MCInstBuilder(RISCV::ADDI)
+                                       .addReg(RISCV::X28)
+                                       .addReg(RISCV::X28)
+                                       .addImm(Size - 1),
+                                   *STI);
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::BGE)
+            .addReg(RISCV::X28)
+            .addReg(RISCV::X6)
+            .addExpr(MCSymbolRefExpr::create(HandleMismatchSym, OutContext)),
+        *STI);
+
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::ORI).addReg(RISCV::X6).addReg(Reg).addImm(0xF),
+        *STI);
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::LBU).addReg(RISCV::X6).addReg(RISCV::X6).addImm(0),
+        *STI);
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::BEQ)
+            .addReg(RISCV::X6)
+            .addReg(RISCV::X7)
+            .addExpr(MCSymbolRefExpr::create(ReturnSym, OutContext)),
+        *STI);
+
+    OutStreamer->emitLabel(HandleMismatchSym);
+
+    // | Previous stack frames...        |
+    // +=================================+ <-- [SP + 256]
+    // |              ...                |
+    // |                                 |
+    // | Stack frame space for x12 - x31.|
+    // |                                 |
+    // |              ...                |
+    // +---------------------------------+ <-- [SP + 96]
+    // | Saved x11(arg1), as             |
+    // | __hwasan_check_* clobbers it.   |
+    // +---------------------------------+ <-- [SP + 88]
+    // | Saved x10(arg0), as             |
+    // | __hwasan_check_* clobbers it.   |
+    // +---------------------------------+ <-- [SP + 80]
+    // |                                 |
+    // | Stack frame space for x9.       |
+    // +---------------------------------+ <-- [SP + 72]
+    // |                                 |
+    // | Saved x8(fp), as                |
+    // | __hwasan_check_* clobbers it.   |
+    // +---------------------------------+ <-- [SP + 64]
+    // |              ...                |
+    // |                                 |
+    // | Stack frame space for x2 - x7.  |
+    // |                                 |
+    // |              ...                |
+    // +---------------------------------+ <-- [SP + 16]
+    // | Return address (x1) for caller  |
+    // | of __hwasan_check_*.            |
+    // +---------------------------------+ <-- [SP + 8]
+    // | Reserved place for x0, possibly |
+    // | junk, since we don't save it.   |
+    // +---------------------------------+ <-- [x2 / SP]
+
+    // Adjust sp
+    OutStreamer->emitInstruction(MCInstBuilder(RISCV::ADDI)
+                                     .addReg(RISCV::X2)
+                                     .addReg(RISCV::X2)
+                                     .addImm(-256),
+                                 *STI);
+
+    // store x10(arg0) by new sp
+    OutStreamer->emitInstruction(MCInstBuilder(RISCV::SD)
+                                     .addReg(RISCV::X10)
+                                     .addReg(RISCV::X2)
+                                     .addImm(8 * 10),
+                                 *STI);
+    // store x11(arg1) by new sp
+    OutStreamer->emitInstruction(MCInstBuilder(RISCV::SD)
+                                     .addReg(RISCV::X11)
+                                     .addReg(RISCV::X2)
+                                     .addImm(8 * 11),
+                                 *STI);
+
+    // store x8(fp) by new sp
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::SD).addReg(RISCV::X8).addReg(RISCV::X2).addImm(8 *
+                                                                            8),
+        *STI);
+    // store x1(ra) by new sp
+    OutStreamer->emitInstruction(
+        MCInstBuilder(RISCV::SD).addReg(RISCV::X1).addReg(RISCV::X2).addImm(1 *
+                                                                            8),
+        *STI);
+    if (Reg != RISCV::X10)
+      OutStreamer->emitInstruction(MCInstBuilder(RISCV::OR)
+                                       .addReg(RISCV::X10)
+                                       .addReg(RISCV::X0)
+                                       .addReg(Reg),
+                                   *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),
+                                 *STI);
+  }
+}