Page MenuHomePhabricator

[sanitizer][LoongArch] Port address sanitizer to LoongArch
ClosedPublic

Authored by xry111 on Jul 9 2022, 12:22 AM.

Details

Summary

Depends on D129371.

It survived all GCC ASan tests.

Changes are trivial and mostly "borrowed" RISC-V logics, except that a different SHADOW_OFFSET is used.

Diff Detail

Event Timeline

xry111 created this revision.Jul 9 2022, 12:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2022, 12:22 AM
xry111 requested review of this revision.Jul 9 2022, 12:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2022, 12:22 AM
xry111 added inline comments.Jul 9 2022, 12:24 AM
compiler-rt/lib/asan/asan_mapping.h
117

47-bit VMA (16-KB pages with 3-level page table) is the default of Linux Kconfig for LoongArch. Is there any way to support other VMA sizes alongside 47-bit?

xry111 updated this revision to Diff 443421.Jul 9 2022, 12:37 AM

Remove unnecessary brackets.

xry111 marked an inline comment as not done.Jul 9 2022, 12:38 AM
SixWeining added a subscriber: XiaodongLoong.

Add @XiaodongLoong who ever ported compiler-rt to LoongArch on LLVM8 in the old world where the VMA is 40-bit. Thanks.

compiler-rt/lib/asan/asan_mapping.h
117

Is the VMA 47-bit or 48-bit? I remember that it’s 48. Maybe I’m wrong?

xry111 added inline comments.Jul 9 2022, 1:03 AM
compiler-rt/lib/asan/asan_mapping.h
117

The default is 47-bit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/loongarch/Kconfig#n187

It can be configured to 36, 39, 42, 48, or 55 if desired. But AFAIK 48 and 55 are not suitable for desktop distributions because Firefox does not support any VMA size >= 48.

xry111 added inline comments.Jul 10 2022, 9:09 AM
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_vfork_loongarch64.inc.S
27

Shall this be la.global or la.local? I'm not sure about the precise definition of "global" or "local" symbols in the context of LoongArch assembly. And we don't have a manual explaining LoongArch pseudo assembly operations yet...

xry111 added inline comments.Jul 10 2022, 8:05 PM
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_vfork_loongarch64.inc.S
27

My experiment shows it seems depending on if the symbol is DSO local (__attribute__((visibility("hidden")))). For __interception::real_vfork it's DSO local so la.local is better.

A new revision incoming...

xry111 updated this revision to Diff 443546.Jul 10 2022, 8:11 PM

Mark __interceptor::real_vfork with ASM_HIDDEN and use la.local for it.

Add %plt for COMMON_INTERCEPTOR_HANDLE_VFORK because it's not DSO local and the linker warns about using calling it w/o PLT.

xry111 marked an inline comment as done.Jul 10 2022, 8:11 PM
xen0n added a comment.Jul 10 2022, 9:09 PM

LGTM besides two extremely minor nits.

compiler-rt/lib/asan/asan_interceptors_vfork.S
10

The list seems to be alphabetized. Insert it before the riscv64 line?

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp
124

Similarly, put loongarch before riscv? Although there are only 2 cases at present, future arches (if any) will likely fall into this category too.

xry111 updated this revision to Diff 443557.Jul 10 2022, 9:30 PM

Keep lexicographic order

xry111 marked 2 inline comments as done.Jul 10 2022, 9:30 PM

Could you share some detail about how to build and test llvm's sanitizer with GCC's testcase? Thanks. Previouly in llvm8 we only test it with clang itself.

compiler-rt/lib/asan/asan_mapping.h
117

Yes. It is 47-bit. The sum of Kernel and User is 48-bit.

xry111 added a comment.EditedJul 11 2022, 4:11 AM

Could you share some detail about how to build and test llvm's sanitizer with GCC's testcase? Thanks. Previouly in llvm8 we only test it with clang itself.

I've rebased and applied the two diffs for GCC. And for ASan, define TARGET_ASAN_SHADOW_OFFSET to return the shadow offset (1 << 46).

The changes are available at https://github.com/xry111/gcc/tree/me/loongarch-sanitizer-test.

Build it with /path/to/gcc/configure --prefix=/home/xry111/gcc-trunk --disable-multilib --with-system-zlib --enable-libsanitizer --disable-bootstrap, then make. --enable-libsanitizer is necessary because libsanitizer is not enabled by default by GCC building system for LoongArch target. --disable-bootstrap saves some time, but if the system compiler is affected by https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106096 you'll need --enable-bootstrap.

Run UBSan tests with make check-gcc RUNTESTFLAGS=ubsan.exp.

Run ASan tests with make check-gcc RUNTESTFLAGS=asan.exp.

xry111 updated this revision to Diff 443597.Jul 11 2022, 4:19 AM

Use full diff

SixWeining accepted this revision.Jul 13 2022, 5:36 AM

Could you share some detail about how to build and test llvm's sanitizer with GCC's testcase? Thanks. Previouly in llvm8 we only test it with clang itself.

I've rebased and applied the two diffs for GCC. And for ASan, define TARGET_ASAN_SHADOW_OFFSET to return the shadow offset (1 << 46).

The changes are available at https://github.com/xry111/gcc/tree/me/loongarch-sanitizer-test.

Build it with /path/to/gcc/configure --prefix=/home/xry111/gcc-trunk --disable-multilib --with-system-zlib --enable-libsanitizer --disable-bootstrap, then make. --enable-libsanitizer is necessary because libsanitizer is not enabled by default by GCC building system for LoongArch target. --disable-bootstrap saves some time, but if the system compiler is affected by https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106096 you'll need --enable-bootstrap.

Run UBSan tests with make check-gcc RUNTESTFLAGS=ubsan.exp.

Run ASan tests with make check-gcc RUNTESTFLAGS=asan.exp.

LGTM. Thanks.

Seems that currently we can only check the changes by GCC because LoongArch hasn’t been supported by clang.

Not sure whether this change could land. What do others think?

compiler-rt/lib/asan/asan_mapping.h
117

I’m not sure is there any way to support other VMA sizes alongside 47-bit. But I think other architectures may have the same problem as LoongArch.

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_vfork_loongarch64.inc.S
2

Seems “defined(loongarch) && ” can be removed.

This revision is now accepted and ready to land.Jul 13 2022, 5:36 AM

Seems that currently we can only check the changes by GCC because LoongArch hasn’t been supported by clang.

Not sure whether this change could land. What do others think?

+vitalybuka for this question.

xry111 updated this revision to Diff 444497.Jul 13 2022, 7:43 PM

run git-clang-format

MaskRay added inline comments.Jul 14 2022, 11:24 AM
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_vfork_loongarch64.inc.S
9

Does this need to use a common symbol? Consider a regular .type ..., @object in .bss

MaskRay accepted this revision.Jul 14 2022, 2:09 PM
xry111 updated this revision to Diff 444927.Jul 15 2022, 3:18 AM

Define __interception::real_vfork as a normal bss object, instead of common symbol.

xry111 updated this revision to Diff 444930.Jul 15 2022, 3:20 AM
xry111 marked an inline comment as done.

Remove unnecessary check for __loongarch__ before __loongarch_lp64.

xry111 marked 3 inline comments as done.Jul 15 2022, 3:23 AM

I don't have write access to rG, if you think these two changes are OK please commit it.

XiaodongLoong accepted this revision.Jul 19 2022, 5:38 AM
SixWeining accepted this revision.Jul 20 2022, 1:20 AM

LGTM.
As noted in D129371, I think this can land after nits fixed. Thanks.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp
124

Nit: uncessary indent.

This revision was automatically updated to reflect the committed changes.