Page MenuHomePhabricator

[sanitizer][LoongArch] Port sanitizer_common to LoongArch
ClosedPublic

Authored by xry111 on Jul 8 2022, 7:09 AM.

Details

Summary

Initial libsanitizer support for LoongArch. It survived all GCC UBSan tests.

Major changes:

  1. LoongArch port of Linux kernel only supports statx for stat and its families. So we need to add statx_to_stat and use it for stat-like libcalls. The logic is "borrowed" from Glibc.
  2. sanitizer_syscall_linux_loongarch64.inc is mostly duplicated from RISC-V port, as the syscall interface is almost same.

Diff Detail

Event Timeline

xry111 created this revision.Jul 8 2022, 7:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 7:09 AM
xry111 requested review of this revision.Jul 8 2022, 7:09 AM
Herald added subscribers: Restricted Project, pcwang-thead. · View Herald TranscriptJul 8 2022, 7:09 AM
xen0n added inline comments.Jul 8 2022, 7:31 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
280

Why not defined(__loongarch64)? I think the future ILP32 ABIs of LA64 will also have GRLEN=64, but the changes above seem to imply the LP64* ABIs.

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
273

nit: minimum

xry111 added inline comments.Jul 8 2022, 7:49 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
280

I'll change it to use __loongarch_lp64. The LoongArch Toolchain Conventions doc does not mention __loongarch64.

xry111 updated this revision to Diff 443257.Jul 8 2022, 8:31 AM

Use __loongarch_lp64 instead of __loongarch_grlen == 64 (as an ILP32 environment on LA64 CPU can also report __loongarch_grlen == 64).

__loongarch64 is not documented, and it may have same issue with __loongarch_grlen. On x86_64-linux-gnux32 and mips64-linux-gnuabin32 we also have __x86_64__ or __mips64 (the latter has already caused some problems in sanitizer). But __loongarch_lp64 is explicit enough, I believe.

Nit: minimal -> minimum.

xry111 marked an inline comment as done.Jul 8 2022, 8:32 AM
SixWeining added a subscriber: XiaodongLoong.

Add @XiaodongLoong who ever ported compiler-rt to LoongArch on LLVM8. Thanks.

It is better to upload diffs with full context (-U99999) to make it possible to browse the whole file.

// llvm/docs/Phabricator.rst
To get a full diff, use one of the following commands (or just use Arcanist
to upload your patch):

* ``git show HEAD -U999999 > mypatch.patch``
* ``git diff -U999999 @{u} > mypatch.patch``
* ``git diff HEAD~1 -U999999 > mypatch.patch``
xry111 updated this revision to Diff 443596.Jul 11 2022, 4:18 AM

Use full diff

vitalybuka accepted this revision.Jul 11 2022, 11:34 AM
vitalybuka added a subscriber: vitalybuka.

LGTM, If you want feedback from particular people please set them as "blocking reviewers"

This revision is now accepted and ready to land.Jul 11 2022, 11:34 AM
MaskRay added inline comments.Jul 11 2022, 1:38 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
280

Removing defined(__loongarch__) && looks good.

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_linux.cpp
67

Do newer kernel ports use another struct stat?

xry111 added inline comments.Jul 11 2022, 5:20 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
280

Will do.

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_linux.cpp
67

Initially I used "0" for struct_kernel_stat_sz, because the kernel does not provide struct stat anymore. At last it turn out we should use the size of struct stat in libc, not kernel.

I'll remove this #if pair.

But I'd say _kernel in the name struct_kernel_stat_sz is completely misleading, and it's [already causing misunderstandings][1]. Maybe we should rename struct_kernel_*_sz to struct_os_stat_sz or something later.

[1]: https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598096.html

xry111 updated this revision to Diff 443795.Jul 11 2022, 5:34 PM

Restore compiler check for sizeof(struct stat).

Simplify the definition of SANITIZER_LOONGARCH64.

xry111 marked 3 inline comments as done.Jul 11 2022, 5:35 PM
XiaodongLoong accepted this revision.Jul 13 2022, 1:00 AM

Thanks! LGTM.

SixWeining accepted this revision.Jul 13 2022, 4:59 AM

LGTM. Thanks.

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

Nit: tab -> spaces

414

Ditto.

vitalybuka added inline comments.Jul 13 2022, 10:45 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
414

for spaces, just keep whatever clang-format does:
git clang-format -f --extensions=inc,cc,h,c,cpp,rst HEAD^

xry111 added inline comments.Jul 13 2022, 7:12 PM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
382

They are spaces, but with a different indent than our standard I guess.

414

When I run this command, it changes the indent of some #if lines but the corresponding #else/#endif lines are not changed. Should I keep it as-is or manually fix those #else/#endif lines?

xry111 updated this revision to Diff 444494.Jul 13 2022, 7:23 PM

run git-clang-format

vitalybuka accepted this revision.Jul 13 2022, 8:21 PM
MaskRay accepted this revision.Jul 13 2022, 10:25 PM
MaskRay added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
273

llvm-project comments prefer one space after .

xry111 updated this revision to Diff 444926.Jul 15 2022, 3:16 AM

nit: use one space instead of two after ..

SixWeining accepted this revision.Jul 19 2022, 6:20 PM

LGTM.

LoongArch is a new port. Although clang hasn't support LoongArch, but I think this revision can land becuase it doesn't break other targets' build and @xry111 has checked GCC's UBSan tests. (We also double checked by same way as @xry111).

If there is objection, please let me know. Thanks.

MaskRay updated this revision to Diff 446060.Jul 20 2022, 12:48 AM

remove unneeded indentation

MaskRay updated this revision to Diff 446063.Jul 20 2022, 12:57 AM

reduce more indentation

This revision was landed with ongoing or failed builds.Jul 20 2022, 12:58 AM
This revision was automatically updated to reflect the committed changes.