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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

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.

clang/lib/CodeGen/CGBuiltin.cpp Outdated Show resolved Hide resolved
clang/lib/CodeGen/CGBuiltin.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaChecking.cpp Outdated Show resolved Hide resolved
This will be reused for __builtin_<op>_overflow, which gets checked in
SemaChecking.cpp instead and so can't currently use this.
@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

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 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

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