On Thu, Jul 30, 2020 at 5:31 AM Joshua via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > +/* Implement TARGET_ASAN_SHADOW_OFFSET. */ > + > +static unsigned HOST_WIDE_INT > +riscv_asan_shadow_offset (void) > +{ > + return HOST_WIDE_INT_UC (0x10000000); > +}
Is there a reason why you used 0x10000000? Looking at other targets, it appears the convention is 1<<29 for 32-bit targets, and a number larger than 1<<32 for 64-bit targets. I think the RISC-V Linux port has a minimum of 39-bit virtual addresses (SV39) suggesting that this should be 1<<36 for 64-bit targets. I can test the 32-bit support on qemu, and the 64-bit support on hardware, but my hardware is doing other stuff today. I should be able to try testing this tomorrow. Otherwise the gcc stuff is pretty simple and looks OK. We just need to double check these numbers. > diff --git a/libsanitizer/sanitizer_common/sanitizer_common.h > b/libsanitizer/sanitizer_common/sanitizer_common.h > index ac16e0e..ea7dff7 100644 > --- a/libsanitizer/sanitizer_common/sanitizer_common.h > +++ b/libsanitizer/sanitizer_common/sanitizer_common.h > @@ -649,7 +649,8 @@ enum ModuleArch { > kModuleArchARMV7, > kModuleArchARMV7S, > kModuleArchARMV7K, > - kModuleArchARM64 > + kModuleArchARM64, > + kModuleArchRISCV > }; > Libsanitizer patches should go upstream and then be pulled down into gcc. I haven't done libsanitizer work before so I'm not sure of the exact details. I would expect to find the info here: https://gcc.gnu.org/codingconventions.html#upstream but it doesn't mention libsanitizer. I found the rules in the libsanitizer/README.gcc and HOWTO_MERGE files. As expected it says to submit upstream first. You are adding a SANITIZER_RISCV macro but not using it. It isn't clear why you need this, unless maybe it is just for completeness. it looks harmless though, and might be useful later. This is something for upstream reviewers to decide though. In sanitizer_common.h I see a comment // When adding a new architecture, don't forget to also update // script/asan_symbolize.py and sanitizer_symbolizer_libcdep.cpp. but I don't see any script/asan_symbolize.py file. So maybe the comment should be fixed. Or if there is a file in the llvm tree that we don't import into gcc, then you will need a patch for it. if the comment is wrong, then there is a similar comment in sanitizer_symbolizer_libcdep.cpp that needs to be fixed too. If the file is gone, the comment fix can be a separate patch. Otherwise this stuff looks pretty simple and obvious but it needs to be submitted upstream. Jim