ci: use full ASAN #2231
ci: use full ASAN #2231
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2231 +/- ##
==========================================
- Coverage 89.71% 89.71% -0.01%
==========================================
Files 1012 1013 +1
Lines 35919 35933 +14
==========================================
+ Hits 32226 32238 +12
- Misses 3693 3695 +2
☔ View full report in Codecov by Sentry. |
The lock-free queue is the source of an ASAN failure. Specifically, the queue does not guarantee strict FIFO circumstances in the name of speed. This means that our code that loops through the queue is incorrect. The goal is to replace this queue-based design soon regardless, and hence we don't want to spend more time on this. Also, this commit removes a useless logger in the buffer manager.
|
The lock-free queue is the source of an ASAN failure. Specifically, the The goal is to replace this queue-based design soon regardless, and Also, this commit removes a useless logger in the buffer manager. |
Previously we used LD_PRELOAD to add ASAN to our build. This is at best imperfect, since the full benefits of ASAN can only be realized by adding compiler instrumentation. This does not run the python, nodejs, or java tests with ASAN. Though Python can be run with ASAN using LD_PRELOAD, it is super slow, and would need parallelized testing to be viable (which is not worth it IMO). Nodejs and Java just die spectacularly. Overall I think the APIs need ASAN much less.
| @@ -113,24 +113,20 @@ clang-tidy-ci: | |||
| $(call mkdirp,build/release) && cd build/release && \ | |||
| cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_EXPORT_COMPILE_COMMANDS=1 ../.. | |||
|
|
|||
| pytest: | |||
| $(MAKE) release | |||
| pytest: release | |||
There was a problem hiding this comment.
If we make this change, will it still rerun make release every time whenmake pytest is run? Since release does not have file dependencies configured and is simply a shortcut to invoke CMake, I am not sure if it will rebuild automatically if the code is changed?
There was a problem hiding this comment.
release is a dummy target, so Make will run it everytime pytest is run
There was a problem hiding this comment.
The change to queue is indeed decreasing the performance of BM, as each unpin now gets to lock the queue. This should be fixed with issue #2137 , so we move away from the queue-based solution. As for now, we decide to merge this first to get CI asan correctly, and fix it later before the next release.
Previously we used LD_PRELOAD to add ASAN to our build. This is at best
imperfect, since the full benefits of ASAN can only be realized by
adding compiler instrumentation.
This does not run the python, nodejs, or java tests with ASAN. Though
Python can be run with ASAN using LD_PRELOAD, it is super slow, and
would need parallelized testing to be viable (which is not worth it
IMO).
Nodejs and Java just die spectacularly. Overall I think the APIs need
ASAN much less.