Page MenuHomePhabricator

[RISC-V] Add support for AddressSanitizer on RISC-V GCC
Needs ReviewPublic

Authored by joshua-arch1 on Aug 18 2020, 11:17 PM.

Details

Summary

libsanitizer/

    
* sanitizer_common/sanitizer_common.h (ModuleArch): New enumerator.
(ModuleArchToString): New architecture option.
* sanitizer_common/sanitizer_symbolizer_libcdep.cpp (GetArgV): New architecture option.

Up till now, only UndefinedBehaviorSanitizer(UBSan) is functionally supported for RISC-V on GCC. With ASan enabled in this patch, AddressSanitizer(ASan) and KernelAddressSANitizer(KASAN) will be both supported with "-fsanitize=address" added when compiling and running the tests.

Diff Detail

Event Timeline

joshua-arch1 created this revision.Aug 18 2020, 11:17 PM
joshua-arch1 requested review of this revision.Aug 18 2020, 11:17 PM
joshua-arch1 added a reviewer: lenary.

Hi Jun:

GCC part should review at gcc-patch mailing list instead of here, for other files (sanitizer_platform.h, sanitizer_common.h and sanitizer_symbolizer_libcdep.cpp), it's right place to review, but it should using the diff which generated within LLVM source-tree, and GCC will sync libsanitizer after LLVM is accepted.

libsanitizer/sanitizer_common/sanitizer_common.h
675 ↗(On Diff #286483)

Seems like you also need to update asan_symbolize.py according this comment, don't remove that I know it not exist in gcc source tree, but LLVM source tree have that[1].

[1] https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/asan/scripts/asan_symbolize.py

libsanitizer/sanitizer_common/sanitizer_platform.h
221 ↗(On Diff #286483)

This marco is not necessary to define if you don't use anywhere.

libsanitizer/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
252 ↗(On Diff #286483)

Same as above, don't remove this.

Hi Jun:

GCC part should review at gcc-patch mailing list instead of here, for other files (sanitizer_platform.h, sanitizer_common.h and sanitizer_symbolizer_libcdep.cpp), it's right place to review, but it should using the diff which generated within LLVM source-tree, and GCC will sync libsanitizer after LLVM is accepted.

What about config/riscv/riscv.c? Should I place it here to review?

Hi Jun:

GCC part should review at gcc-patch mailing list instead of here, for other files (sanitizer_platform.h, sanitizer_common.h and sanitizer_symbolizer_libcdep.cpp), it's right place to review, but it should using the diff which generated within LLVM source-tree, and GCC will sync libsanitizer after LLVM is accepted.

What about config/riscv/riscv.c? Should I place it here to review?

For config/riscv/riscv.c, it's part of GCC source tree, so there is wrong place to review :P

This comment was removed by joshua-arch1.

can you generate the diff with full context?

git format-patch -U99999

  1. to remove unnecessary definitions
  2. to generate the diff with full context
  3. to generate the diff on LLVM source tree

can you generate the diff with full context?

git format-patch -U99999

Hi I have updated the patch. Now it has full context.

joshua-arch1 added a comment.EditedAug 20 2020, 2:51 AM

Hi Jun:

GCC part should review at gcc-patch mailing list instead of here, for other files (sanitizer_platform.h, sanitizer_common.h and sanitizer_symbolizer_libcdep.cpp), it's right place to review, but it should using the diff which generated within LLVM source-tree, and GCC will sync libsanitizer after LLVM is accepted.

What about config/riscv/riscv.c? Should I place it here to review?

For config/riscv/riscv.c, it's part of GCC source tree, so there is wrong place to review :P

I have updated the patch based on LLVM source tree. When will it be reviewed?

joshua-arch1 edited the summary of this revision. (Show Details)
joshua-arch1 edited reviewers, added: asb; removed: alex.Aug 23 2020, 7:00 PM
luismarques requested changes to this revision.Aug 28 2020, 7:23 AM
luismarques added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
273–274

The preprocessor definition should be __riscv, and the arch riscv32 or riscv64. Given that this is incorrect, I suspect this patch needs much more testing. When you update the patch please update the description with more details of why you think this is in good shape to be merged. Adding more tests would make that easier.

This revision now requires changes to proceed.Aug 28 2020, 7:23 AM
joshua-arch1 edited the summary of this revision. (Show Details)
joshua-arch1 edited the summary of this revision. (Show Details)Sep 13 2020, 11:44 PM
joshua-arch1 edited the summary of this revision. (Show Details)
joshua-arch1 added inline comments.Sep 14 2020, 11:02 PM
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
273–274

The preprocessor definition should be __riscv, and the arch riscv32 or riscv64. Given that this is incorrect, I suspect this patch needs much more testing. When you update the patch please update the description with more details of why you think this is in good shape to be merged. Adding more tests would make that easier.

Hi, I have updated my patch and its summary.

joshua-arch1 marked an inline comment as not done.Mon, Sep 21, 10:34 PM
joshua-arch1 marked an inline comment as done.

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