Closed
Bug 1458161
Opened 6 years ago
Closed 6 years ago
Hook rust OOM handler
Categories
(Firefox Build System :: General, enhancement)
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file)
No description provided.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
Nathan, can you review this when you're back?
Comment 3•6 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 4•6 years ago
|
||
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 hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
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)
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8972250 [details]
Bug 1458161 - Hook rust OOM handler.
https://reviewboard.mozilla.org/r/240920/#review246840
Attachment #8972250 -
Flags: review+
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/b115bb8f62e3
Hook rust OOM handler. r=froydnj
Comment 11•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 12•6 years ago
|
||
Evidence this worked:
https://crash-stats.mozilla.com/signature/?build_id=%3E%3D20180502220059&signature=OOM%20%7C%20large%20%7C%20mozalloc_abort%20%7C%20mozalloc_handle_oom%20%7C%20xul.dll%400x5fa9f1%20%7C%20static%20struct%20webrender%3A%3Aprim_store%3A%3APrimitiveIndex%20webrender%3A%3Aprim_store%3A%3APrimitiveStore%3A%3Aadd_primitive&date=%3E%3D2018-05-01T19%3A12%3A04.000Z&date=%3C2018-05-08T19%3A12%3A04.000Z
https://crash-stats.mozilla.com/signature/?build_id=%3E%3D20180502220059&_sort=-date&signature=OOM%20%7C%20large%20%7C%20mozalloc_abort%20%7C%20mozalloc_handle_oom%20%7C%20xul.dll%400x5fc4c1%20%7C%20static%20struct%20webrender%3A%3Aprim_store%3A%3APrimitiveIndex%20webrender%3A%3Aprim_store%3A%3APrimitiveStore%3A%3Aadd_primitive&date=%3E%3D2018-05-01T10%3A12%3A04.000Z&date=%3C2018-05-08T10%3A12%3A04.000Z
Assignee | ||
Comment 13•6 years ago
|
||
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)
Comment 14•6 years ago
|
||
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)
Comment 15•6 years ago
|
||
(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?)
Comment 16•6 years ago
|
||
(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.
Updated•6 years ago
|
Version: Version 3 → 3 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•