This is an archive of the discontinued LLVM Phabricator instance.

[Sanitizers] Add support for RISC-V 64-bit
ClosedPublic

Authored by schwab on Aug 28 2019, 3:49 AM.

Diff Detail

Event Timeline

schwab created this revision.Aug 28 2019, 3:49 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 28 2019, 3:49 AM
Herald added subscribers: llvm-commits, Restricted Project, s.egerton and 8 others. · View Herald Transcript
schwab retitled this revision from [libsanitizer] Add support for RISC-V to [Sanitizers] Add support for RISC-V.Aug 29 2019, 2:11 AM
schwab edited the summary of this revision. (Show Details)
asb added a subscriber: asb.Aug 30 2019, 5:59 AM
luismarques added inline comments.
compiler-rt/cmake/config-ix.cmake
233

Although 32-bit support for Linux on RISC-V has lagged, you can build and boot a 32-bit kernel and it will eventually be more mainstream. Is the reason for not adding RISCV32 here that it was only tested on the HiFive Unleashed? It's possible to get prebuilt 32-bit disk images (and patches to build them yourself) that can be tested with, for instance, QEMU. See for instance [1]. Also, I don't know how problematic it would be to add RISCV32 to the list without testing.
[1] https://bellard.org/tinyemu/buildroot.html (Download section, etc.)

schwab marked an inline comment as done.Sep 2 2019, 6:25 AM
schwab added inline comments.
compiler-rt/cmake/config-ix.cmake
233

The riscv32 ABI isn't finalized yet.

lenary added a comment.Sep 3 2019, 1:58 AM

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.

compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
411

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.

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
97

Please can you confirm that this will cause build errors if __riscv_xlen is 32 (like it will in sanitizer_platform_limits_posix.cpp)?

schwab retitled this revision from [Sanitizers] Add support for RISC-V to [Sanitizers] Add support for RISC-V 64-bit.Sep 3 2019, 2:26 AM
schwab marked 2 inline comments as done.
schwab added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
411

renameat is obsolete, renameat2 is the new generic default.

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
97

A definition of struct_kernel_stat_sz and struct_kernel_stat64_sz is required. That's how you find out that this needs porting.

@schwab Sorry there's been a delay reviewing this. I think it's probably fallen down the crack between "people who are familiar with RISC-V", and "people who are familiar with the sanitizers".

On the RISC-V side, I think @luismarques is your guy, so I've named him as a reviewer. Please also find someone from the sanitizers team who can review this work.

This revision is now accepted and ready to land.Sep 13 2019, 12:06 PM
emaste added a subscriber: mhorne.Sep 27 2019, 8:54 AM
emaste added a subscriber: emaste.
asb added a comment.Oct 3 2019, 5:56 AM

@schwab - do you need someone to commit this for you?

schwab added a comment.Oct 3 2019, 6:06 AM

I don't have commit privileges.

This revision was automatically updated to reflect the committed changes.

I had to revert this in rL375136. Due to some build failures. I need to investigate exactly what the issue is.

luismarques reopened this revision.Oct 17 2019, 12:17 PM
This revision is now accepted and ready to land.Oct 17 2019, 12:17 PM

What was the error?

What was the error?

I now think it was unrelated, but I'm not sure. I now have a setup where I can build the sanitizers, I think, so I'm just about to test the patch myself before pushing to the builders.

I think I've found the error. Comment inline

compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1974

I'm getting compiler errors around these accesses. My sysroot thinks that __gregs isn't a field in this struct, it thinks the field is gregs (It's pointing to /opt/riscv-gcc-toolchain/sysroot/usr/include/bits/sigcontext.h:28:17 as the declaration for the gregs field.

I think this is the version of that file on github: https://github.com/riscv/riscv-glibc/blob/06983fe/sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h#L27 (that git version matches the one used by https://github.com/riscv/riscv-gnu-toolchain)

schwab marked an inline comment as done.Oct 18 2019, 5:07 AM
schwab added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1974

Don't use the obsolete riscv-glibc. The official glibc sources are on sourceware.org.

Ok, It took several days of fighting crosstool-ng, but I finally have a riscv64-unknown-linux-gnu toolchain with glibc 2.30.

I'm happy this builds. I'm going to re-land the patch now. Sorry for the delay.

This revision was automatically updated to reflect the committed changes.

Hello,Can you tell me the resaon why it wasn't support for risc-v before ? Why not add the support for rv32 this time?

Hello,Can you tell me the resaon why it wasn't support for risc-v before ? Why not add the support for rv32 this time?

Just not one had yet put the work to explicitly support it, and be sure that was being done properly --- no deeper reason than that. Regarding riscv32, these is the sanitizer support for Linux, and Linux's RV32 support is being worked on. In particular, there are ABI issues that should be resolved now to avoid the year 2038 problem, and not cause breaking changes to address that at a later point.