[BoundsSafety] Reland the miscompilation fix for lvalues binding with temporary locations #11469
Conversation
rapidsna
requested review from
delcypher,
hnrklssn,
patrykstefanski and
apple-fcloutier
|
@swift-ci test llvm |
delcypher
added
the
clang:bounds-safety
|
CI seems to be blocked on rdar://161075623 ([BlueDragon] Build failed, AssertionError: {'CXCursor_LastExpr', 'CXCursor_ForgePtrExpr'} variants are missing.) |
|
@swift-ci test llvm |
| ElemBitCast); | ||
| : nullptr; | ||
|
|
||
| EmitWidePointerToDest(E->getType(), Ptr, Upper, Lower, ElemBitCast); |
There was a problem hiding this comment.
@hnrklssn No behavioral change here. This is just to avoid execution order of arguments, which is an unspecified behavior, triggering non-determinism for codegen, and CI failures on other platforms.
clang/lib/CodeGen/CGExpr.cpp
Outdated
|
|
||
| LValue CodeGenFunction::EmitTerminatedByToIndexableExprLValue(const TerminatedByToIndexableExpr *E) { | ||
| if (!E->getType()->isPointerTypeWithBounds()) | ||
| return EmitUnsupportedLValue(E, "l-value expression"); |
There was a problem hiding this comment.
Does Sema guard against this occurring, or is this something that can actually happen in real code?
There was a problem hiding this comment.
@hnrklssn Thanks for calling this out. This is not supposed to happen here.
I'll remove it. The assertion from EmitAggExprToLValue should be sufficient.
|
These are failing in next (b9b0b72) and unrelated to this change. 10:58:32 ******************** |
rapidsna
force-pushed
the
eng/PR-146329029_swift_next
branch
from
95c5ed9 to
250f73c
Compare
|
@swift-ci test llvm |
|
@hnrklssn Do you have any other comments? |
… temporary locations (#11469) - Solution from #10969: This should fix a miscompilation bug found during adoption. When a base is lvalue and is wrapped in OVE, the compiler created a temporary and incorrectly mapped the temporary's address as the lvalue instead of the actual address of the lvalue. The solution is to emit lvalue directly instead of creating extra temporary. - Issue with the solution: The issue #10969 got originally reverted due "error: cannot compile this l-value expression yet" occurred when __terminated_by_to_indexable is wrapped in an OVE. - Fix for the new issue with __terminated_by_to_indexable: The fix is to implement "TerminatedByToIndexableExprLValue" so the codegen can also emit as if it's an lvalue. rdar://146329029 (cherry picked from commit 04da1af)
Solution from #10969:
This should fix a miscompilation bug found during adoption. When a base is lvalue and is wrapped in OVE, the compiler created a temporary and incorrectly mapped the temporary's address as the lvalue instead of the actual address of the lvalue.
The solution is to emit lvalue directly instead of creating extra temporary.
Issue with the solution:
The issue #10969 got reverted due "error: cannot compile this l-value expression yet" occurred when
__terminated_by_to_indexableis wrapped in an OVE.Fix for the new issue with
__terminated_by_to_indexable:The fix is to implement "TerminatedByToIndexableExprLValue" so the codegen can also emit as if it's an lvalue.
rdar://146329029