ThreadSanitizer support for Risc-V #12907

dustanddreams · 2024-01-17T06:32:00Z

This adds TSan support for Linux/riscv64.

Risc-V support in TSan is quite recent - in order to use it, you need to build gcc from sources after 20231218, when TSan support was enabled on this platform.

Support is also enabled for clang in the hope than clang will eventually be able to produce a working OCaml compiler on this platform (this is currently not the case, regardless of whether TSan is enabled or not).

All tests pass but tests/regression/pr9853/compaction_corner_case.ml which times out (running the binary manually, it takes a bit more than 14 minutes to complete successfully).

gasche · 2024-01-17T06:58:34Z

Note: what the compaction_corner_case does is to run Gc.compact 25_000 times, with an almost-empty heap (from 0 to 3*25_000 words allocated).

dustanddreams · 2024-01-17T07:01:16Z

This only shows that the overhead of TSan (at least on this platform) is quite high.

gasche · 2024-01-17T07:13:47Z

I would propose adding a no-tsan qualifier on this test to avoid the slow runtime. (I think there is more value in a reliable testsuite than in TSAN-checking a regression test that is a sequential program.)

If you look for no-tsan occurrences in the testsuite, the noble tradition is to always include a comment explaining why it is here. In this case I think you could use the same comment as testsuite/tests/lib-marshal/intext_par.ml

dustanddreams · 2024-01-17T07:18:08Z

Sure, done.

dustanddreams · 2024-01-17T09:07:06Z

I forgot to mention that these changes are very similar to the arm64 PR merged some time ago - the logic is completely similar, and so are the minor platform-specific tweaks required.

gasche · 2024-01-17T10:41:34Z

I wonder who would be interested in reviewing this. @nojb is the original author of the riscv backend, are there other riscv enthusiasts out there?

nojb · 2024-01-17T10:45:21Z

Unfortunately, I know nothing about TSan. If there is a specification or some documentation of what TSam instrumentation is supposed to do, I can take a look, but without any guide it would probably be futile for me to do so :)

gasche · 2024-01-17T10:52:55Z

I think we could ask the TSan band (@fabbing and @OlivierNicole) to look at the TSan aspects, and have someone familiar with riscv review the code for readability, consistency of style with the rest of the backend, and verify (should be easy) that this does not change behavior when TSan is not enabled.

dustanddreams · 2024-01-17T10:52:59Z

Unfortunately, I know nothing about TSan. If there is a specification or some documentation of what TSam instrumentation is supposed to do, I can take a look, but without any guide it would probably be futile for me to do so :)

There is some information in manual/src/cmds/tsan.etex, and the the machine-dependent work consists of adapting the runtime/amd64.S or runtime/arm64.S changes to the new platform. (which is why I pointed at arm64, as the changes have been done in a very similar way)

nojb · 2024-01-17T10:56:33Z

I think we could ask the TSan band (@fabbing and @OlivierNicole) to look at the TSan aspects, and have someone familiar with riscv review the code for readability, consistency of style with the rest of the backend, and verify (should be easy) that this does not change behavior when TSan is not enabled.

Makes sense, I'll do the riscv part.

Unfortunately, I know nothing about TSan. If there is a specification or some documentation of what TSam instrumentation is supposed to do, I can take a look, but without any guide it would probably be futile for me to do so :)

There is some information in manual/src/cmds/tsan.etex, and the the machine-dependent work consists of adapting the runtime/amd64.S or runtime/arm64.S changes to the new platform. (which is why I pointed at arm64, as the changes have been done in a very similar way)

Thanks for the pointers.

OlivierNicole · 2024-01-18T16:38:37Z

Naive question: do you know when RISC-V will be officially supported by ThreadSanitizer? It’s not yet listed on the LLVM documentation page.

dustanddreams · 2024-01-18T16:45:35Z

Naive question: do you know when RISC-V will be officially supported by ThreadSanitizer? It’s not yet listed on the LLVM documentation page.

The documentation is out-of-date, as usual! If you follow the link to the wiki at the end of that page, the "supported platforms" section - which is also out-of-date - points to the latest version of tsan_platform.h in the official repository. In this file you can see that mips64, riscv64 and s390x support are also available.

Gcc has enabled support for riscv64 early this year and for s390x 2.5 years ago.

nojb

LGTM

Nit: one could use the existing macros C_ARG_{1,2,3,4} instead of a0,a1,a2,a3 when calling C functions, but this would only make sense if the same change is made to the arm64 backend in order to maintain symmetry.

dustanddreams · 2024-01-22T06:39:31Z

Nit: one could use the existing macros C_ARG_{1,2,3,4} instead of a0,a1,a2,a3 when calling C functions, but this would only make sense if the same change is made to the arm64 backend in order to maintain symmetry.

As I mentioned in the arm64 PR, these C_ARG_# macro make sense for amd64 where the runtime code may be used with different ABIs, but this is usually not the case for RISC platforms, and on mips-derivatives such as riscv, the friendly names (a0-a7 instead of x10-x17, s0-s11 instead of x8-x9 and x18-x27, etc) make such macros completely unnecessary.

nojb · 2024-01-22T06:43:44Z

Nit: one could use the existing macros C_ARG_{1,2,3,4} instead of a0,a1,a2,a3 when calling C functions, but this would only make sense if the same change is made to the arm64 backend in order to maintain symmetry.

As I mentioned in the arm64 PR, these C_ARG_# macro make sense for amd64 where the runtime code may be used with different ABIs, but this is usually not the case for RISC platforms, and on mips-derivatives such as riscv, the friendly names (a0-a7 instead of x10-x17, s0-s11 instead of x8-x9 and x18-x27, etc) make such macros completely unnecessary.

Makes sense, thanks.

dra27 · 2024-01-24T10:00:33Z

(added to 5.2 milestone for discussion later)

nojb · 2024-01-27T05:50:53Z

This looks ready to merge in my opinion. @OlivierNicole: are you intending to take a look? Otherwise I think we can move to merge.

OlivierNicole · 2024-01-27T11:11:31Z

I believe @fabbing is having a look?

fabbing · 2024-01-29T08:25:54Z

I believe @fabbing is having a look?

Yes, I am!

fabbing

Apart from the few suggested changes, it looks good to me. Note that I'm new to RISC-V assembly, so I may have missed some things...
I like the addition of the TSAN_C_CALL macro, we could probably improve amd64.S with that idea!

I also suggest updating the comment in configure.ac:

diff --git a/configure.ac b/configure.ac
index 741553a06d..f442a2de60 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1663,8 +1663,8 @@ AS_IF([test "x$enable_instrumented_runtime" != "xno" ],
     )]
 )
 
-# ThreadSanitizer support is only for Linux/FreeBSD/macOS on amd64, as well as
-# Linux/macOS on arm64.
+# ThreadSanitizer supported for Linux/FreeBSD/macOS on amd64, Linux/macOS on
+# arm64, and Linux on RISC-V.
 # ThreadSanitizer supports more architectures but the OCaml client side is not
 # implemented (yet).
 AS_IF([test "x$enable_tsan" = "xyes" ],

fabbing · 2024-01-29T13:56:19Z

runtime/riscv.S

+       or fa0:fa1. */
+        addi    sp, sp, -32
+        CFI_ADJUST(32)
+        sd      a0, 0(sp)


Why save only a0 (and not a1), but both fa0 and fa1?

caml_c_call wraps any kind of C function, and needs to preserve the returned value during the TSan instrumentation.

On RiscV, the ABI requires that integer values are returned into a0, or a0:a1 if the return value does not fit in one register, and that floating-point values are similarly returned in fa0 or fa0:fa1.

On 64-bit RiscV, until C grows a 128-bit integer type, all integer types will fit in a single register, so we only need to save a0. However, a long double return type would span both fa0 and fa1, so we save them both.

I was checking with @OlivierNicole our choice not to save xmm1 in amd64.S: the reasoning was that the compatible returned type of the C function called by OCaml would be only be float (double in C) or int32/int64/nativeint (all fitting in 64bits register), so we don't bother saving rdx and xmm1. See the table in https://v2.ocaml.org/manual/intfc.html#s:C-cheaper-call for reference.
Do you agree with this, and if so, pehaps we shouldn't save fa1 either to be consistent with amd64.S.

Given that limited list of supported types, I agree that only one floating-point register needs to be saved here. Since the arm64 code is already in and saves two, I intend to clean this in a later PR once all TSan support PRs are in (since all of them save two floating-point registers here), if that's ok with you.

I'm not sure if it will make a big difference in terms of performance, but I'm glad we agree on this: at least it's not a bug in amd64.S!
And yes, it's perfectly fine to do this in another PR.

runtime/riscv.S

It takes more than 14 minutes to run with tsan on riscv64.

dustanddreams · 2024-01-29T16:22:50Z

Rebased, updated configure.ac comments.

nojb · 2024-01-30T08:45:51Z

@fabbing can you give a formal approval if you are happy with the current state of the PR? We can then move to merge it. Thanks!

fabbing

Apart from the discussion about possibly avoiding saving a register in caml_c_call, which shouldn't affect correctness (and could could be part of another PR if deemed useful) this looks fine to me.

* Implement Ireturn_addr, needed by TSan instrumentation * TSan riscv bits for runtime * Allow for TSan to be enabled on Linux/riscv64 * Mark this test no-tsan. It takes more than 14 minutes to run with tsan on riscv64. * Document changes (cherry picked from commit 993a7e8)

nojb · 2024-02-01T06:44:40Z

Cherry-picked to 5.2: dc41ba9

dra27 added the thread-sanitizer Related to data races, thread sanitizer label Jan 17, 2024

nojb approved these changes Jan 20, 2024

View reviewed changes

dustanddreams force-pushed the riscv64_tsan branch from 327a588 to f9536c3 Compare January 22, 2024 06:42

dra27 added this to the 5.2 milestone Jan 24, 2024

OlivierNicole mentioned this pull request Jan 24, 2024

Fix data races between marking and verify_pool #12939

Closed

fabbing suggested changes Jan 29, 2024

View reviewed changes

dustanddreams added 5 commits January 29, 2024 16:21

Implement Ireturn_addr, needed by TSan instrumentation

bdff5d7

TSan riscv bits for runtime

468288f

Allow for TSan to be enabled on Linux/riscv64

cc6ec73

Mark this test no-tsan.

42c994b

It takes more than 14 minutes to run with tsan on riscv64.

Document changes

bc12d52

dustanddreams force-pushed the riscv64_tsan branch from f9536c3 to bc12d52 Compare January 29, 2024 16:22

fabbing approved these changes Jan 30, 2024

View reviewed changes

Merge branch 'trunk' into riscv64_tsan

8ec217d

nojb added the merge-me label Feb 1, 2024

nojb merged commit 993a7e8 into ocaml:trunk Feb 1, 2024
13 of 14 checks passed

dustanddreams deleted the riscv64_tsan branch February 1, 2024 06:44

OlivierNicole mentioned this pull request Feb 2, 2024

Fix data race between marking and sweeping #12934

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ThreadSanitizer support for Risc-V #12907

ThreadSanitizer support for Risc-V #12907

dustanddreams commented Jan 17, 2024

gasche commented Jan 17, 2024

dustanddreams commented Jan 17, 2024

gasche commented Jan 17, 2024

dustanddreams commented Jan 17, 2024

dustanddreams commented Jan 17, 2024

gasche commented Jan 17, 2024

nojb commented Jan 17, 2024

gasche commented Jan 17, 2024

dustanddreams commented Jan 17, 2024

nojb commented Jan 17, 2024

OlivierNicole commented Jan 18, 2024

dustanddreams commented Jan 18, 2024

nojb left a comment

dustanddreams commented Jan 22, 2024

nojb commented Jan 22, 2024

dra27 commented Jan 24, 2024

nojb commented Jan 27, 2024

OlivierNicole commented Jan 27, 2024

fabbing commented Jan 29, 2024

fabbing left a comment

fabbing Jan 29, 2024

dustanddreams Jan 29, 2024

fabbing Jan 30, 2024

dustanddreams Jan 31, 2024

fabbing Jan 31, 2024

dustanddreams commented Jan 29, 2024

nojb commented Jan 30, 2024

fabbing left a comment

nojb commented Feb 1, 2024

ThreadSanitizer support for Risc-V #12907

ThreadSanitizer support for Risc-V #12907

Conversation

dustanddreams commented Jan 17, 2024

gasche commented Jan 17, 2024

dustanddreams commented Jan 17, 2024

gasche commented Jan 17, 2024

dustanddreams commented Jan 17, 2024

dustanddreams commented Jan 17, 2024

gasche commented Jan 17, 2024

nojb commented Jan 17, 2024

gasche commented Jan 17, 2024

dustanddreams commented Jan 17, 2024

nojb commented Jan 17, 2024

OlivierNicole commented Jan 18, 2024

dustanddreams commented Jan 18, 2024

nojb left a comment

Choose a reason for hiding this comment

dustanddreams commented Jan 22, 2024

nojb commented Jan 22, 2024

dra27 commented Jan 24, 2024

nojb commented Jan 27, 2024

OlivierNicole commented Jan 27, 2024

fabbing commented Jan 29, 2024

fabbing left a comment

Choose a reason for hiding this comment

fabbing Jan 29, 2024

Choose a reason for hiding this comment

dustanddreams Jan 29, 2024

Choose a reason for hiding this comment

fabbing Jan 30, 2024

Choose a reason for hiding this comment

dustanddreams Jan 31, 2024

Choose a reason for hiding this comment

fabbing Jan 31, 2024

Choose a reason for hiding this comment

dustanddreams commented Jan 29, 2024

nojb commented Jan 30, 2024

fabbing left a comment

Choose a reason for hiding this comment

nojb commented Feb 1, 2024