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
