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

[Sema][CodeGen] Support __builtin_<op>_overflow with __intcap #747

Merged
merged 2 commits into from
Mar 17, 2025

Conversation

jrtc27
Copy link
Member

@jrtc27 jrtc27 commented Jul 31, 2024

Morello LLVM has downstream support for this, but it's both incomplete
(see https://git.morello-project.org/morello/llvm-project/-/issues/80)
and incorrect with regards to provenance (in that it takes a naive
type-based approach rather than considering the cheri_no_provenance
attribute, meaning it differs from the binary operators in provenance
semantics). This is a from-scratch implementation that aims to not have
the same shortcomings.

@jrtc27 jrtc27 force-pushed the builtin-op-overflow branch from 1d08433 to 89bb014 Compare July 31, 2024 20:37
struct WidthAndSignedness {
unsigned Width;
struct RangeAndSignedness {
unsigned Range;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to upstream this NFC rename to reduce the diff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I was confused and our getIntWidth in fact gives the range, which I guess makes sense in C land where this is. Our getIntRange is just a wrapper for getIntWidth, and I suppose is a stalled effort to distinguish the two, which likely needs a hell of a lot more work and commitment from upstream to adopt. So probably this renaming should just be dropped?

Copy link
Member Author

@jrtc27 jrtc27 Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still worth upstreaming llvm/llvm-project#101765 as a minor thing though

Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments otherwise LGTM.

@jrtc27 jrtc27 force-pushed the builtin-op-overflow branch from 89bb014 to def11ee Compare August 2, 2024 23:09
@jrtc27
Copy link
Member Author

jrtc27 commented Aug 2, 2024

Width -> Rename dropped, existing Sema code tweaked and exposed, and ProvenanceCap now using ConstantPointerNull

@jrtc27 jrtc27 force-pushed the builtin-op-overflow branch from def11ee to 937cbb9 Compare August 3, 2024 02:03
@jrtc27
Copy link
Member Author

jrtc27 commented Nov 20, 2024

Should mark as closing #720 when rebasing on the 16 merge

jrtc27 added 2 commits March 17, 2025 20:04
This will be reused for __builtin_<op>_overflow, which gets checked in
SemaChecking.cpp instead and so can't currently use this.
Morello LLVM has downstream support for this, but it's both incomplete
(see https://git.morello-project.org/morello/llvm-project/-/issues/80)
and incorrect with regards to provenance (in that it takes a naive
type-based approach rather than considering the cheri_no_provenance
attribute, meaning it differs from the binary operators in provenance
semantics). This is a from-scratch implementation that aims to not have
the same shortcomings.

Fixes #720
@jrtc27 jrtc27 force-pushed the builtin-op-overflow branch from 937cbb9 to 4516e08 Compare March 17, 2025 20:34
@jrtc27 jrtc27 merged commit 81f4a37 into dev Mar 17, 2025
5 of 6 checks passed
@jrtc27 jrtc27 deleted the builtin-op-overflow branch March 17, 2025 23:05
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