Added initial version of thread.c #239

JCash · 2022-09-09T15:41:04Z

No description provided.

JCash · 2022-09-09T15:41:25Z

Compiles with

clang -std=c11 -DRMT_ENABLED=1 -DRMT_ARCH_64BIT -DRMT_USE_C_ATOMICS -g -O0 -fsanitize=thread -Ilib sample/thread.c lib/Remotery.c -o build/test && ./build/test

dwilliamson · 2022-09-12T19:14:51Z

So just to be clear: this defeats all your TSAN warnings, right? I will comb through this tomorrow but given we don't have load/acquire and store/release memory fences on non-x64/86 platforms, I feel this is only just the beginning...

lib/Remotery.c

JCash · 2022-09-13T06:58:00Z

lib/Remotery.c

+#if __STDC_VERSION__ >= 201112L || __cplusplus >= 199711L
+    #if !defined(__STDC_NO_ATOMICS__)
+        #if !defined(RMT_USE_C_ATOMICS) // Check if the user already specified it
+            #define RMT_USE_C_ATOMICS
+        #endif
+    #endif
+#endif


Do we want a way to explicitly choose not to use the C atomics?

I guess we do because we want to have an explicit test case that ensures that path always compiles.

lib/Remotery.c

…rt may be spotty

lib/Remotery.c

JCash · 2022-09-13T11:30:51Z

On linux, I still get this TSAN warning, when using the C11 atomics:

  Read of size 4 at 0x0000037b903c by thread T4:
    #0 ThreadProfiler_GetNameHash /home/builder/lib/Remotery.c:5336:21 (test+0x4be8e8)
    #1 _rmt_BeginCPUSample /home/builder/lib/Remotery.c:7378:28 (test+0x4be763)
    #2 recursiveFunction /home/builder/sample/thread.c:102:5 (test+0x4bb567)
    #3 Run /home/builder/sample/thread.c:116:13 (test+0x4bb491)
    #4 thread_start_proxy /home/builder/sample/thread.c:36:5 (test+0x4bb1c9)

  Previous write of size 4 at 0x0000037b903c by thread T3:
    #0 ThreadProfiler_GetNameHash /home/builder/lib/Remotery.c:5346:29 (test+0x4be98f)
    #1 _rmt_BeginCPUSample /home/builder/lib/Remotery.c:7378:28 (test+0x4be763)
    #2 recursiveFunction /home/builder/sample/thread.c:102:5 (test+0x4bb567)
    #3 Run /home/builder/sample/thread.c:116:13 (test+0x4bb491)
    #4 thread_start_proxy /home/builder/sample/thread.c:36:5 (test+0x4bb1c9)

  Location is global 'recursiveFunction.rmt_sample_hash_recursive' of size 4 at 0x0000037b903c (test+0x0000037b903c)

  Thread T4 'thread_1' (tid=17, running) created by main thread at:
    #0 pthread_create /local/mnt/workspace/bcain_clang_hu-bcain-lv_8872/final/llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:977:3 (test+0x4476dd)
    #1 thread_create /home/builder/sample/thread.c:70:11 (test+0x4bb0a3)
    #2 main /home/builder/sample/thread.c:147:22 (test+0x4bb37d)

  Thread T3 'thread_0' (tid=16, running) created by main thread at:
    #0 pthread_create /local/mnt/workspace/bcain_clang_hu-bcain-lv_8872/final/llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:977:3 (test+0x4476dd)
    #1 thread_create /home/builder/sample/thread.c:70:11 (test+0x4bb0a3)
    #2 main /home/builder/sample/thread.c:147:22 (test+0x4bb37d)

And, I'm not sure what's going on with the Windows "dump.exe". I need to look into that as well.

dwilliamson · 2022-09-13T11:45:02Z

Your line numbers are different to the version I have so I'm not sure what it's referencing.

At a guess I'd say it's the hash_cache read/write. Which makes sense: multiple threads will be reading and writing the cached hash value. However it's completely harmless as the value is either zero or the same calculated hash value across all threads.

The side effects are:

The string gets added to the string table more than once (which it can safely ignore).
The expensive path of calculating the hash happens more than once, which again is only once per thread in contention.

Options:

Expose atomic types outside the implementation and use a rmtAtomicU32 pointer, but I really don't like that.
Can we cast to rmtAtomicU32?
Put the whole thing in a CAS loop to ensure 100% validity.
Can we comment the code in a way that TSAN classifies it as a false positive?

JCash · 2022-09-13T14:19:28Z

lib/Remotery.c

@@ -5333,7 +5359,7 @@ static rmtU32 ThreadProfiler_GetNameHash(ThreadProfiler* thread_profiler, rmtMes
    if (hash_cache != NULL)
    {
        // Calculate the hash first time round only
-        name_hash = *hash_cache;
+        name_hash = AtomicLoadU32((rmtAtomicU32*)hash_cache);


Tried some variants but so far, this was the one that worked and wasn't too intrusive.

lib/Remotery.c

JCash · 2022-09-13T14:45:36Z

I think it's all coming together now.

However, I haven't figured out why the windows test stopped working.
It builds and runs fine locally on my windows machine.

JCash · 2022-09-13T14:46:29Z

lib/Remotery.c

@@ -167,6 +167,28 @@ static rmtBool g_SettingsInitialized = RMT_FALSE;
    #include <cuda.h>
 #endif

+#if RMT_USE_LEGACY_ATOMICS==0
+    #if !defined(RMT_PLATFORM_WINDOWS)


Hopefully can enable this once the windows tests start working again.

lib/Remotery.c

lib/Remotery.h

…-sample

JCash · 2022-09-14T06:50:13Z

.github/scripts/compile.bat

@@ -55,6 +55,7 @@ cl.exe %CL_OPTS% -Ilib -DRMT_USE_INTERNAL_HASH_FUNCTION=1 sample/sample.c lib/Re
 echo "Samples"
 cl.exe %CL_OPTS% -Ilib sample/sample.c lib/Remotery.c /link /out:build\sample.exe || goto :error
 cl.exe %CL_OPTS% -Ilib sample/dump.c lib/Remotery.c /link /out:build\dump.exe || goto :error
+cl.exe %CL_OPTS% -TP /MTd /O1 /fsanitize=address /Zi -Ilib sample/thread.c lib/Remotery.c /link /DEBUG /out:build\thread_asan.exe || goto :error


Trying out address sanitizer for windows

dwilliamson · 2022-09-14T11:11:40Z

Is this ready to go? What is the status of running ASAN/UBSAN/TSAN after this change?

JCash · 2022-09-14T11:47:27Z

I think it's good to go.
I have no asan/tsan/ubsan issues locally, and the linux/darwin tests produce no warnings either. And the windows asan test is green as well.

dwilliamson · 2022-09-14T12:25:08Z

Awesome work, thanks!

Added initial version of thread.c

20f294a

This was referenced Sep 9, 2022

Thread Sanitizer data race issues. #240

Closed

thread sanitizer warnings #195

Closed

JCash added 2 commits September 10, 2022 09:31

Fix thread name

6692be7

Use stdatomic on compilers that support it

827e503

JCash mentioned this pull request Sep 12, 2022

fix TSAN warnings on macOS #238

Closed

Compile fixes

4bdafa3

dwilliamson reviewed Sep 12, 2022

View reviewed changes

lib/Remotery.c Outdated Show resolved Hide resolved

lib/Remotery.c Outdated Show resolved Hide resolved

lib/Remotery.c Show resolved Hide resolved

JCash added 3 commits September 13, 2022 08:41

Added support for c atomics for c+11

fd4f8e9

Added rmtAtomicPtr define to wrap pointer types

c90db43

Changed request_exit back to a bool

251569b

JCash commented Sep 13, 2022

View reviewed changes

JCash added 3 commits September 13, 2022 09:41

bool fix

ad69437

Fix warning

5c8baa1

Let user define the RMT_USE_C_ATOMICS manually, as the compiler suppo…

c6d4286

…rt may be spotty

JCash commented Sep 13, 2022

View reviewed changes

lib/Remotery.c Outdated Show resolved Hide resolved

Trying std::atomic for C++ compilation

412d085

JCash commented Sep 13, 2022

View reviewed changes

lib/Remotery.c Outdated Show resolved Hide resolved

cleanup

c02afae

JCash commented Sep 13, 2022

View reviewed changes

lib/Remotery.c Show resolved Hide resolved

JCash added 2 commits September 13, 2022 11:54

Disable C atomic autodetection for windows (since the tests failed)

6776c4c

Use std::atomic explicitly when built with c++

514caf2

dwilliamson reviewed Sep 13, 2022

View reviewed changes

lib/Remotery.c Show resolved Hide resolved

JCash added 5 commits September 13, 2022 12:23

Disabled windows + c/C++ atomics again

1fe1670

The C++ api requires a reference

6c4609b

Review fix: renamed defines

c404d00

Compile issue for linux

ac9ae4e

Added way to manually fallback to legacy atomics

78f0437

JCash added 2 commits September 13, 2022 16:08

Made the tsan warning go away for getnamehash function

f4e443d

Added thread sample to unit tests with tsan, ubsan and asan

460b74e

JCash commented Sep 13, 2022

View reviewed changes

lib/Remotery.c Show resolved Hide resolved

JCash added 2 commits September 13, 2022 16:25

test fix

5b05dbf

removed thread.c form building on win32

d75b1a4

JCash commented Sep 13, 2022

View reviewed changes

lib/Remotery.c Show resolved Hide resolved

JCash commented Sep 13, 2022

View reviewed changes

lib/Remotery.h Show resolved Hide resolved

JCash added 6 commits September 13, 2022 17:24

Added thread test + asan for win32

4bd1f42

cleanup

803a916

Reenabled usage of c++ atomics if for Win32

b60d3e0

Merge branch 'thread-sample' of github.com:JCash/Remotery into thread…

b8b3c13

…-sample

testing linker fix

8fc3d06

testing more flags

ab80dfc

JCash commented Sep 14, 2022

View reviewed changes

JCash marked this pull request as ready for review September 14, 2022 07:36

dwilliamson merged commit 0cc955e into Celtoys:main Sep 14, 2022

JCash deleted the thread-sample branch September 14, 2022 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added initial version of thread.c #239

Added initial version of thread.c #239

JCash commented Sep 9, 2022

JCash commented Sep 9, 2022 •

edited

Loading

dwilliamson commented Sep 12, 2022

JCash Sep 13, 2022

dwilliamson Sep 13, 2022

JCash commented Sep 13, 2022

dwilliamson commented Sep 13, 2022

JCash Sep 13, 2022

JCash commented Sep 13, 2022

JCash Sep 13, 2022

JCash Sep 14, 2022

dwilliamson commented Sep 14, 2022

JCash commented Sep 14, 2022

dwilliamson commented Sep 14, 2022

Added initial version of thread.c #239

Added initial version of thread.c #239

Conversation

JCash commented Sep 9, 2022

JCash commented Sep 9, 2022 • edited Loading

dwilliamson commented Sep 12, 2022

JCash Sep 13, 2022

Choose a reason for hiding this comment

dwilliamson Sep 13, 2022

Choose a reason for hiding this comment

JCash commented Sep 13, 2022

dwilliamson commented Sep 13, 2022

JCash Sep 13, 2022

Choose a reason for hiding this comment

JCash commented Sep 13, 2022

JCash Sep 13, 2022

Choose a reason for hiding this comment

JCash Sep 14, 2022

Choose a reason for hiding this comment

dwilliamson commented Sep 14, 2022

JCash commented Sep 14, 2022

dwilliamson commented Sep 14, 2022

JCash commented Sep 9, 2022 •

edited

Loading