[PATCH] D66870: [Sanitizers] Add support for RISC-V

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.

I have two comments/queries below.

I don't feel it's an issue that this doesn't support RISC-V 32, though the patch title and summary should be updated to reflect this, and a build requesting risc-v 32 should hard error.



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:411
+#if defined(__riscv)
+  return internal_syscall(SYSCALL(renameat2), AT_FDCWD, (uptr)oldpath, AT_FDCWD,
+                          (uptr)newpath, 0);
----------------
What's the justification here? I'm not familiar with the sanitizer syscall interface, but it would be good to have a comment explaining why risc-v is the only architecture that doesn't use `rename` or `renameat`.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h:100
   const unsigned struct_kernel_stat64_sz = 104;
+#elif defined(__riscv) && __riscv_xlen == 64
+  const unsigned struct_kernel_stat_sz = 128;
----------------
Please can you confirm that this will cause build errors if `__riscv_xlen` is 32 (like it will in `sanitizer_platform_limits_posix.cpp`)?


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66870/new/

https://reviews.llvm.org/D66870