-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[IR] Disable unsound inttoptr(ptrtoint(p)) to p fold #161662
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
base: main
Are you sure you want to change the base?
Conversation
; CHECK-NEXT: [[PTRCMP:%.*]] = icmp ult ptr [[B_ADDR_02]], [[A_END]] | ||
; CHECK-NEXT: [[B_ADDR_02:%.*]] = phi i64 [ [[ADD_INT:%.*]], [[BB]] ], [ [[B]], [[FOR_BODY_PREHEADER]] ] | ||
; CHECK-NEXT: [[TMP:%.*]] = inttoptr i64 [[B_ADDR_02]] to ptr | ||
; CHECK-NEXT: [[PTRCMP:%.*]] = icmp ugt ptr [[A_END]], [[TMP]] |
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.
We could still fold this as the inttoptr is only used in icmp...
; CHECK: end: | ||
; CHECK-NEXT: [[I:%.*]] = ptrtoint ptr [[A]] to i64 | ||
; CHECK-NEXT: [[G:%.*]] = inttoptr i64 [[I]] to ptr | ||
; CHECK-NEXT: store i32 10, ptr [[G]], align 4 |
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 we could fold this by arguing that any provenance inttoptr
here may pick cannot be better than the provenance of %a
, which is dereferenceable for the full store size.
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.
How does that argument interact with restrict pointers? (Say you have a pointer @g
, and a restrict argument %r
equal to that pointer, and a ptrtoint of both of those. Can you replace a ptrtoint/inttoptr of @g
with @g
, or only %r
?)
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.
Yeah, I think this reasoning doesn't really hold up as soon as some kind of derived provenance is involved. I think it's fine for allocas for now, but maybe not in the future with full restrict.
This area (interaction of noalias and dereferenceability) is something we generally have problems with.
One example is:
func(noalias p) {
*p = 1
if (p == global) {
*p = 2
}
return *p
}
Where we use the dereferenceability of global to justify the pointer replacement inside the if, but that's incorrect in the presence of noalias.
And of course, we can speculate a load from a global such that it conflicts with a noalias store, introducing UB. (And IIRC it's not possible to define that to be poison either.)
; CHECK-NEXT: [[V3:%.*]] = call i32 [[P:%.*]]() | ||
; CHECK-NEXT: [[V0:%.*]] = ptrtoint ptr [[P1:%.*]] to i64 | ||
; CHECK-NEXT: [[P:%.*]] = inttoptr i64 [[V0]] to ptr | ||
; CHECK-NEXT: [[V3:%.*]] = call i32 [[P]]() |
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.
This looks like a design defect in the ptrauth intrinsics -- why are they working on integers, not pointers?
No description provided.