Skip to content

Fix ICE when compiling huge allocations with GVN enabled#153481

Open
sgasho wants to merge 1 commit intorust-lang:mainfrom
sgasho:fix-149643-ice
Open

Fix ICE when compiling huge allocations with GVN enabled#153481
sgasho wants to merge 1 commit intorust-lang:mainfrom
sgasho:fix-149643-ice

Conversation

@sgasho
Copy link
Contributor

@sgasho sgasho commented Mar 6, 2026

closes: #149643

I removed PANIC_ON_ALLOC_FAIL flag and introduced ALLOC_FAILURE_REPORTING enum whether to force-print allocation(whether to emit delayed_bug or not in this case) bug or not.

I checked that sample code in the issue has been compiled successfully without ICE.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 6, 2026
@sgasho sgasho marked this pull request as ready for review March 6, 2026 09:26
@rustbot
Copy link
Collaborator

rustbot commented Mar 6, 2026

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 6, 2026
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 6, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 6, 2026

r? @adwinwhite

rustbot has assigned @adwinwhite.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler, mir
  • compiler, mir expanded to 69 candidates
  • Random selection from 16 candidates

@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2026

Please explain the idea behind this PR -- why replace the bool by an enum, and what exactly does the enum do?

///
/// Example use case: To obtain an Allocation filled with specific data,
/// first call this function and then call write_scalar to fill in the right data.
pub fn new(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want three ways of creating an allocation. It's even more strange that the enum that decides which one we use has just two variants.

@sgasho
Copy link
Contributor Author

sgasho commented Mar 6, 2026

why replace the bool by an enum, and what exactly does the enum do?

I'll remove the enum then just rename the bool name. I created three fields in it in my first implementation then removed one. I forgot to convert the enum back to bool 🙇

@adwinwhite
Copy link
Contributor

r? miri

@rustbot rustbot assigned saethlin and unassigned adwinwhite Mar 6, 2026
@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2026

The "obvious" fix here is to just set this to false

const PANIC_ON_ALLOC_FAIL: bool = true;

Is there some reason that doesn't work? I haven't checked all consumers of DummyMachine.

@sgasho
Copy link
Contributor Author

sgasho commented Mar 6, 2026

Is there some reason that doesn't work?

Just setting that to false doesn't work. rustc produces another ICE, saying "exhausted memory during interpretation", which is printed from delayed_bug on Allocation::try_new.

@sgasho
Copy link
Contributor Author

sgasho commented Mar 6, 2026

I think moving delayed_bug out of try_new might work. I'll try to implement it

@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2026

Hm... I think we added that delay_bug because successful queries have to be deterministic, and failing to allocate is not deterministic.

That actually would also apply to the GVN pass -- I'm not sure if it is sound to have the optimization sometimes work and sometimes not depending on whether we have enough RAM.

Cc @cjgillot @oli-obk

@sgasho
Copy link
Contributor Author

sgasho commented Mar 6, 2026

So you mean we should turn this into a normal compilation failure instead of an ICE, and avoid silently continuing compilation when optimization failed due to OOM?

@sgasho
Copy link
Contributor Author

sgasho commented Mar 6, 2026

Ah, looking at gvn.rs, many interpreter errors are dropped via discard_err()? I'm not fully sure about the intended policy there, so I'm less confident in my previous comment.

@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2026

The best way I can imagine for fixing this is to simply not even try to create such large allocations during GVN -- just don't touch values that are too big. But maybe there's a good reason for us not having such a limit currently?

Cc @rust-lang/wg-mir-opt

@saethlin
Copy link
Member

saethlin commented Mar 6, 2026

I think the fix here is to not have GVN try to ignore allocation failures. const-eval reports them, and GVN should too.

Yes GVN ignores a lot of errors, IIRC that's because MIR bodies may have type-checking or other kinds of errors in them when they are fed into the MIR opt pipeline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ICE interpreter ran out of memory

5 participants