Closed Bug 1458161 Opened 2 years ago Closed 2 years ago

Hook rust OOM handler

Categories

(Firefox Build System :: General, enhancement)

3 Branch
enhancement
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

No description provided.
Nathan, can you review this when you're back?
Comment on attachment 8972250 [details]
Bug 1458161 - Hook rust OOM handler.

https://reviewboard.mozilla.org/r/240920/#review246678

::: toolkit/library/rust/shared/build.rs:5
(Diff revision 1)
> +extern crate rustc_version;
> +use rustc_version::{version, Version};
> +
> +fn main() {
> +    if version().unwrap() < Version::parse("1.27.0-nightly").unwrap() {

We could instead do this version check in rust.configure and enable the feature in gkrust-features.mozbuild:
https://dxr.mozilla.org/mozilla-central/source/toolkit/library/rust/gkrust-features.mozbuild
We still need the build script RUSTC_BOOTSTRAP, with is better to set at the crate level than globally, because it only affects that one crate, not its dependencies.
Comment on attachment 8972250 [details]
Bug 1458161 - Hook rust OOM handler.

https://reviewboard.mozilla.org/r/240920/#review246690

::: toolkit/library/rust/shared/build.rs:8
(Diff revision 1)
> +        // versions of rustc, >= 1.24 < 1.27, that are not subject to
> +        // change the unstable APIs we use here.

"that are not subject to change the unstable APIs"?  I'm not sure what this is communicating.

::: toolkit/xre/nsAppRunner.cpp:5375
(Diff revision 1)
> +// Because rust doesn't handle weak symbols, this function wraps the weak
> +// malloc_handle_oom for it.

If `mozalloc_handle_oom` is weak, what is ensuring that `mozalloc_handle_oom` is always defined?  Or do we just make it weak for Reasons, and then always define it?
Attachment #8972250 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #6)
> Comment on attachment 8972250 [details]
> Bug 1458161 - Hook rust OOM handler.
> 
> https://reviewboard.mozilla.org/r/240920/#review246690
> 
> ::: toolkit/library/rust/shared/build.rs:8
> (Diff revision 1)
> > +        // versions of rustc, >= 1.24 < 1.27, that are not subject to
> > +        // change the unstable APIs we use here.
> 
> "that are not subject to change the unstable APIs"?  I'm not sure what this
> is communicating.

That in the given context, they can be considered "stable" as in, they won't change from under us and break the build.

> ::: toolkit/xre/nsAppRunner.cpp:5375
> (Diff revision 1)
> > +// Because rust doesn't handle weak symbols, this function wraps the weak
> > +// malloc_handle_oom for it.
> 
> If `mozalloc_handle_oom` is weak, what is ensuring that
> `mozalloc_handle_oom` is always defined?  Or do we just make it weak for
> Reasons, and then always define it?

It's always defined. It's weak because it's in the firefox binary, and libxul can't depend on symbols from there.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/b115bb8f62e3
Hook rust OOM handler. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/b115bb8f62e3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
dmajor, could you take a look at the reports I linked in comment 12 and identify what the unresolved frame corresponds to, like if it's a thunk created by the rust compiler for which it doesn't create corresponding debug info? Because the address changes for every build, but ultimately, it looks like all those linked crashes are the same thing, and they should be automatically bucketed together.
Flags: needinfo?(dmajor)
It's a function that does a "call GeckoHandleOOM", so it must be your oom().

I have a suspicion that rust debug info is broken for divergent functions -- see what happened to abort() before this patch in bug 1448868 comment 25 to 27.
Flags: needinfo?(dmajor)
(In reply to David Major [:dmajor] from comment #14)
> It's a function that does a "call GeckoHandleOOM", so it must be your oom().
> 
> I have a suspicion that rust debug info is broken for divergent functions --
> see what happened to abort() before this patch in bug 1448868 comment 25 to
> 27.

Can you file a bug on this? Either here in bugzilla or upstream in the rust-lang repo (or both?)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15)
> (In reply to David Major [:dmajor] from comment #14)
> > It's a function that does a "call GeckoHandleOOM", so it must be your oom().
> > 
> > I have a suspicion that rust debug info is broken for divergent functions --
> > see what happened to abort() before this patch in bug 1448868 comment 25 to
> > 27.
> 
> Can you file a bug on this? Either here in bugzilla or upstream in the
> rust-lang repo (or both?)

I filed bug 1460038.
Blocks: 1465709
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.