regression: encountered mutable pointer in final value when "outer scope" rule applies in const/static with interior mutability #121610

Mark-Simulacrum · 2024-02-25T21:03:53Z

[INFO] [stdout] error: encountered mutable pointer in final value of constant
[INFO] [stdout]    --> /opt/rustwide/cargo-home/registry/src/index.crates.io-6f17d22bba15001f/Boa-0.13.0/src/builtins/map/mod.rs:341:9
[INFO] [stdout]     |
[INFO] [stdout] 341 |         const JS_ZERO: &JsValue = &JsValue::Rational(0f64);
[INFO] [stdout]     |         ^^^^^^^^^^^^^^^^^^^^^^^

The text was updated successfully, but these errors were encountered:

apiraino · 2024-02-26T14:37:21Z

bisection seems to point to #119044 ? cc @RalfJung @oli-obk

searched nightlies: from nightly-2024-01-01 to nightly-2024-02-26
regressed nightly: nightly-2024-01-24
searched commit range: d5fd099...5d3d347
regressed commit: 6265a95

bisected with cargo-bisect-rustc v0.6.8

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start=2024-01-01 -- test

RalfJung · 2024-02-26T14:59:00Z

Oh strange, this landed a while ago and so far we got no reports I think...

Boa-0.13.0/src/builtins/map/mod.rs:341:9

That seems to be a deprecated crate, the repo has moved on to a new version. Not sure how to get source code for it.

The epo crate also regresses via boa

[INFO] [stdout] error: encountered mutable pointer in final value of constant
[INFO] [stdout]    --> /opt/rustwide/cargo-home/registry/src/index.crates.io-6f17d22bba15001f/boa_engine-0.14.0/src/builtins/mod.rs:205:9
[INFO] [stdout]     |
[INFO] [stdout] 205 |         const UNDEFINED: &JsValue = &JsValue::Undefined;
[INFO] [stdout]     |

That code I found here. (The current version of the crate on master doesn't have this any more, at least not in that file.) But so far my attempts at extracting an example that reproduces the issue failed. I assume it has to do with that JsObject type, that mentions some Gc things so it might be particularly tricky.

apiraino · 2024-02-27T11:00:39Z

I can reproduce the crash by compiling an old tarball of the boa crate (v0.13). But it also ~~crashes~~ stops compiling on rustc stable (1.76) for other reasons (glob imports), so unsure how helpful can that be.

I have the feeling that trying to get to a reproducer would be expensive in terms of time vs. benefit 🤔 But I am unsure if behind this ~~crash~~ compile error lies something useful for us to know

RalfJung · 2024-02-27T11:19:50Z

"crash"? It doesn't crash for any of them I hope, just show this error?

I am certainly surprised that there is a regression at all, in particular given that an earlier version of that PR (#116745) has been cratered and didn't find anything. So in that sense it would be interesting to extract a testcase here.

apiraino · 2024-02-27T12:49:03Z

"crash"? It doesn't crash for any of them I hope, just show this error?

I stand corrected. Compile stops and exits with the error reported in this issues (sorry for mixing up apple and oranges).

RalfJung · 2024-02-28T11:11:14Z

I copied more of the type definition into its own file, still no luck.

But I think the only way this can happen is if different parts of the compiler disagree on whether a type has interior mutability, which is quite concerning.

RalfJung · 2024-02-28T16:49:39Z

Aah! I finally got it:

use std::cell::Cell;

pub enum JsValue {
    Undefined,
    Object(Cell<bool>),
}

impl ::std::ops::Drop for JsValue {
    fn drop(&mut self) {}
}

const UNDEFINED: &JsValue = &JsValue::Undefined;

fn main() {
}

The Drop is key. In boa it's generated by a derive which made this hard to spot...

So, I think what happens here is that we don't do promotion because of the Drop, but the "outer scope" rule still applies. The reference is created to JsValue::Undefined and some analysis concludes that this is fine since while the type has interior mutability, the value doesn't. But when we actually evaluate the initializer for UNDEFINED we just see a shared reference created to a type that has interior mutability so we mark it as potentially mutable, and then interning sees that and rejects it.

Honestly my preference would be to reject this in const-checking, IMO we shouldn't allow references to interior mutable types under the "outer scope" rule. Note that both Stacked Borrows and Tree Borrows actually currently allow writing through &JsValue::Undefined (or they would if we turned off promotion, which changes the code in a way that UB arises without even involving the aliasing model). In that sense both promotion and const-checking are really playing with fire when they accept enum variants of enums where other variants have interior mutability.

RalfJung · 2024-02-29T07:31:22Z

So... actually allowing this code again will be quite tricky. Not sure if we want to do that.

What I would prefer to do is make this check entirely type-based, and thus give a nicer error for when this happens:

rust/compiler/rustc_const_eval/src/transform/check_consts/check.rs

Lines 487 to 491 in 010f394

    
           let borrowed_place_has_mut_interior = qualifs::in_place::<HasMutInterior, _>( 
        
               self.ccx, 
        
               &mut |local| self.qualifs.has_mut_interior(self.ccx, local, location), 
        
               place.as_ref(), 
        
           );

But that is quite the departure from our attempts to handle interior mutability in a value-based way, i.e. with these "qualifs".

EDIT: That's now implemented in #121786.

RalfJung · 2024-02-29T10:01:58Z

So... what shall we do here? Retroactively ask T-lang to approve the part of #119044 that is a breaking change? That PR went through FCP but we didn't realize it would be a breaking change.

oli-obk · 2024-02-29T12:18:19Z

So, I think what happens here is that we don't do promotion because of the Drop,

I'm assuming changing that, specifically for promotion in consts/statics is not gonna fly? We are already trying to remove/limit the method call exception that promotion has there

RalfJung · 2024-02-29T12:28:03Z

You mean making the code work by having promotion kick in regardless? That sounds very tricky, we'd have to ensure this happens only when it's the final value of the const being promoted, not some intermediate value.

If we absolutely must accept this code then we can make const-eval do a type-driven traversal of the x in &x expressions to check whether there's any UnsafeCell anywhere. But that would introduce UB into code that I don't think should have UB (when x is not valid at the given type) and it could be terrible for performance...

oli-obk · 2024-02-29T12:37:06Z

If we absolutely must accept this code then we can make const-eval do a type-driven traversal of the x in &x expressions to check whether there's any UnsafeCell anywhere. But that would introduce UB into code that I don't think should have UB (when x is not valid at the given type) and it could be terrible for performance...

Yea, i discarded this option right away and was looking for alternatives

RalfJung · 2024-02-29T18:44:40Z

Yea, i discarded this option right away and was looking for alternatives

I think that option is still better than making promotion cover this.^^

oli-obk · 2024-03-01T09:56:44Z

I guess we each hate each other's preferred solution, and neither of us love our own preferred solution. So...

@rust-lang/lang We did an accidental breaking change in #119044

The following example does not compile on beta anymore, but does compile on stable:

use std::cell::Cell;

pub enum JsValue {
    Undefined,
    Object(Cell<bool>),
}

impl ::std::ops::Drop for JsValue {
    fn drop(&mut self) {}
}

const UNDEFINED: &JsValue = &JsValue::Undefined;

with

error: encountered mutable pointer in final value of constant
  --> src/main.rs:12:1
   |
12 | const UNDEFINED: &JsValue = &JsValue::Undefined;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^

what happens here is that we don't do promotion because of the Drop, but the "outer scope" rule still applies. The reference is created to JsValue::Undefined and const checking concludes that this is fine since while the type has interior mutability, the value doesn't. But when we actually evaluate the initializer for UNDEFINED we just see a shared reference created to a type that has interior mutability so we mark it as potentially mutable, and then interning sees that and rejects it.

The reason this worked before was that before #119044 we did a value based (not just type based) interning traversal, so we picked up on the fact that the specific value has no interior mutability.

We have come up with two ways to get back the previous behaviour:

we can make const-eval do a type-driven traversal of the x in &x expressions to check whether there's any UnsafeCell anywhere.
special case promotion to trigger for these values, even though their type is Drop and !Freeze

The issues with those are respectively:

that would introduce UB into code that should not have UB (when x is not valid at the given type) and it would be (very) terrible for performance...
terrible extension to an already fragile part of the compiler: promotion.

So we would like to just keep rejecting this code, as it requires special cases to support instead of falling out of a set of simple rules.

RalfJung · 2024-03-01T10:36:20Z

I guess we each hate each other's preferred solution, and neither of us love our own preferred solution. So...

Well, "hate" is a strong term.

I did think some more about this, and I think if we can make promotion entirely take over the "enclosing scope" rule then maybe it's worth it. That will simplify const-checking. However, I think this means duplicating and carefully aligning the "enclosing scope" logic inside promotion which has a huge risk of going wrong -- in particular as both of them keep evolving.

The reason this worked before was that before #119044 we did a value based (not just type based) interning traversal, so we picked up on the fact that the specific value has no interior mutability.

The point to emphasize is that "value has (no) interior mutability" is not necessarily a meaningful concept, as far as t-opsem is concerned. There has been a lot of discussion around exactly how far interior mutability "leaks" into its surroundings when a type contains some UnsafeCell and some data outside the UnsafeCell, but all the options that t-opsem was still seriously considering would all agree that there is some leakage, i.e., that at least some of the bytes in *UNDEFINED would actually be considered mutable no matter its value.

This is also tied up with rust-lang/unsafe-code-guidelines#493. There are two places where rustc does value-based reasoning for interior mutability: promotion and const-checking. This issue here is about const-checking. No matter what we do here, we'll have to figure out what to do with rust-lang/unsafe-code-guidelines#493 eventually... I don't know if we can ever get rid of value-based reasoning for interior mutability, but I also don't think we want to have an operational semantics that would permit value-based reasoning for interior mutability.

So we would like to just keep rejecting this code, as it requires special cases to support instead of falling out of a set of simple rules.

To add to that, that doesn't mean we consider the issue resolved, we want to keep tracking it somewhere. But right now we feel like the cure for this regression is worse than the disease, so if we can get away with this breaking change then that seems like the least bad option.

The regressions listed above all come from a single crate, boa. The problematic code has been removed from boa at least a few months ago; the latest released version builds fine with current nightly, modulo some unrelated issues.

…g, r=<try> const checking: do not do value-based reasoning for interior mutability This basically means a nicer error for rust-lang#121610, see the new test in `tests/ui/consts/refs-to-cell-in-final.rs`. Currently we have a mismatch between the dynamic semantics of `&` (things might be mutable when the pointee type is `!Freeze`, no matter the pointee value) and const-qualif (doing value-based reasoning). This removes that mismatch. I don't think this can break anything that builds on current nightly; any such code would already later run into the error that the interner complains about the mutable pointer (that's exactly rust-lang#121610). So a crater run makes little sense. Overall this peanuts compared to the *actual* problem with value-based reasoning for interior mutability in promotion (rust-lang/unsafe-code-guidelines#493). I have no idea how to resolve that. But at least now the use of the problematic qualif is down to 1... The first commit is independent, I just got worried about promotion of unions and was happy to notice that at least there we don't do value-based reasoning. r? `@oli-obk`

traviscross · 2024-03-06T23:52:15Z

We discussed this today in triage without resolution. We're planning to take this question back up on the 20th.

pnkfelix · 2024-03-07T15:35:19Z

link to Wednesday's T-lang discussion: https://hackmd.io/U-CKiZx_RKiaANAPXtWf7g#regression-encountered-mutable-pointer-in-final-value-rust121610

wesleywiser · 2024-03-07T15:46:28Z

Discussed in the compiler team triage meeting. The 1.77 release ships on the 21st so there won't really be time for anything to happen after the T-lang meeting on the 20th so we're going to attempt a revert targeted at beta to give T-lang more time to reach a conclusion.

RalfJung · 2024-03-07T17:30:52Z

A full revert will be hard since there's a bunch of follow-on work that landed since then that either further changes the interner, or requires the new interner.

I think it's possible to just turn that error ("encountered mutable pointer in final value") into a future-compat lint. Other than this bug, it can only be triggered by unsound flags/features (const_heap, miri-unleashed). Then we also get an idea of whether there is more code out there that relies on this pattern.

RalfJung · 2024-03-10T13:31:11Z

I was wondering why promotion doesn't trigger this error; turns out the interner simply disables it for promoteds. I guess we have that case covered in our test suite (or in the standard library itself).

I think increasingly my tendency is to fix this by changing &x semantics such that under specific conditions it traverses x to determine whether there is actually an UnsafeCell there. Ideally those conditions are "only for borrows in the tail expression of a const/static that got elevated to module scope". I don't know how easy it is to determine this; one extreme option would be "only in const/static initializers" (but not in const fn bodies).

…ng-ptr-in-final-to-future-incompat-lint, r=wesleywiser Downgrade const eval dangling ptr in final to future incompat lint Short term band-aid for issue rust-lang#121610, downgrading the prior hard error to a future-incompat lint (tracked in issue rust-lang#122153). Note we should not mark rust-lang#121610 as resolved until after this (or something analogous) is beta backported.

cuviper · 2024-03-17T16:45:20Z

This was downgraded in #122204 and beta-backported in #122524.

RalfJung · 2024-03-17T16:48:46Z

We should still track this issue, it's just not a stable-to-beta regression any more.

RalfJung · 2024-03-20T18:07:35Z

This was downgraded in #122204 and beta-backported in #122524.

Tracking issue for that: #122153.

RalfJung · 2024-03-20T18:37:33Z

This was discussed in a T-lang meeting.

There was general sympathy with the t-opsem desire to not do value-based reasoning for interior mutability. But there also wasn't a clear idea for how value-based reasoning could be removed from both promotion and tail-expression checking. The one thing everyone agreed on was to at least do a crater run to see how bad it'd be. Then based on that, maybe there could be an edition-based transition plan.

For this concrete issue, the goal is to monitor the tracking issue of the future compat lint (#122153) to see if anyone speaks up -- to see if there's notable code out there not covered by crater that would regress if we just made the current check a hard error. Depending on that we could either take the hit and break old versions of boa, or do value-based reasoning in const-eval when determining the mutability of shared references (at least in the "root frame" of const-eval, i.e. the initializer expression of the const itself).

traviscross · 2024-03-20T20:07:16Z

@rustbot labels -I-lang-nominated

Thanks to @RalfJung and @oli-obk for joining us in the meeting to talk through this. As @RalfJung mentioned, there are some next steps here, so please renominate when we have more data and a new proposal for how to move forward based on that.

RalfJung · 2024-03-31T11:48:39Z

The crater experiment in #122789 which entirely removed value-based reasoning for interior mutability had almost 4000 regressions. I think we can conclude that this is not an option.^^

Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Feb 25, 2024

Mark-Simulacrum added this to the 1.77.0 milestone Feb 25, 2024

rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 25, 2024

Mark-Simulacrum mentioned this issue Feb 25, 2024

Crater runs for 1.77 #120894

Closed

apiraino mentioned this issue Feb 26, 2024

regression: visibility qualifiers are not permitted #121607

Closed

jieyouxu added the S-needs-repro Status: This issue has no reproduction and needs a reproduction to make progress. label Feb 26, 2024

RalfJung mentioned this issue Feb 28, 2024

Promotion introduces UB into otherwise well-defined code rust-lang/unsafe-code-guidelines#493

Open

RalfJung mentioned this issue Feb 29, 2024

const checking: do not do value-based reasoning for interior mutability #121786

Closed

RalfJung mentioned this issue Feb 29, 2024

const-eval interning: get rid of type-driven traversal #119044

Merged

fmease removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 1, 2024

traviscross mentioned this issue Mar 6, 2024

Extended triage meeting 2024-03-20 rust-lang/lang-team#257

Closed

pnkfelix self-assigned this Mar 7, 2024

This was referenced Mar 7, 2024

Tracking Issue for CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE #122153

Open

Downgrade const eval dangling ptr in final to future incompat lint #122204

Merged

RalfJung mentioned this issue Mar 14, 2024

Tracking issue for &mut T in const contexts (const_mut_refs) #57349

Open

6 tasks

cuviper closed this as completed Mar 17, 2024

RalfJung reopened this Mar 17, 2024

RalfJung removed P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. S-needs-repro Status: This issue has no reproduction and needs a reproduction to make progress. labels Mar 17, 2024

RalfJung changed the title ~~regression: encountered mutable pointer in final value~~ regression: encountered mutable pointer in final value when "outer scope" rule applies in const/static with interior mutability Mar 17, 2024

rustbot removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Mar 20, 2024

RalfJung mentioned this issue Apr 1, 2024

What's the source of immutability for pointers produced by const? rust-lang/unsafe-code-guidelines#502

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

regression: encountered mutable pointer in final value when "outer scope" rule applies in const/static with interior mutability #121610

regression: encountered mutable pointer in final value when "outer scope" rule applies in const/static with interior mutability #121610

Mark-Simulacrum commented Feb 25, 2024 •

edited

Loading

apiraino commented Feb 26, 2024

RalfJung commented Feb 26, 2024 •

edited

Loading

apiraino commented Feb 27, 2024 •

edited

Loading

RalfJung commented Feb 27, 2024

apiraino commented Feb 27, 2024

RalfJung commented Feb 28, 2024

RalfJung commented Feb 28, 2024

RalfJung commented Feb 29, 2024 •

edited

Loading

RalfJung commented Feb 29, 2024

oli-obk commented Feb 29, 2024

RalfJung commented Feb 29, 2024

oli-obk commented Feb 29, 2024

RalfJung commented Feb 29, 2024

oli-obk commented Mar 1, 2024

RalfJung commented Mar 1, 2024 •

edited

Loading

traviscross commented Mar 6, 2024

pnkfelix commented Mar 7, 2024

wesleywiser commented Mar 7, 2024

RalfJung commented Mar 7, 2024 •

edited

Loading

RalfJung commented Mar 10, 2024

cuviper commented Mar 17, 2024

RalfJung commented Mar 17, 2024

RalfJung commented Mar 20, 2024

RalfJung commented Mar 20, 2024

traviscross commented Mar 20, 2024

RalfJung commented Mar 31, 2024

regression: encountered mutable pointer in final value when "outer scope" rule applies in const/static with interior mutability #121610

regression: encountered mutable pointer in final value when "outer scope" rule applies in const/static with interior mutability #121610

Comments

Mark-Simulacrum commented Feb 25, 2024 • edited Loading

apiraino commented Feb 26, 2024

RalfJung commented Feb 26, 2024 • edited Loading

apiraino commented Feb 27, 2024 • edited Loading

RalfJung commented Feb 27, 2024

apiraino commented Feb 27, 2024

RalfJung commented Feb 28, 2024

RalfJung commented Feb 28, 2024

RalfJung commented Feb 29, 2024 • edited Loading

RalfJung commented Feb 29, 2024

oli-obk commented Feb 29, 2024

RalfJung commented Feb 29, 2024

oli-obk commented Feb 29, 2024

RalfJung commented Feb 29, 2024

oli-obk commented Mar 1, 2024

RalfJung commented Mar 1, 2024 • edited Loading

traviscross commented Mar 6, 2024

pnkfelix commented Mar 7, 2024

wesleywiser commented Mar 7, 2024

RalfJung commented Mar 7, 2024 • edited Loading

RalfJung commented Mar 10, 2024

cuviper commented Mar 17, 2024

RalfJung commented Mar 17, 2024

RalfJung commented Mar 20, 2024

RalfJung commented Mar 20, 2024

traviscross commented Mar 20, 2024

RalfJung commented Mar 31, 2024

Mark-Simulacrum commented Feb 25, 2024 •

edited

Loading

RalfJung commented Feb 26, 2024 •

edited

Loading

apiraino commented Feb 27, 2024 •

edited

Loading

RalfJung commented Feb 29, 2024 •

edited

Loading

RalfJung commented Mar 1, 2024 •

edited

Loading

RalfJung commented Mar 7, 2024 •

edited

Loading