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

Fix some UBSan errors #339

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

fvrmatteo
Copy link

@fvrmatteo fvrmatteo commented Nov 3, 2024

Opened by mistake, I'm currently doing tests on my fork. Although it might be useful to confirm if these UBSan errors are legitimate.

@marjevan
Copy link
Member

marjevan commented Nov 3, 2024

Hi,
If this is a mistake, please close the PR.
Thank you!

@fvrmatteo
Copy link
Author

fvrmatteo commented Nov 3, 2024

Hi @marjevan!

The PR was opened by mistake, but I now confirmed that the UBSan errors are legitimate (due to type punning and strict aliasing being violated) and that this PR fixes them.

I see that the issue was acknowledged in 2021, but it doesn't seem to have been addressed since then:

# undefined behavior sanitizer
#
# 2021-05-13: xed has several locations where it dereferences
#  unaligned pointers for 16/32/64 displacements & immediates. The
#  code generated by clang -O3 is worse for these cases when using
#  legal C. Thinking about how to handle that.

The usual approach to handle this case is to use std::memcpy and let the compiler recognise the idiom and lower it into a direct memory access with proper alias information associated to the load instruction (more details here: https://gist.github.com/shafik/848ae25ee209f698763cffee272a58f8).

Please let me know if we want to proceed with fixing these type punning issues (e.g. if someone wants to take a look at the issue over the whole codebase, since it was officially acknowledged in the past) or if we want to keep ignoring it (since I also see xed_mbuild.py is purposefully disabling the UBSan sanitiser).

@marjevan
Copy link
Member

marjevan commented Nov 3, 2024

Thank you for reaching out! We're constantly working to improve the XED library and its code quality, and we greatly value contributions from our user community.
If you're interested in contributing, please refer to our contribution guide at the following link.

@marjevan marjevan marked this pull request as draft November 4, 2024 07:31
Signed-off-by: Matteo Favaro <[email protected]>
@fvrmatteo
Copy link
Author

Hello @marjevan, I read the contribution guide and signed-off my commit.

@marjevan marjevan self-assigned this Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants