llvm enable sanitizer (RISCV64)

[joshua-arch1]

Summary

compiler-rt/

    
* cmake/config-ix.cmake: New options for RISCV64.
* test/asan/CMakeLists.txt: Ditto.

Up till now, only UndefinedBehaviorSanitizer(UBSan) is functionally supported for RISC-V on LLVM. With ASan enabled in this patch, AddressSanitizer(ASan) and KernelAddressSANitizer(KASAN) will be both supported with -DLLVM_DEFAULT_TARGET_TRIPLE="riscv64-unknown-linux-gnu" added when running 'cmake'.

[luismarques]

1. You need to provide the full context in the patch. See [1], particularly the part about the -U999999.
2. You need to base your patch on a recent LLVM commit, as this patch doesn't apply cleanly.
3. This patch repeats changes from D86198. Are you abandoning D86198? If so you need to update the status of that review.
4. I would expect this review to provide a more detailed summary, including the support status of the various sanitizers for RISC-V, and why it's reasonable to enable them.

[1] https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

[joshua-arch1]

In D86198#2225802, @luismarques wrote:

1. You need to provide the full context in the patch. See [1], particularly the part about the -U999999.
2. You need to base your patch on a recent LLVM commit, as this patch doesn't apply cleanly.
3. This patch repeats changes from D86198. Are you abandoning D86198? If so you need to update the status of that review.
4. I would expect this review to provide a more detailed summary, including the support status of the various sanitizers for RISC-V, and why it's reasonable to enable them.

[1] https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

What do you mean by '3) This patch repeats changes from D86198'? This patch is actually D86198.
Did you mean D84727? For that patch, no one has given an update feedback so far.

[joshua-arch1]

1. to provide the full context in the patch
2. to base the patch on a recent LLVM commit

[joshua-arch1]

In D86198#2225802, @luismarques wrote:

1. You need to provide the full context in the patch. See [1], particularly the part about the -U999999.
2. You need to base your patch on a recent LLVM commit, as this patch doesn't apply cleanly.
3. This patch repeats changes from D86198. Are you abandoning D86198? If so you need to update the status of that review.
4. I would expect this review to provide a more detailed summary, including the support status of the various sanitizers for RISC-V, and why it's reasonable to enable them.

[1] https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

Hi I have updated the patch according to your comments.

[luismarques]

In D86198#2227534, @joshua-arch1 wrote:

Did you mean D84727? For that patch, no one has given an update feedback so far.

Yes, I meant D84727. I don't see why the lack of additional feedback on that patch lead you to submit another patch that also enabled ASan, among other sanitizers.

Hi I have updated the patch according to your comments.

You didn't actually address point 4. The review summary doesn't accurately list the enabled sanitizers and doesn't provide an explanation or evidence for why enabling them is reasonable. For instance, this patch enables DFSan but there is no actual support for RISC-V, and it won't actually compile as is.

[joshua-arch1]

In D86198#2227922, @luismarques wrote:

In D86198#2227534, @joshua-arch1 wrote:

Did you mean D84727? For that patch, no one has given an update feedback so far.

Yes, I meant D84727. I don't see why the lack of additional feedback on that patch lead you to submit another patch that also enabled ASan, among other sanitizers.

Hi I have updated the patch according to your comments.

You didn't actually address point 4. The review summary doesn't accurately list the enabled sanitizers and doesn't provide an explanation or evidence for why enabling them is reasonable. For instance, this patch enables DFSan but there is no actual support for RISC-V, and it won't actually compile as is.

For D84727, I didn't know how to correctly submit an updated patch before. That is why I submitted another patch. For the current stage, you can just abandon D84727.

[luismarques]

In D86198#2227975, @joshua-arch1 wrote:

For D84727, I didn't know how to correctly submit an updated patch before. That is why I submitted another patch. For the current stage, you can just abandon D84727.

For D84727 you need to go to the dropdown menu near the bottom that says "Add Action..." and choose "Abandon Revision". To update a patch you use the "Update Diff" link elsewhere on the review page.

[joshua-arch1]

Remove some unnecessary sanitizers and update the summary

[joshua-arch1]

This comment was removed by joshua-arch1.

[joshua-arch1]

In D86198#2228030, @luismarques wrote:

In D86198#2227975, @joshua-arch1 wrote:

For D84727, I didn't know how to correctly submit an updated patch before. That is why I submitted another patch. For the current stage, you can just abandon D84727.

For D84727 you need to go to the dropdown menu near the bottom that says "Add Action..." and choose "Abandon Revision". To update a patch you use the "Update Diff" link elsewhere on the review page.

I have updated my patch and summary. Are there any new updates with the patch review?

[luismarques]

In D86198#2243850, @joshua-arch1 wrote:

I have updated my patch and summary. Are there any new updates with the patch review?

There are still issues with compiling and running the sanitizer tests for the added architecture. I think fixing those should be a precondition for this to be considered mergeable.

[joshua-arch1]

In D86198#2244488, @luismarques wrote:

In D86198#2243850, @joshua-arch1 wrote:

I have updated my patch and summary. Are there any new updates with the patch review?

There are still issues with compiling and running the sanitizer tests for the added architecture. I think fixing those should be a precondition for this to be considered mergeable.

Have you added the flag "-DLLVM_DEFAULT_TARGET_TRIPLE="riscv64-unknown-linux-gnu"" and "-fsanitize=address" when compiling the sanitizer test?

[luismarques]

There are still issues with compiling and running the sanitizer tests for the added architecture. I think fixing those should be a precondition for this to be considered mergeable.

Have you added the flag "-DLLVM_DEFAULT_TARGET_TRIPLE="riscv64-unknown-linux-gnu"" and "-fsanitize=address" when compiling the sanitizer test?

It's not that I had trouble compiling tests due to an incorrect configuration (I think), it's that when running the entire set of sanitizer tests I encountered things (e.g. #ifdefs) that weren't yet implemented for RISC-V, and so would fail to compile or run. Have you tried running the entire set of (relevant) sanitizer checks?

[joshua-arch1]

In D86198#2260639, @luismarques wrote:

There are still issues with compiling and running the sanitizer tests for the added architecture. I think fixing those should be a precondition for this to be considered mergeable.

Have you added the flag "-DLLVM_DEFAULT_TARGET_TRIPLE="riscv64-unknown-linux-gnu"" and "-fsanitize=address" when compiling the sanitizer test?

It's not that I had trouble compiling tests due to an incorrect configuration (I think), it's that when running the entire set of sanitizer tests I encountered things (e.g. #ifdefs) that weren't yet implemented for RISC-V, and so would fail to compile or run. Have you tried running the entire set of (relevant) sanitizer checks?

I think it is necessary to add the flag "-DLLVM_DEFAULT_TARGET_TRIPLE="riscv64-unknown-linux-gnu"" and "-fsanitize=address" when running the entire set of sanitizer check. Note that currently I have only added support for the address sanitizer.

[luismarques]

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

compiler-rt/cmake/config-ix.cmake

if(APPLE)
   set(ARM64 arm64)
   set(ARM32 armv7 armv7s armv7k)
   set(X86_64 x86_64 x86_64h)
 endif()
 
 set(ALL_SANITIZER_COMMON_SUPPORTED_ARCH ${X86} ${X86_64} ${PPC64} ${RISCV64}
     ${ARM32} ${ARM64} ${MIPS32} ${MIPS64} ${S390X} ${SPARC} ${SPARCV9})
 set(ALL_ASAN_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64}
-    ${MIPS32} ${MIPS64} ${PPC64} ${S390X} ${SPARC} ${SPARCV9})
+    ${MIPS32} ${MIPS64} ${PPC64} ${S390X} ${SPARC} ${SPARCV9} ${RISCV64})
 set(ALL_CRT_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${RISCV32} ${RISCV64})
-set(ALL_DFSAN_SUPPORTED_ARCH ${X86_64} ${MIPS64} ${ARM64})
+set(ALL_DFSAN_SUPPORTED_ARCH ${X86_64} ${MIPS64} ${ARM64} ${RISCV64})
 
 if(ANDROID)
   set(OS_NAME "Android")
 else()
   set(OS_NAME "${CMAKE_SYSTEM_NAME}")
 endif()
 
 if(OS_NAME MATCHES "Linux")
-  set(ALL_FUZZER_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM64} ${S390X})
+  set(ALL_FUZZER_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM64} ${S390X} ${RISCV64})
 elseif (OS_NAME MATCHES "Windows")
   set(ALL_FUZZER_SUPPORTED_ARCH ${X86} ${X86_64})
 elseif(OS_NAME MATCHES "Android")
   set(ALL_FUZZER_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64})
 else()
   set(ALL_FUZZER_SUPPORTED_ARCH ${X86_64} ${ARM64})
 endif()
 
-set(ALL_GWP_ASAN_SUPPORTED_ARCH ${X86} ${X86_64})
+set(ALL_GWP_ASAN_SUPPORTED_ARCH ${X86} ${X86_64} ${RISCV64})
 if(APPLE)
   set(ALL_LSAN_SUPPORTED_ARCH ${X86} ${X86_64} ${MIPS64} ${ARM64})
 else()
   set(ALL_LSAN_SUPPORTED_ARCH ${X86} ${X86_64} ${MIPS64} ${ARM64} ${ARM32} ${PPC64} ${S390X})
 endif()
-set(ALL_MSAN_SUPPORTED_ARCH ${X86_64} ${MIPS64} ${ARM64} ${PPC64} ${S390X})
-set(ALL_HWASAN_SUPPORTED_ARCH ${X86_64} ${ARM64})
+set(ALL_MSAN_SUPPORTED_ARCH ${X86_64} ${MIPS64} ${ARM64} ${PPC64} ${S390X} ${RISCV64})
+set(ALL_HWASAN_SUPPORTED_ARCH ${X86_64} ${ARM64} ${RISCV64})
 set(ALL_PROFILE_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${PPC64}
-    ${MIPS32} ${MIPS64} ${S390X} ${SPARC} ${SPARCV9})
-set(ALL_TSAN_SUPPORTED_ARCH ${X86_64} ${MIPS64} ${ARM64} ${PPC64})
+    ${MIPS32} ${MIPS64} ${S390X} ${SPARC} ${SPARCV9} ${RISCV64})
+set(ALL_TSAN_SUPPORTED_ARCH ${X86_64} ${MIPS64} ${ARM64} ${PPC64} ${RISCV64})
 set(ALL_UBSAN_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${RISCV64}
     ${MIPS32} ${MIPS64} ${PPC64} ${S390X} ${SPARC} ${SPARCV9})
 set(ALL_SAFESTACK_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM64} ${MIPS32} ${MIPS64})
-set(ALL_CFI_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${MIPS64})
-set(ALL_SCUDO_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${MIPS32} ${MIPS64} ${PPC64})
-set(ALL_SCUDO_STANDALONE_SUPPORTED_ARCH ${X86} ${X86_64})
+set(ALL_CFI_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${MIPS64} ${RISCV64})
+set(ALL_SCUDO_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${MIPS32} ${MIPS64} ${PPC64} ${RISCV64})
+set(ALL_SCUDO_STANDALONE_SUPPORTED_ARCH ${X86} ${X86_64} ${RISCV64})
 if(APPLE)
 set(ALL_XRAY_SUPPORTED_ARCH ${X86_64})
 else()
 set(ALL_XRAY_SUPPORTED_ARCH ${X86_64} ${ARM32} ${ARM64} ${MIPS32} ${MIPS64} powerpc64le)
 endif()
 set(ALL_SHADOWCALLSTACK_SUPPORTED_ARCH ${ARM64})
 
 if(APPLE)

compiler-rt/test/asan/CMakeLists.txt

 set(ASAN_LIT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
 
 set(ASAN_TESTSUITES)
 set(ASAN_DYNAMIC_TESTSUITES)
 
 # Before Windows 8 (CMAKE_SYSTEM_VERSION 6.2), reserving large regions of shadow
 # memory allocated physical memory for page tables, which made it very
 # unreliable. Remove the asan tests from check-all in this configuration.
 set(SHADOW_MAPPING_UNRELIABLE FALSE)
 if(OS_NAME MATCHES "Windows" AND CMAKE_SIZEOF_VOID_P EQUAL 8 AND
     CMAKE_SYSTEM_VERSION LESS 6.2)
   set(SHADOW_MAPPING_UNRELIABLE TRUE)
   message(WARNING "Disabling ASan tests because they are unreliable on Windows 7 and earlier")
 endif()
 
 macro(get_bits_for_arch arch bits)
-  if (${arch} MATCHES "x86_64|powerpc64|powerpc64le|aarch64|arm64|mips64|mips64el|s390x|sparcv9")
+  if (${arch} MATCHES "x86_64|powerpc64|powerpc64le|aarch64|arm64|mips64|mips64el|s390x|sparcv9|riscv64")
     set(${bits} 64)
   elseif (${arch} MATCHES "i386|arm|mips|mipsel|sparc")
     set(${bits} 32)
   else()
     message(FATAL_ERROR "Unknown target architecture: ${arch}")
   endif()
 endmacro()