-
Notifications
You must be signed in to change notification settings - Fork 49
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
Conversation
1d08433
to
89bb014
Compare
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
struct WidthAndSignedness { | ||
unsigned Width; | ||
struct RangeAndSignedness { | ||
unsigned Range; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this 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.
89bb014
to
def11ee
Compare
Width -> Rename dropped, existing Sema code tweaked and exposed, and ProvenanceCap now using ConstantPointerNull |
def11ee
to
937cbb9
Compare
Should mark as closing #720 when rebasing on the 16 merge |
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
937cbb9
to
4516e08
Compare
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.