1369424 - OOM handler probably doesn't do the right thing

Status: NEW

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[(Away)]

The Rust code has an OOM handler that looks like (https://dxr.mozilla.org/rust/rev/f89d8d184490ecb3cf91f7b6bb7296d649f931ba/src/liballoc/oom.rs#39):

    #[inline(always)]
    pub fn oom() -> ! {
        let value = OOM_HANDLER.load(Ordering::SeqCst);
        let handler: fn() -> ! = unsafe { mem::transmute(value) };
        handler();
    }

But on a Win64 opt build, the disassembly is:

0:000> u xul!alloc::oom::oom 
xul!alloc::oom::oom [C:\projects\rust\src\liballoc\oom.rs @ 26]:
00000001`8052c300 55              push    rbp
00000001`8052c301 4889e5          mov     rbp,rsp
00000001`8052c304 488b059d732603  mov     rax,qword ptr [xul!OOM_HANDLER (00000001`837936a8)]
00000001`8052c30b 0f0b            ud2

In other words, we load up the value of OOM_HANDLER but we never call it.

I wonder if the "!" makes the compiler assume it can elide more than it should?

(Oh, and also, the function didn't get inlined, if that matters. But given bug 1369420, maybe that's a good thing!)

Comment 1

[(Away)]

This was built with:

nightly-x86_64-pc-windows-msvc (default)
rustc 1.19.0-nightly (5de00925b 2017-05-29)

Comment 2

[Alex Crichton [:acrichto]]

Thanks for the heads up! FWIW changing the OOM handler isn't a stable feature today and in general is only here to support libstd's use case. Are you familiar with what Gecko would do for a custom OOM handler? Just curious! If this is a high priority then we can look to stabilizing something in this space soon as well.

As for the disassembly question, that seems quite odd! Are you using the rustup produced nightlies? It looks like the raw object code looks good:


$ objdump -S nightly-2017-05-30-x86_64-pc-windows-msvc/lib/rustlib/x86_64-pc-windows-msvc/lib/liballoc-b9c9173c47c20c41.rlib | grep -A5 'oom3oom'
0000000000000000 <_ZN5alloc3oom3oom17h3058f2f8bf84e7b0E>:
   0:   55                      push   %rbp
   1:   48 83 ec 20             sub    $0x20,%rsp
   5:   48 8d 6c 24 20          lea    0x20(%rsp),%rbp
   a:   48 8b 05 00 00 00 00    mov    0x0(%rip),%rax        # 11 <_ZN5alloc3oom3oom17h3058f2f8bf84e7b0E+0x11>
  11:   ff d0                   callq  *%rax
  13:   0f 0b                   ud2


But maybe the linker is doing something funny?

Comment 3

[(Away)]

(In reply to Alex Crichton [:acrichto] from comment #2)

> Are you familiar with what Gecko would do for a custom OOM handler? Just curious!

Most allocations go through mozjemalloc, which stores the failed allocation's size for the crash reporter to later read, and then MOZ_CRASH()es. A few other allocation sites call into one of the mozjemalloc helpers manually.
https://searchfox.org/mozilla-central/search?q=mozalloc_handle_oom 
https://searchfox.org/mozilla-central/search?q=mozalloc_abort
https://searchfox.org/mozilla-central/search?q=gOOMAllocationSize

> As for the disassembly question, that seems quite odd! Are you using the
> rustup produced nightlies? It looks like the raw object code looks good:

Yeah, that was with rustc nightly 05-29 on my local machine. Today's official Firefox nightly (whatever rust version that uses) looks ok:

xul!alloc::oom::oom [C:\projects\rust\src\liballoc\oom.rs @ 26]:
00007ffd`9dcb2b90 55              push    rbp
00007ffd`9dcb2b91 4883ec20        sub     rsp,20h
00007ffd`9dcb2b95 488d6c2420      lea     rbp,[rsp+20h]
00007ffd`9dcb2b9a 488b0557649202  mov     rax,qword ptr [xul!OOM_HANDLER (00007ffd`a05d8ff8)]
00007ffd`9dcb2ba1 e88a1fffff      call    xul!alloc::oom::default_oom_handler (00007ffd`9dca4b30)
00007ffd`9dcb2ba6 0f0b            ud2

Comment 4

[Alex Crichton [:acrichto]]

Ok thanks for the info! Right now we're planning on eventually stabilizing a method of customizing the global allocator as well as overriding requests to OOM. I've added a comment (https://github.com/rust-lang/rust/issues/32838#issuecomment-306584529) on the relevant issue indicating that we may wish to add a contextual parameter to the OOM function indicating what allocation request failed (e.g. the size that was requested and which failed)

Comment 5

[(Away)]

Regarding the missing 'call' instruction, are you aware of any recent work in the compiler that might cause this to appear on nightly rustc? Maybe related to optimization?

Ideally I'd bisect some compilers but I don't have the cycles right now. :-/

Comment 6

[Alex Crichton [:acrichto]]

Ah no sorry, I'm not sure what change now would have caused that :(

Comment 7

[Bobby Holley (:bholley)]

David, is this something that would block us from shipping? The implications and next steps aren't totally clear to me.

Comment 8

[(Away)]

The bug is that we don't call OOM_HANDLER() and instead continue execution in oom::oom.

Right now this happens to be benign, because:
- OOM_HANDLER() currently doesn't do anything interesting, just a "ud2" to trigger a crash.
- oom::oom itself ends with a "ud2" anyway. I want to say the compiler sticks this in because oom::oom is divergent.

So if OOM_HANDLER ever changes to do more interesting work, then we'd miss out on it. And if the compiler ever stops inserting a ud2 in oom::oom (perhaps because it sees that oom::oom ends with a call to a divergent function anyway) then we might continue execution past the OOM altogether. But this doesn't seem likely.

Comment 9

[Bobby Holley (:bholley)]

Ok. Moving this over to tooling, and it doesn't sound like it's something we need to jump on for now.