The-OpenROAD-Project / OpenROAD

OpenROAD's unified application implementing an RTL-to-GDS Flow. Documentation at https://openroad.readthedocs.io/en/latest/
https://theopenroadproject.org/
BSD 3-Clause "New" or "Revised" License
1.38k stars 485 forks source link

gui: lock the updates to the TimingPathRenderer's fields #5106

Closed openroad-robot closed 2 weeks ago

openroad-robot commented 2 weeks ago

The main thread could get a key/mouse event that updates the renderer while it is drawing leading to a crash. This provides locking to avoid this.

Fixes #4990

github-actions[bot] commented 2 weeks ago

clang-tidy review says "All clean, LGTM! :+1:"

maliberty commented 2 weeks ago

good point - I'll fix that and then you can try it out. I can't make it crash but that doesn't mean much.

maliberty commented 2 weeks ago

@oharboe Please try it.

github-actions[bot] commented 2 weeks ago

clang-tidy review says "All clean, LGTM! :+1:"

QuantamHD commented 2 weeks ago

@oharboe You should try to build the GUI with TSAN https://clang.llvm.org/docs/ThreadSanitizer.html and see if you can find any other thread issues

oharboe commented 2 weeks ago

@oharboe You should try to build the GUI with TSAN https://clang.llvm.org/docs/ThreadSanitizer.html and see if you can find any other thread issues

Like so?

$ ./build_openroad.sh --local --latest --openroad-args "-DCMAKE_BUILD_TYPE=DEBUG -DCMAKE_CXX_FLAGS=-fsanitize=thread" 
[INFO FLW-0027] Saving logs to build_openroad.log
[INFO FLW-0028] ./build_openroad.sh --local --latest --openroad-args -DCMAKE_BUILD_TYPE=DEBUG -DCMAKE_CXX_FLAGS=-fsanitize=thread
[INFO FLW-0002] Updating git submodules.
Submodule path 'tools/OpenROAD': checked out '3612ee88d2343255851eeaa4fd0d4895fe8489a1'
[INFO FLW-0004] Updating OpenROAD app to the HEAD  of origin/master.
Previous HEAD position was 3612ee88d Merge pull request #5092 from The-OpenROAD-Project-staging/TR-fix-gc-surgicalFix
HEAD is now at a515fc6cc Merge pull request #5104 from The-OpenROAD-Project-staging/dpl-hard-blockages
From https://github.com/The-OpenROAD-Project/OpenROAD
 * branch                master     -> FETCH_HEAD
Already up to date.
[INFO FLW-0001] Using local build method. This will create binaries at 'tools/install' unless overwritten.
[INFO FLW-0017] Compiling Yosys.
make: Entering directory '/home/oyvind/OpenROAD-flow-scripts/tools/yosys'
mkdir -p /home/oyvind/OpenROAD-flow-scripts/tools/install/yosys/bin
cp yosys yosys-config yosys-abc yosys-filterlib yosys-smtbmc yosys-witness /home/oyvind/OpenROAD-flow-scripts/tools/install/yosys/bin
strip -S /home/oyvind/OpenROAD-flow-scripts/tools/install/yosys/bin/yosys
strip /home/oyvind/OpenROAD-flow-scripts/tools/install/yosys/bin/yosys-abc
strip /home/oyvind/OpenROAD-flow-scripts/tools/install/yosys/bin/yosys-filterlib
mkdir -p /home/oyvind/OpenROAD-flow-scripts/tools/install/yosys/share/yosys
cp -r share/. /home/oyvind/OpenROAD-flow-scripts/tools/install/yosys/share/yosys/.
make: Leaving directory '/home/oyvind/OpenROAD-flow-scripts/tools/yosys'
[INFO FLW-0018] Compiling OpenROAD with  -D CMAKE_INSTALL_PREFIX=/home/oyvind/OpenROAD-flow-scripts/tools/install/OpenROAD -D CMAKE_INSTALL_RPATH=/home/oyvind/OpenROAD-flow-scripts/dependencies/lib:/home/oyvind/OpenROAD-flow-scripts/dependencies/lib64 -D CMAKE_INSTALL_RPATH_USE_LINK_PATH=TRUE -DCMAKE_BUILD_TYPE=DEBUG -DCMAKE_CXX_FLAGS=-fsanitize=thread
-- OpenROAD version: v2.0-13772-ga515fc6cc
-- System name: Linux
-- Compiler: GNU 13.2.0
-- Build type: DEBUG
[deleted]
Consolidate compiler generated dependencies of target OpenSTA
FATAL: ThreadSanitizer: unexpected memory mapping 0x5c3e04a7b000-0x5c3e04a86000
CMake Error at /home/oyvind/OpenROAD-flow-scripts/dependencies/share/cmake-3.24/Modules/GoogleTestAddTests.cmake:112 (message):
  Error running test executable.

    Path: '/home/oyvind/OpenROAD-flow-scripts/tools/OpenROAD/build/src/gpl/test/fft_test'
    Result: 66
    Output:

Call Stack (most recent call first):
  /home/oyvind/OpenROAD-flow-scripts/dependencies/share/cmake-3.24/Modules/GoogleTestAddTests.cmake:225 (gtest_discover_tests_impl)
[deleted]
QuantamHD commented 2 weeks ago

Is that compiling with clang?

QuantamHD commented 2 weeks ago

I'm pretty sure you need to set a flag in the build script to use clang. It looks like that error is GCC related.

image
QuantamHD commented 2 weeks ago

https://medium.com/@joshisameeran/using-tsan-threadsanitizer-and-ways-to-avoid-false-sharing-in-clang-and-gcc-15fae5283ad1

oharboe commented 2 weeks ago

Hmm... doesn't look like this is a quick thing to try....

$ ODB_FILE=results/asap7/mock-array_Element/base/6_final.odb ./run-me-mock-array_Element-asap7-base.sh
FATAL: ThreadSanitizer: unexpected memory mapping 0x57f541c11000-0x57f545b3a000
oharboe commented 2 weeks ago

@maliberty @QuantamHD I don't seem to be able to reproduce the crash with this PR. Note that I haven't gotten ThreadSanatizer to work, but at least with release build and lots of mouse wiggling, the problem seems to be gone.

oharboe commented 2 weeks ago

@maliberty I think this could be merged, if you believe this is all that is required for #4990 and that #5107 can be pursued as a future improvement and further refinement.