-
-
Notifications
You must be signed in to change notification settings - Fork 14.9k
GVN assumes nested shared references are read-only #155884
Copy link
Copy link
Open
Labels
A-mir-opt-GVNArea: MIR opt Global Value Numbering (GVN)Area: MIR opt Global Value Numbering (GVN)C-bugCategory: This is a bug.Category: This is a bug.I-miscompileIssue: Correct Rust code lowers to incorrect machine codeIssue: Correct Rust code lowers to incorrect machine codeI-unsoundIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/SoundnessIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/SoundnessP-highHigh priorityHigh priorityT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.Relevant to the compiler team, which will review and decide on the PR/issue.T-opsemRelevant to the opsem teamRelevant to the opsem teamneeds-triageThis issue may need triage. Remove it if it has been sufficiently triaged.This issue may need triage. Remove it if it has been sufficiently triaged.
Metadata
Metadata
Assignees
Labels
A-mir-opt-GVNArea: MIR opt Global Value Numbering (GVN)Area: MIR opt Global Value Numbering (GVN)C-bugCategory: This is a bug.Category: This is a bug.I-miscompileIssue: Correct Rust code lowers to incorrect machine codeIssue: Correct Rust code lowers to incorrect machine codeI-unsoundIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/SoundnessIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/SoundnessP-highHigh priorityHigh priorityT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.Relevant to the compiler team, which will review and decide on the PR/issue.T-opsemRelevant to the opsem teamRelevant to the opsem teamneeds-triageThis issue may need triage. Remove it if it has been sufficiently triaged.This issue may need triage. Remove it if it has been sufficiently triaged.
Type
Fields
Give feedbackNo fields configured for issues without a type.
GVN optimizes this function
to remove the apparently redundant second load of the inner integer.
However, if we call that function as follows...
then this optimization changes the resulting program behavior. Neither SB nor TB flag UB in this program, making the optimization unsound under our current models.
This issue was previously fixed in GVN by #133474 but got re-introduced by #132527. The example needs to use
&(&i32,)instead of&&i32to bypass a check that was recently added to GVN. I cannot tell if that check is ineffective to fix the actual problem it was meant to fix, but it is certainly ineffective for this issue.Arguably this is more of a defect in the model than in GVN, but it could be non-trivial to fix / the fix might cause problems on its own. This is basically an instance of rust-lang/unsafe-code-guidelines#593. Under the current model, GVN has to prove that the shared reference was constructed properly (i.e., that it went through a retag) before it can assume that the pointee is truly immutable.
Cc @rust-lang/opsem @rust-lang/wg-mir-opt @cjgillot @dianqk