-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[WIP][LLVM] Test: Validate Auto Shared Library Resolver Correctness. #19514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 5597d85. ♻️ This comment has been updated with latest results. |
|
Most of these tests are currently failing: Some of them are failing with this error: |
How do we solve that? |
|
Do you have any idea what this macro is doing? I'm not familiar with it: It looks like it's trying to load a library into Cling, but I'm not sure exactly where or how this is being triggered. |
|
This is user code forcing a dlopen. |
|
I'm seeing this error in those tests even on my master branch. |
You can check here what are the issues https://github.com/root-project/root/runs/47314709783 I see on windows: Can we resolve these first? |
|
LLVM Error: if (Obj->isCOFF())
return; // TODO: Add COFF support |
Sure, if that does not work in the master that’s a reasonable fix. |
|
Do you think these test failures are related to our changes? I have some async workarounds locally, so we could try them there too. |
Yes, most of them seem like this. The ROOT CI is generally green. |
|
Ok, I first need to understand this error. Do you know where this macro expands? |
You can build the master and break at dlopen. The debugger will show you when and where it gets expanded. |
|
I tried to reproduce the CI failure for the test This happens when the code tries to read symbols from a library that isn’t loaded yet (similar to ROOT/Cling’s The same type of error is also showing up on other platforms:
These errors cause test failures due to error diffs. The To test locally, I ran on my Mac M1 with the same Docker image and steps used in CI: docker run --security-opt label=disable --platform linux/amd64 -it registry.cern.ch/root-ci/ubuntu2504:buildreadyFollowing the GitHub Actions steps, the test passed locally, so I couldn’t reproduce the failure outside CI. |
@dpiparo, we need help here with the reproduction logic I guess... |
|
Hi @SahilPatidar looks like we have a relatively stable build. I'd like to point out that the two failing tests on macbeta: are unrelated to this PR. The TMVA test failures only on alma platforms seems strange, can you rebase on master? |
|
Ok, I’ll do that. But I recently rebased it — if the alma tests are failing due to this PR, could you let me know what might be going wrong based on the test failure details? |
The 3 failing tests belong to ROOT's machine learning component - TMVA. Two of the TMVA tests ( And on both alma8 and alma10 we see a seg fault coming from Perhaps you can try reproducing these with the relevant docker images? |
I think they are failing in many other PR, too |
|
If that's so, we should move forward... |
Yes, looks like none of the test failures here are related.
Yes, @SahilPatidar @vgvassilev I'd like to know how we proceed in this case. If the PR ready to land in LLVM, can we go ahead there? I assume we rework the DLM in Cling to use the new LibraryResolver with the next upgrade? |
|
I’d wait until landing this in llvm. I think we are quite close now. |
|
One more task I need to complete is moving LLVM PR code from Orc/TargetProcess to Orc/Shared, so that it can be used on the Root side as an API. |
|
@SahilPatidar, can we also drop the relevant code that this PR is replacing? |
|
Okay, I’m also going to push the concurrency-related issue fixes we encountered. Also in llvm side. |
This Pull request:
Changes or fixes:
Checklist:
This PR fixes #