Status

enhancement
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

3 Branch
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

a year ago
No description provided.
Comment hidden (mozreview-request)
Assignee

Comment 2

a year ago
Nathan, can you review this when you're back?

Comment 3

a year 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

a year 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

a year 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)
Assignee

Comment 8

a year 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 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b115bb8f62e3
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee

Comment 13

a year 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)
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.
Assignee

Updated

a year ago
Blocks: 1465709
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.