[RISC-V][HWASAN] Add support for HWASAN code instrumentation for RISC-V

[smd]

Summary

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

Depends On D132995

[vitalybuka]

some test needs to be updated/added

[smd]

In D131575#3719631, @vitalybuka wrote:

some test needs to be updated/added

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

[smd]

Addressing comments

[vitalybuka]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

838

Code should respect ClUseShortGranules, even if it's not useful mode.
Otherwise UseShortGranules should be true for non android

997

Why not here?

[smd]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

838

@vitalybuka
Could you please clarify:
If I get you right, you're suggesting allowing users to override UseShortGranules and make it false, even if the platform doesn't support this mode, right?
For cases rather than this, short granules would be always used. If the user decides to use non-short granules for riscv, in this case the compilation would fail with:

fatal error: error in backend: Cannot select: intrinsic %llvm.hwasan.check.memaccess

which is sort of ok, I guess. But maybe an explicit check like (isRISCV64() && !UseShortGranules) with an error message would be a better idea?

[vitalybuka]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

838

@vitalybuka
Could you please clarify:
If I get you right, you're suggesting allowing users to override UseShortGranules and make it false, even if the platform doesn't support this mode, right?
For cases rather than this, short granules would be always used. If the user decides to use non-short granules for riscv, in this case the compilation would fail with:

fatal error: error in backend: Cannot select: intrinsic %llvm.hwasan.check.memaccess

which is sort of ok, I guess. But maybe an explicit check like (isRISCV64() && !UseShortGranules) with an error message would be a better idea?

I assume hwasan.check.memaccess is just not implemented yet?
copt<> are internal flags, if some one using them, it's their responsibility to understand consequences.
So I would prefer we respect user provided ClUseShortGranules and special error is not needed.

in this case these lines not need to change

[smd]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

838

I assume hwasan.check.memaccess is just not implemented yet?

Indeed it's not implemented, but to be honest I didn't think it should be implemented. I got an impression that short_granules(v2) is a preferred way of checking tags, while v1 function is kept for older targets.
Or am I wrong?

[smd]

Addressing comments

[vitalybuka]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

583–585

clang-format please

605

[smd]

Addressing comments

[smd]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

583–585

It's already formatted

605

Fixed, thanks

[smd]

Don't push tagged globals option for riscv64 hwasan tests

[smd]

Fixing bug

[smd]

Hi @vitalybuka
Could you please take another look at this patch series and if you wouldn't have any objections, I'd like to push it.
Thanks

[vitalybuka]

Done.
I guess you don't need another round if it's accepted and you just addressing trivial issues.

compiler-rt/test/hwasan/lit.cfg.py

26 ↗

(On Diff #453934)

It belongs to a different patch?

[vitalybuka]

Please try to start landing accepted patches to have a few hours delay in between.
If you land all at once, it would be annoying to revert entire stack to fix a bot.

[smd]

Addressing comments

[arsenm]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

923

/home/matt/src/llvm-project/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:923:72: warning: implicit conversion turns string literal into bool: 'const char[6]' to 'bool' [-Wstring-conversion]

"ebreak\naddiw x0, x0, " + itostr(0x40 + AccessInfo), "{x10}", "{x11}",

[smd]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

923

For some reason this code works with g++, but indeed fails with clang++. I'll investigate this, thanks:

g++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/build/llvm-project/lib/Transforms/Instrumentation -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/lib/Transforms/Instrumentation -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/build/llvm-project/include -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-uninitialized  -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -g  -fno-exceptions -fno-rtti -std=c++17 -o CMakeFiles/LLVMInstrumentation.dir/HWAddressSanitizer.cpp.o -c /tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp -Wall -Werror

smd@neptune:/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/build/llvm-project/lib/Transforms/Instrumentation$ clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/build/llvm-project/lib/Transforms/Instrumentation -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/lib/Transforms/Instrumentation -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/build/llvm-project/include -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-uninitialized  -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -g  -fno-exceptions -fno-rtti -std=c++17 -o CMakeFiles/LLVMInstrumentation.dir/HWAddressSanitizer.cpp.o -c /tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp -Wall -Werror -Wstring-conversion
/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:923:72: error: implicit conversion turns string literal into bool: 'const char [6]' to 'bool' [-Werror,-Wstring-conversion]
        "ebreak\naddiw x0, x0, " + itostr(0x40 + AccessInfo), "{x10}", "{x11}",
                                                                       ^~~~~~~
1 error generated.

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
+#include "llvm/IR/NoFolder.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/Value.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/Instrumentation/AddressSanitizerCommon.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
void HWAddressSanitizer::initializeModule() {
   // short granules, global or personality function instrumentation. On other
   // platforms we currently require using the latest version of the runtime.
   bool NewRuntime =
       !TargetTriple.isAndroid() || !TargetTriple.isAndroidVersionLT(30);
 
   UseShortGranules =
       ClUseShortGranules.getNumOccurrences() ? ClUseShortGranules : NewRuntime;
   OutlinedChecks =
-      TargetTriple.isAArch64() && TargetTriple.isOSBinFormatELF() &&
+      (TargetTriple.isAArch64() || TargetTriple.isRISCV64()) &&
+      TargetTriple.isOSBinFormatELF() &&
       (ClInlineAllChecks.getNumOccurrences() ? !ClInlineAllChecks : !Recover);
 
   if (ClMatchAllTag.getNumOccurrences()) {
     if (ClMatchAllTag != -1) {
       HasMatchAllTag = true;
       MatchAllTag = ClMatchAllTag & 0xFF;
     }
   } else if (CompileKernel) {
     HasMatchAllTag = true;
     MatchAllTag = 0xFF;
   }
 
   // If we don't have personality function support, fall back to landing pads.
   InstrumentLandingPads = ClInstrumentLandingPads.getNumOccurrences()
                               ? ClInstrumentLandingPads
                               : !NewRuntime;
 
   if (!CompileKernel) {
     createHwasanCtorComdat();
     bool InstrumentGlobals =
         ClGlobals.getNumOccurrences() ? ClGlobals : NewRuntime;
 
-    if (InstrumentGlobals && !UsePageAliases)
+    // Currently we do not instrumentation of globals for RISCV
+    // The reason is that the existing memory models does not allow us
+    // to use tagged pointers in la/lla expressions
+    if (InstrumentGlobals && !UsePageAliases && !TargetTriple.isRISCV64())
       instrumentGlobals();
 
     bool InstrumentPersonalityFunctions =
         ClInstrumentPersonalityFunctions.getNumOccurrences()
             ? ClInstrumentPersonalityFunctions
             : NewRuntime;
     if (InstrumentPersonalityFunctions)
       instrumentPersonalityFunctions();

 
 static size_t TypeSizeToSizeIndex(uint32_t TypeSize) {
   size_t Res = countTrailingZeros(TypeSize / 8);
   assert(Res < kNumberOfAccessSizes);
   return Res;
 }
 
 void HWAddressSanitizer::untagPointerOperand(Instruction *I, Value *Addr) {
-  if (TargetTriple.isAArch64() || TargetTriple.getArch() == Triple::x86_64)
+  if (TargetTriple.isAArch64() || TargetTriple.getArch() == Triple::x86_64 ||
+      TargetTriple.isRISCV64())
     return;
 
   IRBuilder<> IRB(I);
   Value *AddrLong = IRB.CreatePointerCast(Addr, IntptrTy);
   Value *UntaggedPtr =
       IRB.CreateIntToPtr(untagPointer(IRB, AddrLong), Addr->getType());
   I->setOperand(getPointerOperandIndex(I), UntaggedPtr);
 }

 void HWAddressSanitizer::instrumentMemAccessOutline(Value *Ptr, bool IsWrite,
                                                     unsigned AccessSizeIndex,
                                                     Instruction *InsertBefore) {
   assert(!UsePageAliases);
   const int64_t AccessInfo = getAccessInfo(IsWrite, AccessSizeIndex);
   IRBuilder<> IRB(InsertBefore);
   Module *M = IRB.GetInsertBlock()->getParent()->getParent();
   Ptr = IRB.CreateBitCast(Ptr, Int8PtrTy);
+  // In case of RISC-V always use shortgranules
   IRB.CreateCall(Intrinsic::getDeclaration(
-                     M, UseShortGranules
+                     M, UseShortGranules || TargetTriple.isRISCV64()
                             ? Intrinsic::hwasan_check_memaccess_shortgranules
                             : Intrinsic::hwasan_check_memaccess),
                  {ShadowBase, Ptr, ConstantInt::get(Int32Ty, AccessInfo)});
 }
 
 void HWAddressSanitizer::instrumentMemAccessInline(Value *Ptr, bool IsWrite,
                                                    unsigned AccessSizeIndex,
                                                    Instruction *InsertBefore) {
void HWAddressSanitizer::instrumentMemAccessInline(Value *Ptr, bool IsWrite,
   case Triple::aarch64_be:
     // The signal handler will find the data address in x0.
     Asm = InlineAsm::get(
         FunctionType::get(IRB.getVoidTy(), {PtrLong->getType()}, false),
         "brk #" + itostr(0x900 + (AccessInfo & HWASanAccessInfo::RuntimeMask)),
         "{x0}",
         /*hasSideEffects=*/true);
     break;
+  case Triple::riscv64:
+    // The signal handler will find the data address in x10.
+    Asm = InlineAsm::get(
+        FunctionType::get(IRB.getVoidTy(), {PtrLong->getType()}, false),
+        "ebreak\naddiw x0, x0, " + itostr(0x40 + AccessInfo), "{x10}", "{x11}",
+        /*hasSideEffects=*/true);
+    break;
   default:
     report_fatal_error("unsupported architecture");
   }
   IRB.CreateCall(Asm, PtrLong);
   if (Recover)
     cast<BranchInst>(CheckFailTerm)->setSuccessor(0, CheckTerm->getParent());
 }
 
bool HWAddressSanitizer::instrumentMemAccess(InterestingMemoryOperand &O) {
   untagPointerOperand(O.getInsn(), Addr);
 
   return true;
 }
 
 void HWAddressSanitizer::tagAlloca(IRBuilder<> &IRB, AllocaInst *AI, Value *Tag,
                                    size_t Size) {
   size_t AlignedSize = alignTo(Size, Mapping.getObjectAlignment());
   if (!UseShortGranules)
     Size = AlignedSize;
 
   Value *JustTag = IRB.CreateTrunc(Tag, IRB.getInt8Ty());
   if (InstrumentWithCalls) {
     IRB.CreateCall(HwasanTagMemoryFunc,
                    {IRB.CreatePointerCast(AI, Int8PtrTy), JustTag,
                     ConstantInt::get(IntptrTy, AlignedSize)});
   } else {