[CI] Enable ASAN build properly - #22943
Conversation
Test Results 23 files 23 suites 3d 19h 21m 59s ⏱️ Results for commit 6442c1c. ♻️ This comment has been updated with latest results. |
fe70534 to
bd3d738
Compare
This reverts commit d8695be.
There are too many failing tests with it
In ROOTTEST_ADD_TEST we have a piece of code that adds the Asan libraries to LD_PRELOAD for python scripts, but it excludes a few special cases. Among these there is driveTabCom.py, which is mentioned explicitly by driveRedirect.py to have a similar functionality (that functionality being driving the root.exe process through a pseudo-interactive tty to test some stuff). For reasons that are not entirely clear to me, these scripts don't want the asan libraries in LD_PRELOAD. Possibly because, not having an `import ROOT`, they don't benefit from the tweaks added by this PR: root-project#11621 which prevents Asan libraries from leaking into the ROOT process. This is just a guess. This change fixes a problem with roottest-root-rint-Redirect, which otherwise fails on Alma10 with: ld.so: object '/usr/lib/gcc/x86_64-redhat-linux/14/libasan.so' from LD_PRELOAD cannot be preloaded (file too short): ignored. Notably, this failure doesn't happen on Archlinux. As far as I can tell, the main difference is that on Alma, libasan.so is actually a linker script redirecting to /usr/lib64/libasan.so.8.0.0, whereas in Arch the preloaded library (/usr/lib/libasan.so in that case) is an actual shared object. To be ascertained whether this fix is the "proper" one, but it surely is in line with the way we fixed driveTabCom.py, so if this is to be revised, that one likely needs to be as well.
| } else { | ||
| if (needToUnlinkTempFile) { | ||
| // Remove the temporary stderr file if it was created. | ||
| gSystem->Unlink(stderrfile); | ||
| } | ||
| gSystem->Unlink(bakdepfilename); |
There was a problem hiding this comment.
It is not clear from the succinct commit message whether the change of behavior here is intended or not. Previously (if I remember/read correctly) the err log file was kept in case of error behavior (thus allowing it to be reviewed if need be) while in the new behavior it seems to always be removed. Was that the intent? If it is, did we check there is no useful information being lost?
There was a problem hiding this comment.
Based on the fact that stderrfile is set to /dev/null except on Windows, I interpreted the code's intention as a "we must get rid of this temp file if we created it" rather than a conditionally-kept error log (if it was generally an err log to be kept, why only on Windows?), but I might have been wrong.
But in that case, why do we only keep it in that very specific case?
3 participants
Based on #22726, with the following additions: