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.

Sign up for GitHub

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

Jump to bottom

ACLE and UBSan disagree about clz with input zero #63113

Closed
tmatheson-arm opened this issue Jun 5, 2023 · 4 comments
Closed

ACLE and UBSan disagree about clz with input zero #63113

tmatheson-arm opened this issue Jun 5, 2023 · 4 comments
Assignees
@topperc
Labels
backend:AArch64 backend:ARM

Comments

@tmatheson-arm
Copy link
Contributor

tmatheson-arm commented Jun 5, 2023 •
edited

D152023 made calls to __builtin_clz and __builtin_ctz with input 0 to be undefined behaviour (RFC here). However we use them in the arm-acle.h to implement __clz:

llvm-project/clang/lib/Headers/arm_acle.h

Line 143 in c91246b

return (uint32_t)__builtin_clz(__t);

The ACLE documentation for __clz states:

When x is zero it returns the argument width, i.e. 32 or 64.

The definitions in the header will need to be updated to handle the zero case.
@llvmbot
Copy link
Collaborator

llvmbot commented Jun 5, 2023

@llvm/issue-subscribers-backend-aarch64

@llvmbot
Copy link
Collaborator

llvmbot commented Jun 5, 2023

@llvm/issue-subscribers-backend-arm

@topperc topperc self-assigned this Jul 10, 2023
@topperc
Copy link
Collaborator

topperc commented Jul 10, 2023

I broke this so I'll fix it.

@topperc
Copy link
Collaborator

topperc commented Jul 11, 2023

Its look gcc might have the same bug https://godbolt.org/z/YxKab9cMG ? The code appears to instrumented with a zero check.

topperc added a commit that referenced this issue Jul 11, 2023
@topperc
[ARM][AArch64] Make ACLE __clzl/__clzll return unsigned int instead o…
2df12f3 
…f unsigned long/uint64_t.

Use unsigned long in place of uint32_t for both clz and cls.

As far as I can tell this matches what ACLE defines and what gcc implements.

Noticed while investigating fixing #63113

Reviewed By: tmatheson

Differential Revision: https://reviews.llvm.org/D154910
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@topperc topperc

Labels
backend:AArch64 backend:ARM
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@EugeneZelenko @topperc @llvmbot @tmatheson-arm