Celtoys / Remotery

Single C file, Realtime CPU/GPU Profiler with Remote Web Viewer
Apache License 2.0
3.11k stars 264 forks source link

Added initial version of thread.c #239

Closed JCash closed 2 years ago

JCash commented 2 years ago

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 commented 2 years ago

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...

JCash commented 2 years ago

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 commented 2 years ago

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:

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

Options:

JCash commented 2 years ago

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.

dwilliamson commented 2 years ago

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

JCash commented 2 years ago

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 commented 2 years ago

Awesome work, thanks!