Remove implicit total orderings #107982

dingxiangfei2009 · 2023-02-13T08:24:13Z

This PR will attempt to remove implicit orderings on sty data that are not naturally bestowed with either total or partial ordering.

It has come to attention specifically that a subtle mis-compilation was introduced in #104786 causing segfaults. By transposing the order of the variants in Mutability, in rare cases ADT constructors are miscompiled with incorrect memory layout. More details will come as the investigation into making a MVCE is underway.

rustbot · 2023-02-13T08:24:20Z

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

compiler-errors · 2023-02-13T18:47:06Z

I'm happy with this implementation, but I'm curious why...

By transposing the order of the variants in Mutability, in rare cases ADT constructors are miscompiled with incorrect memory layout.

^ this was happening. My understanding of #104786 (seeing that I reviewed it) is that we're allowed to rely on the total ordering of enums in Rust. In what cases were segfaults occurring? Are there any issue # filed against rust-lang/rust associated with it?

bors · 2023-02-13T21:13:01Z

☔ The latest upstream changes (presumably #107924) made this pull request unmergeable. Please resolve the merge conflicts.

dingxiangfei2009 · 2023-02-15T15:32:17Z

@compiler-errors I am curious, too. It is unfortunate that the code is proprietary, but I will post as much detail as possible here.

The segfault happens when a Option<&String> is read from without checking if it is None. I have inspected the disassembly of the mis-compiled binary.
The original Rust program. Sketch

let options = if cond {
    Some(format!(...))
} else {
    None
};
// ...
let obj = Struct {
    val: options.as_ref(),
};

This is compiled into this AArch64 disassembly. Sketch

LAB_COND_TRUE:
    # some calls to alloc::fmt::format::format_inner
    ldr q0, [x25] # move the String...
    str q0, [sp, 0xb0] # ... into the destination place
    b LAB_BOMB
LAB_COND_FALSE:
    str xzr, [sp, 0xb8]
    b LAB_BOMB
    # ...
LAB_BOMB:
    add x12, sp, 0xb0 # unconditionally take pointer to Option<String>
    str x12, [sp, 0x1b0] # and put it into the "Struct"

Further in another call, it would test if this pointer to Option<String> is not null and access the content accordingly. However, since $sp+0xb0 is never properly initialized, it is reading garbage in that call because the pointer here is never null.

This appears to happen on AMD64 as well.

By inserting println! randomly, it sometimes compiles correctly. Here is the difference in the disassembly.

LAB_COND_TRUE:
    # some calls to alloc::fmt::format::format_inner
    ldr q0, [x25] # move the String...
    str q0, [sp, 0xb0] # ... into the destination place
    # ...
    ldr x8, [sp, 0xb8]
    add x9, sp, 0xb0
    cmp x8, 0x0
    csel x11, xzr, x9, eq # I believe it is checking if String is empty
    b LAB_GOOD
LAB_COND_FALSE:
    mov x11, 0x0
    str xzr, [sp, 0xb8]
    b LAB_GOOD
    # ...
LAB_GOOD:
    str x11, [sp, 0x1b0] # Put it into the "Struct"

Strangely, it is really hard to make a MVCE out of it. It seems that even by adding RUSTFLAGS=--emit=llvm-ir or --emit=mir would make the segfault go away. I would happily accept advice on how to dump the IRs without "disturbing" the internals to much.

cjgillot · 2023-02-15T18:19:53Z

compiler/rustc_borrowck/src/diagnostics/bound_region_errors.rs

                }
                _ => None,
            }
-        })?;
+        })
+        .min_by_key(|(discr, vid, ..)| (*discr, *vid))?;


Why doesn't find_map work any more?

cjgillot · 2023-02-15T18:23:32Z

compiler/rustc_ast/src/ast.rs

 #[derive(HashStable_Generic, Encodable, Decodable)]
 pub enum Mutability {
-    // N.B. Order is deliberate, so that Not < Mut


Mutability is perhaps the one case where keeping the order is useful and sensible.

cjgillot · 2023-02-15T18:24:27Z

compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs

+        input_constraints.sort_by_key(|c| match c {
+            Constraint::VarSubVar(..) => 0u8,
+            Constraint::RegSubVar(..) => 1,
+            Constraint::VarSubReg(..) | Constraint::RegSubReg(..) => 2,


This definitely needs a comment.

cjgillot · 2023-02-15T18:24:56Z

compiler/rustc_middle/src/mir/query/order.rs

@@ -0,0 +1,27 @@
+use super::ConstraintCategory;
+
+impl<'tcx> ConstraintCategory<'tcx> {


This does not need to be in a separate file.

cjgillot · 2023-02-15T18:27:34Z

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

-            candidates.sort();
-            candidates.dedup();
-            let len = candidates.len();
+        let report = |candidates: Vec<TraitRef<'tcx>>, err: &mut Diagnostic| {


Directly make candidates a FxIndexSet.

bors · 2023-02-16T11:24:32Z

☔ The latest upstream changes (presumably #108116) made this pull request unmergeable. Please resolve the merge conflicts.

Dylan-DPC · 2023-05-19T15:11:21Z

@dingxiangfei2009 any updates on this?

dingxiangfei2009 · 2023-05-21T21:21:24Z

@Dylan-DPC I forgot about this! The original issue was fixed in LLVM. However, I think it might still be a good idea to work with the types without ordering. If there is no objection to this idea, I will come back to it, rebase it and post fresh commits.

compiler-errors · 2023-05-21T21:29:02Z

If this was due to an LLVM bug, and not a Rust bug, then I don't see why it's beneficial to remove these total orderings personally.

JohnCSimon · 2023-08-27T06:12:19Z

@dingxiangfei2009
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rustbot assigned wesleywiser Feb 13, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 13, 2023

dingxiangfei2009 force-pushed the remove-ord branch from 4c646b0 to 6d3612d Compare February 15, 2023 15:44

Mark-Simulacrum force-pushed the master branch from 47c0792 to 999ac5f Compare February 15, 2023 16:11

remove implicit total orderings

f5e1f88

dingxiangfei2009 force-pushed the remove-ord branch from 6d3612d to f5e1f88 Compare February 15, 2023 17:54

dingxiangfei2009 marked this pull request as ready for review February 15, 2023 17:55

cjgillot reviewed Feb 15, 2023

View reviewed changes

cjgillot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 6, 2023

cjgillot self-assigned this Mar 6, 2023

cjgillot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 6, 2023

JohnCSimon closed this Aug 27, 2023

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Aug 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove implicit total orderings #107982

Remove implicit total orderings #107982

dingxiangfei2009 commented Feb 13, 2023

rustbot commented Feb 13, 2023

compiler-errors commented Feb 13, 2023 •

edited

Loading

bors commented Feb 13, 2023

dingxiangfei2009 commented Feb 15, 2023

cjgillot Feb 15, 2023

cjgillot Feb 15, 2023

cjgillot Feb 15, 2023

cjgillot Feb 15, 2023

cjgillot Feb 15, 2023

bors commented Feb 16, 2023

Dylan-DPC commented May 19, 2023

dingxiangfei2009 commented May 21, 2023

compiler-errors commented May 21, 2023

JohnCSimon commented Aug 27, 2023

		@@ -0,0 +1,27 @@
		use super::ConstraintCategory;

		impl<'tcx> ConstraintCategory<'tcx> {

Remove implicit total orderings #107982

Remove implicit total orderings #107982

Conversation

dingxiangfei2009 commented Feb 13, 2023

rustbot commented Feb 13, 2023

compiler-errors commented Feb 13, 2023 • edited Loading

bors commented Feb 13, 2023

dingxiangfei2009 commented Feb 15, 2023

cjgillot Feb 15, 2023

Choose a reason for hiding this comment

cjgillot Feb 15, 2023

Choose a reason for hiding this comment

cjgillot Feb 15, 2023

Choose a reason for hiding this comment

cjgillot Feb 15, 2023

Choose a reason for hiding this comment

cjgillot Feb 15, 2023

Choose a reason for hiding this comment

bors commented Feb 16, 2023

Dylan-DPC commented May 19, 2023

dingxiangfei2009 commented May 21, 2023

compiler-errors commented May 21, 2023

JohnCSimon commented Aug 27, 2023

compiler-errors commented Feb 13, 2023 •

edited

Loading