-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[ExceptionDemo] Correct and update example ExceptionDemo #69485
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
This is switching to OrcJit, so maybe @lhames can help review this example? Also seems like no bot is building with LLVM_ENABLE_EH and the examples enabled? |
ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be needed on top-of-tree: LLJIT now reflects process symbols by default.
Otherwise LGTM. |
That true, I just removed the corresponding code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @epitavy!
c4f2629
to
1589168
Compare
Thanks @lhames . |
Ping @lhames |
@epitavy , can you rebase and follow guideline here https://llvm.org/docs/GitHub.html |
Signed-off-by: Eliaz Pitavy <eliaz.pitavy@obspm.fr>
bb52a1c
to
558c089
Compare
@kmistryic The rebase is done, I messed up a little bit with the commit history, but it should be clean. Can you land the changes ? I don't have commit permission to do it |
@epitavy , Have you tested locally with below command? |
I just tried It still failing but different error now. |
Right, I missed an error because I didn't build with debug enabled, so assertions were removed. It's fixed now. |
825586c
to
8e65c7f
Compare
8e65c7f
to
7047434
Compare
ping @kamleshbhalui |
@epitavy Sorry I didn't get to this earlier -- I've been busy with a relocation. @kamleshbhalui -- thanks very much for landing! |
)" This reverts commit 2ece5cc.
@epitavy Please have a look at it again |
It is still not building correctly ? |
Correct, test by executing the |
It works as expected for me, except that some values are wrong.
96 should be 2, but the bug was already here, and I don't know how to fix that |
I have reverted for now, and reopened the issue. |
You expect someone to fix the bug on this pull request ? I am not sure about the process to follow |
I think I fixed the bug, where should I push the patch ? |
Can you share entire execution log of |
I think you have to create new PR with all the change, you can mention this PR there. |
I share the log on the new PR |
The ExceptionDemo example was no longer compiling (since llvm 14 at least). The PR makes the example work with the current API and also transition from MCJIT to ORC.
Fixes #63702