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

ck_pr/aarch64: Specify output operands for ck_pr_md_store_* #215

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

markjdb
Copy link
Contributor

@markjdb markjdb commented Dec 18, 2023

This is required to enable KMSAN for aarch64 on FreeBSD. It will likely also be useful for userspace code which makes use of CK and is compiled with MSAN enabled.

As in commit 2f9acab, we want to specify output operand widths so that
MSAN compiler instrumentation correctly updates the shadow map.  In
particular, LLVM's implementation depends on having type information for
output operands, even when that's not otherwise necessary.  Without it,
KMSAN in FreeBSD generates false positives on aarch64.
@markjdb
Copy link
Contributor Author

markjdb commented Feb 1, 2024

Hello, is there any interest in taking this PR? It's useful when CK is used with code instrumented by MSAN and is low-risk otherwise.

@cognet cognet merged commit 0c9d3df into concurrencykit:master Feb 1, 2024
6 of 7 checks passed
@cognet
Copy link
Contributor

cognet commented Feb 1, 2024

Hi @markjdb, sorry about that, I just forgot, it is now merged, thank you!

@markjdb
Copy link
Contributor Author

markjdb commented Feb 1, 2024

Hi @markjdb, sorry about that, I just forgot, it is now merged, thank you!

Thank you! Can I simply cherry-pick this into FreeBSD?

@cognet
Copy link
Contributor

cognet commented Feb 2, 2024

Yes, please do

@markjdb
Copy link
Contributor Author

markjdb commented Feb 2, 2024

@cognet I'm very sorry, I just realized that this commit doesn't exactly match the one I'm using in FreeBSD. It's not right. Could you please revert, and I'll submit the updated patch? I'm sorry again.

@cognet
Copy link
Contributor

cognet commented Feb 2, 2024

@markjdb done!
Out of curiosity, what's wrong with this one?

@markjdb
Copy link
Contributor Author

markjdb commented Feb 2, 2024

Thanks. I failed to adjust the indexes of the operands in the actual assembly. Now that there's an output operand, %0 becomes %1, %1 becomes %2. I'm running the test suite on an arm64 board now just to confirm that I didn't miss something else.

@cognet
Copy link
Contributor

cognet commented Feb 2, 2024

Oh of course, I wonder how I missed that

@markjdb
Copy link
Contributor Author

markjdb commented Feb 8, 2024

I resubmitted the corrected and properly tested patch in #216.

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.

2 participants