This is an archive of the discontinued LLVM Phabricator instance.

[tsan] Add tsan support for loongarch64
ClosedPublic

Authored by tangyouling on Nov 22 2022, 4:00 AM.

Details

Summary

This patch enabled tsan for loongarch64 with 47-bit VMA layout. All
tests are passing.

Also adds assembly routines to enable setjmp/longjmp for loongarch64
on linux.

Diff Detail

Event Timeline

tangyouling created this revision.Nov 22 2022, 4:00 AM
tangyouling requested review of this revision.Nov 22 2022, 4:00 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 22 2022, 4:00 AM
Herald added subscribers: Restricted Project, cfe-commits, pcwang-thead. · View Herald Transcript
tangyouling added a reviewer: Restricted Project.Nov 22 2022, 4:00 AM
$ make check-tsan -j4
Testing Time: 78.34s
  Unsupported      :  91
  Passed           : 345
  Expectedly Failed:   2
SixWeining added a comment.EditedNov 22 2022, 5:35 AM

I haven't check the correctness of tsan_rtl_loongarch64.S. Perhaps @XiaodongLoong has some inputs as I know he has implemented this previously.

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

I'm not sure if this is necessary. Maybe int res; is enough?

1543

Shall we list $t0-$t8 here? Ref D137396.

compiler-rt/lib/sanitizer_common/sanitizer_linux.h
80–82

May be it's better to keep original indention. Otherwise the paired #endif doesn't look good.

compiler-rt/test/sanitizer_common/print_address.h
12

Should be __loongarch64?

compiler-rt/test/tsan/mmap_large.cpp
20

Should be __loongarch64?

xry111 added inline comments.Nov 22 2022, 5:57 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.h
80–82

The problem is git clang-format sometimes insist you to change the indentation...

compiler-rt/test/sanitizer_common/print_address.h
12

__loongarch64 is deprecated, use __loongarch_lp64 instead.

MaskRay added inline comments.Nov 22 2022, 5:05 PM
compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
269

We should add new code in this fallback function. The generic code handles all new ports.

It's incorrect to assume that sizeof(pthread) stays 1936 as new glibc versions can add more fields.

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

I'm not sure if this is necessary. Maybe int res; is enough?

Referring to riscv64/aarch64, the existing definition is probably appropriate.
The return value res is stored directly in $a0, which may reduce the need to move the res value to $a0.

1543

Shall we list $t0-$t8 here? Ref D137396.

I will add the missing $t0-$t8.

compiler-rt/lib/sanitizer_common/sanitizer_linux.h
80–82

The problem is git clang-format sometimes insist you to change the indentation...

Yeah.

compiler-rt/test/sanitizer_common/print_address.h
12

__loongarch64 is deprecated, use __loongarch_lp64 instead.

Okay, use __loongarch_lp64 instead.

tangyouling added inline comments.Nov 23 2022, 6:43 PM
compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
269

We should add new code in this fallback function. The generic code handles all new ports.

It's incorrect to assume that sizeof(pthread) stays 1936 as new glibc versions can add more fields.

By looking at D119007, I can see that the val value obtained from _thread_db_sizeof_pthread is 1856 (glibc version 2.36).

Is it correct to change the ThreadDescriptorSizeFallback to the following,

#elif SANITIZER_LOONGARCH64
  val = 1856;
SixWeining added inline comments.Nov 24 2022, 6:33 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1543

As this inline asm is almost at the end of the function, it's sure $t* would not be used any more. So I think we could not add them to the clobber list. Just keep current approach.

  • Fixed the ThreadDescriptorSize calculation on loongarch64.
  • Use __loongarch_lp64 to distinguish loongarch64.
xry111 added inline comments.Nov 25 2022, 2:56 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1543

It's hard to tell: the compiler may decide to inline this function of perform some inter-procedural analysis. So IMO we should add t0-t8 here.

1543

It's hard to tell: the compiler may decide to inline this function of perform some inter-procedural analysis. So IMO we should add t0-t8 here.

"or perform", not "of perform".

SixWeining added inline comments.Nov 25 2022, 5:01 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1543

Make sense. Thanks.

Add the missing $t0-$t8 in internal_clone().

SixWeining accepted this revision.Dec 4 2022, 7:57 PM

LGTM from the LoongArch side but it's better to wait for others.

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
270

Better to add some comment about the glibc version?

This revision is now accepted and ready to land.Dec 4 2022, 7:57 PM

Add glibc version comments.

dvyukov accepted this revision.Dec 5 2022, 1:38 AM
dvyukov added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1526

Please use C-style one-line comments //

Use C-style one-line comments //.

This revision was landed with ongoing or failed builds.Dec 7 2022, 6:12 PM
This revision was automatically updated to reflect the committed changes.