Page MenuHomePhabricator

[tsan] Add tsan support for loongarch64
Needs ReviewPublic

Authored by tangyouling on Tue, Nov 22, 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.Tue, Nov 22, 4:00 AM
tangyouling requested review of this revision.Tue, Nov 22, 4:00 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptTue, Nov 22, 4:00 AM
Herald added subscribers: Restricted Project, cfe-commits, pcwang-thead. · View Herald Transcript
tangyouling added a reviewer: Restricted Project.Tue, Nov 22, 4:00 AM
$ make check-tsan -j4
Testing Time: 78.34s
  Unsupported      :  91
  Passed           : 345
  Expectedly Failed:   2
SixWeining added a comment.EditedTue, Nov 22, 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.Tue, Nov 22, 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.Tue, Nov 22, 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.Wed, Nov 23, 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.Thu, Nov 24, 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.Fri, Nov 25, 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.Fri, Nov 25, 5:01 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
1543

Make sense. Thanks.