[RISC-V] Add support for AddressSanitizer on RISC-V GCC

[joshua-arch1]

Summary

libsanitizer/

    
* sanitizer_common/sanitizer_common.h (ModuleArch): New enumerator.
(ModuleArchToString): New architecture option.
* sanitizer_common/sanitizer_symbolizer_libcdep.cpp (GetArgV): New architecture option.

Up till now, only UndefinedBehaviorSanitizer(UBSan) is functionally supported for RISC-V on GCC. With ASan enabled in this patch, AddressSanitizer(ASan) and KernelAddressSANitizer(KASAN) will be both supported with "-fsanitize=address" added when compiling and running the tests.

[kito-cheng]

Hi Jun:

GCC part should review at gcc-patch mailing list instead of here, for other files (sanitizer_platform.h, sanitizer_common.h and sanitizer_symbolizer_libcdep.cpp), it's right place to review, but it should using the diff which generated within LLVM source-tree, and GCC will sync libsanitizer after LLVM is accepted.

libsanitizer/sanitizer_common/sanitizer_common.h

675

Seems like you also need to update asan_symbolize.py according this comment, don't remove that I know it not exist in gcc source tree, but LLVM source tree have that[1].

[1] https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/asan/scripts/asan_symbolize.py

libsanitizer/sanitizer_common/sanitizer_platform.h

221

This marco is not necessary to define if you don't use anywhere.

libsanitizer/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

252

Same as above, don't remove this.

[joshua-arch1]

In D86195#2225532, @kito-cheng wrote:

Hi Jun:

GCC part should review at gcc-patch mailing list instead of here, for other files (sanitizer_platform.h, sanitizer_common.h and sanitizer_symbolizer_libcdep.cpp), it's right place to review, but it should using the diff which generated within LLVM source-tree, and GCC will sync libsanitizer after LLVM is accepted.

What about config/riscv/riscv.c? Should I place it here to review?

[kito-cheng]

Hi Jun:

GCC part should review at gcc-patch mailing list instead of here, for other files (sanitizer_platform.h, sanitizer_common.h and sanitizer_symbolizer_libcdep.cpp), it's right place to review, but it should using the diff which generated within LLVM source-tree, and GCC will sync libsanitizer after LLVM is accepted.

What about config/riscv/riscv.c? Should I place it here to review?

For config/riscv/riscv.c, it's part of GCC source tree, so there is wrong place to review :P

[joshua-arch1]

This comment was removed by joshua-arch1.

[zzheng]

can you generate the diff with full context?

git format-patch -U99999

[joshua-arch1]

1. to remove unnecessary definitions
2. to generate the diff with full context
3. to generate the diff on LLVM source tree

[joshua-arch1]

In D86195#2226367, @zzheng wrote:

can you generate the diff with full context?

git format-patch -U99999

Hi I have updated the patch. Now it has full context.

[joshua-arch1]

In D86195#2225591, @kito-cheng wrote:

Hi Jun:

GCC part should review at gcc-patch mailing list instead of here, for other files (sanitizer_platform.h, sanitizer_common.h and sanitizer_symbolizer_libcdep.cpp), it's right place to review, but it should using the diff which generated within LLVM source-tree, and GCC will sync libsanitizer after LLVM is accepted.

What about config/riscv/riscv.c? Should I place it here to review?

For config/riscv/riscv.c, it's part of GCC source tree, so there is wrong place to review :P

I have updated the patch based on LLVM source tree. When will it be reviewed?

[luismarques]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

273–274 ↗

(On Diff #286725)

The preprocessor definition should be __riscv, and the arch riscv32 or riscv64. Given that this is incorrect, I suspect this patch needs much more testing. When you update the patch please update the description with more details of why you think this is in good shape to be merged. Adding more tests would make that easier.

[joshua-arch1]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

273–274 ↗

(On Diff #286725)

The preprocessor definition should be __riscv, and the arch riscv32 or riscv64. Given that this is incorrect, I suspect this patch needs much more testing. When you update the patch please update the description with more details of why you think this is in good shape to be merged. Adding more tests would make that easier.

Hi, I have updated my patch and its summary.

[luismarques]

RISC-V support for ASan has been merged (see D87582 and its predecessors). This patch should probably be abandoned. Thank you.

gcc/config/riscv/riscv.c

   return true;
 }
 
+/* Implement TARGET_ASAN_SHADOW_OFFSET.  */
+
+static unsigned HOST_WIDE_INT
+riscv_asan_shadow_offset (void)
+{
+  return HOST_WIDE_INT_1U << 36;
+}
+
 /* Initialize the GCC target structure.  */
 #undef TARGET_ASM_ALIGNED_HI_OP
 #define TARGET_ASM_ALIGNED_HI_OP "\t.half\t"

 #undef TARGET_NEW_ADDRESS_PROFITABLE_P
 #define TARGET_NEW_ADDRESS_PROFITABLE_P riscv_new_address_profitable_p
 
+#undef TARGET_ASAN_SHADOW_OFFSET
+#define TARGET_ASAN_SHADOW_OFFSET riscv_asan_shadow_offset
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 #include "gt-riscv.h"

libsanitizer/sanitizer_common/sanitizer_common.h

   kModuleArchARMV7,
   kModuleArchARMV7S,
   kModuleArchARMV7K,
-  kModuleArchARM64
+  kModuleArchARM64,
+  kModuleArchRISCV
 };
 
 // Opens the file 'file_name" and reads up to 'max_len' bytes.

                       error_t *errno_p = nullptr);
 
 // When adding a new architecture, don't forget to also update
-// script/asan_symbolize.py and sanitizer_symbolizer_libcdep.cpp.
+// sanitizer_symbolizer_libcdep.cpp.
 inline const char *ModuleArchToString(ModuleArch arch) {
   switch (arch) {
     case kModuleArchUnknown:

       return "armv7k";
     case kModuleArchARM64:
       return "arm64";
+    case kModuleArchRISCV;
+      return "riscv";
   }
   CHECK(0 && "Invalid module arch");
   return "";

libsanitizer/sanitizer_common/sanitizer_platform.h

 # define SANITIZER_MYRIAD2 0
 #endif
 
+
+#if defined(__riscv__)
+# define SANITIZER_RISCV 1
+#else
+# define SANITIZER_RISCV 0
+#endif
+
+
 // By default we allow to use SizeClassAllocator64 on 64-bit platform.
 // But in some cases (e.g. AArch64's 39-bit address space) SizeClassAllocator64
 // does not work well and we need to fallback to SizeClassAllocator32.

libsanitizer/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

   }
 
   // When adding a new architecture, don't forget to also update
-  // script/asan_symbolize.py and sanitizer_common.h.
+  // sanitizer_common.h.
   void GetArgV(const char *path_to_binary,
                const char *(&argv)[kArgVMax]) const override {
 #if defined(__x86_64h__)

     const char* const kSymbolizerArch = "--default-arch=s390x";
 #elif defined(__s390__)
     const char* const kSymbolizerArch = "--default-arch=s390";
+#elif defined(__riscv__)
+    const char* const kSymbolizerArch = "--default-arch=riscv";
 #else
     const char* const kSymbolizerArch = "--default-arch=unknown";
 #endif