Skip to content
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

[Test][ORC][JITLink] Preserve rbx in the test ExecutionEngine/JITLink/x86-64/ELF_vtune.s #86472

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

yingcong-wu
Copy link
Contributor

@yingcong-wu yingcong-wu commented Mar 25, 2024

The callee should preserve rbx according to the calling convention, but it is not in the test case ExecutionEngine/JITLink/x86-64/ELF_vtune.s. Not preserving the rbx register may result in some random error to the caller function. This patch adds the missing command to preserve the rbx.

Copy link

✅ With the latest revision this PR passed the Python code formatter.

Copy link

✅ With the latest revision this PR passed the C/C++ code formatter.

@yingcong-wu
Copy link
Contributor Author

yingcong-wu commented Mar 25, 2024

Hi @yugier, I changed the test case, but I am not sure if the cfi directives need to update accordingly or not. Also, please check if this change would affect the test case or not. Can you please take a look?

Hi @MaskRay , @lhames , please help review the patch.

@yingcong-wu
Copy link
Contributor Author

Kindly ping @yugier , @MaskRay , @lhames. Please review the patch.

@yugier
Copy link
Contributor

yugier commented Apr 1, 2024

Hi, do you mean preserve rbxin your PR title?

@yingcong-wu yingcong-wu changed the title [Test][ORC][JITLink] Preserve rbp in the test ExecutionEngine/JITLink/x86-64/ELF_vtune.s [Test][ORC][JITLink] Preserve rbx in the test ExecutionEngine/JITLink/x86-64/ELF_vtune.s Apr 1, 2024
@yingcong-wu
Copy link
Contributor Author

Hi, do you mean preserve rbxin your PR title?

Updated, thanks.

@yugier
Copy link
Contributor

yugier commented Apr 1, 2024

I changed the test case, but I am not sure if the cfi directives need to update accordingly or not. Also, please check if this change would affect the test case or not. Can you please take a look?

I believe we do not need to update cfi directives here since we didn't modify anything related to the frame pointer. But please correct me if anyone believes I made a mistake here.

@yingcong-wu
Copy link
Contributor Author

Kindly ping @MaskRay , @lhames to review this PR.

@yingcong-wu yingcong-wu requested a review from yugier April 10, 2024 01:12
@yingcong-wu
Copy link
Contributor Author

Kindly ping @MaskRay , @lhames to review this PR.

@lhames
Copy link
Contributor

lhames commented Apr 15, 2024

Very belatedly -- LGTM. Thanks @yingcong-wu!

@lhames lhames requested review from lhames and removed request for yugier April 15, 2024 23:36
@lhames lhames merged commit 9cb755c into llvm:main Apr 15, 2024
4 checks passed
@yingcong-wu
Copy link
Contributor Author

No problem. Thank you @lhames and @yugier .

@yingcong-wu yingcong-wu deleted the yc/0325-orc-test-case-problem branch April 23, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants