Page MenuHomePhabricator

[RISCV][ASAN] support code for architecture-specific parts of asan
Needs ReviewPublic

Authored by EccoTheDolphin on Sat, Sep 12, 6:17 PM.

Details

Summary

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

Depends On D87579

Diff Detail

Event Timeline

EccoTheDolphin created this revision.Sat, Sep 12, 6:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Sep 12, 6:17 PM
Herald added subscribers: Restricted Project, evandro, luismarques and 11 others. · View Herald Transcript
EccoTheDolphin requested review of this revision.Sat, Sep 12, 6:17 PM
luismarques requested changes to this revision.Mon, Sep 14, 6:15 AM
luismarques added inline comments.
compiler-rt/lib/asan/asan_mapping.h
82

Nit: RISC-V.

86

Nit: missing period before "High"?

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
309–311

The comment and the value were copied from AArch64, but they are incorrect for RISC-V.
The size of struct pthread that I see when I build glibc on riscv64 Linux is 1936.

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
74–76

Since you copied the size of struct pthread from AArch64 and it was incorrect, I suspect that here the same might have happened... :-)

This revision now requires changes to proceed.Mon, Sep 14, 6:15 AM
eugenis added inline comments.
compiler-rt/lib/asan/asan_mapping.h
227

missing indent

compiler-rt/lib/crt/crtbegin.c
55 ↗(On Diff #291435)

crtbegin change is unrelated, please move it to a different revision

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

This is a bit too long, please define SANITIZER_RISCV64 (or some other name) in sanitizer_platform.h, similar to SANITIZER_X32.

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
74–76

There is a compile-time test for this in sanitizer_platform_limits_linux.cpp

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

I assume that this is not just about this file?
If yes, then can we address this as part of this commit without modifications of the diffs before (less than) D87580

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
309–311

well, actually when I build glibc for RISCV the size I get is 1792 (so the value we use is off by 4 bytes for whatever reason.

I don't remember how exactly this value was calculated originally (but I definitely believe that I've checked it somehow). Moreover, I don't know/remember an effective way to do that :).

This time I did is as follows (I modified nptl sources and re-compiled):

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 670cb8ffe6..6d50fa76f1 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -169,6 +169,9 @@ stack_list_add (list_t *elem, list_t *list)
 static struct pthread *
 get_cached_stack (size_t *sizep, void **memp)
 {
+  int array[sizeof (struct pthread)];
+  __builtin_printf("%d", &array);
+
   size_t size = *sizep;
   struct pthread *result = NULL;
   list_t *entry;

GCC gives me the following error:

warning: format '%d' expects argument of type 'int', but argument 2 has type 'int (*)[1792]'

Hacky? Yes!. But we have the result ).

This is for glibc-2.29.

The target compiler (riscv64-unknown-linux-gnu, 9.2.0) uses the following (default) options:

 '-std=gnu11' '-fgnu89-inline' '-g' '-O2' '-Wall' '-Wwrite-strings' '-Wundef' '-fmerge-all-constants' '-frounding-math' '-fno-stack-protector' '-Wstrict-prototypes' '-Wold-style-definition' '-fmath-errno' '-ftls-model=initial-exec' \
'-march=rv64gc' '-mabi=lp64d
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
74–76

Apparently, these definitions were merged in https://reviews.llvm.org/D66870. (git show 977205b595cd65fbeb4a045daf34990d8e8a5efd, by Sam Elliott <selliott@lowrisc.org>)

The value for struct_kernel_stat_sz is the correct one (I've double checked on qemu). I have doubts about struct_kernel_stat64_sz though since it looks like that this struct is not used on rv64. Linux kernel have the following header:

// from arch/riscv/include/generated/uapi/asm/stat.h
#include <asm-generic/stat.h>

// include/uapi/asm-generic/stat.h
/* This matches struct stat64 in glibc2.1. Only used for 32 bit. */
#if BITS_PER_LONG != 64 || defined(ARCH_WANT_STAT64)
struct stat64 {
...
#endif

So given that this is for rv64 only, probably it would be more correct to define it as 0?

74–76

This test passes, though it does not perform checks for stat64, we have the following there:

#if defined(i386)
COMPILER_CHECK(struct_kernel_stat64_sz == sizeof(struct stat64));
#endif

factored-out crt-related change

corrected indentation to comply with existing code style

EccoTheDolphin edited the summary of this revision. (Show Details)Sun, Sep 20, 11:39 PM
EccoTheDolphin added a reviewer: eugenis.
EccoTheDolphin marked 5 inline comments as done.
EccoTheDolphin added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
716
EccoTheDolphin marked 2 inline comments as done.Sun, Sep 20, 11:42 PM
eugenis added inline comments.Mon, Sep 21, 1:02 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
74–76

OK, this makes sense.

eugenis added inline comments.Mon, Sep 21, 1:07 PM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
716

I'd prefer if the patches were changed to use the new macro from the start. It would produce cleaner git history.

addressing comments

EccoTheDolphin edited the summary of this revision. (Show Details)Tue, Sep 22, 11:41 AM
EccoTheDolphin marked an inline comment as done.Tue, Sep 22, 11:43 AM
EccoTheDolphin added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
716

modified the order of patches. now the patch series starts with https://reviews.llvm.org/D87997, gradually adopting the new define.

eugenis added inline comments.Tue, Sep 22, 1:03 PM
compiler-rt/lib/sanitizer_common/sanitizer_common_syscalls.inc
2300

remove "defined". SANITIZER_* macros are not defined/undefined, they are 1/0.

EccoTheDolphin marked an inline comment as done.Tue, Sep 22, 1:33 PM
EccoTheDolphin added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_common_syscalls.inc
2300

remove "defined". SANITIZER_* macros are not defined/undefined, they are 1/0.

Yes. You are correct. Quite an embarrassing error. Will fix.

Actually, one way to fix it would be to avoid defining zero value. However, this will violate the current coding conventions, so I guess it's better to update the patchset once again.

eugenis added inline comments.Tue, Sep 22, 1:36 PM
compiler-rt/lib/sanitizer_common/sanitizer_common_syscalls.inc
2300

Right. This way is shorter and the compiler will complain if you forget to include the header, instead of assuming that the property is false (when the macro is undefined).

addressing review comments

EccoTheDolphin marked 2 inline comments as done.Tue, Sep 22, 8:26 PM
luismarques added inline comments.Wed, Sep 23, 6:26 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
309–311

well, actually when I build glibc for RISCV the size I get is 1792 (so the value we use is off by 4 bytes for whatever reason.

What do you mean? You have 1772 in the patch, and you got 1792, I'm confused by the "off by 4 bytes" comment.

warning: format '%d' expects argument of type 'int', but argument 2 has type 'int (*)[1792]'

Hacky? Yes!. But we have the result ).

This is for glibc-2.29.

Ha! Hey, whatever works, right? I had used a static assert, but it was with a more recent version of glibc.

EccoTheDolphin added inline comments.Wed, Sep 23, 7:05 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
309–311

What do you mean? You have 1772 in the patch, and you got 1792, I'm confused by the "off by 4 bytes" comment.

The original version of the patch had 1776.

Ha! Hey, whatever works, right? I had used a static assert, but it was with a more recent version of glibc.

By a more recent version of gcc - you mean 2.30+?
If this is the case - do you have any recommendations how we could handle that? Should I add some kind of #ifdef?

rebasing + addressing formatting issues