Closed Bug 1289847 Opened 4 years ago Closed 4 years ago

Pass -Wl,-no_compact_unwind on mac builds with --enable-rust

Categories

(Firefox Build System :: General, defect)

Unspecified
macOS
defect
Not set

Tracking

(firefox50 affected, firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: rillian, Assigned: froydnj)

References

Details

Attachments

(3 files)

Work around bug 1188030 by passing -no_compact_unwind (the typical work-around) to the linker on Mac. We need this to unblock updates of the rust toolchain on that platform so we can move forward with other work.
I tried a local build with rust 1.10.0-stable and the following lines in mozconfig:

  ac_add_options LDFLAGS="-Wl,-no_compact_unwind"
  mk_add_options LDFLAGS="-Wl,-no_compact_unwind"

It really seems to require it be set in the parent mach process environment. I'm not sure how to do that. Maybe with mozharness for integration, and punt on making local builds work?

Rail, any suggestions how we can set an environment variable for mac builds in automation?
Flags: needinfo?(rail)
(In reply to Ralph Giles (:rillian) needinfo me from comment #1)
> Rail, any suggestions how we can set an environment variable for mac builds
> in automation?

Use export?

http://dxr.mozilla.org/mozilla-central/source/build/unix/mozconfig.asan#5

Alternatively, set it in configury, where everybody will benefit, and nobody needs to add weird hacks.
Flags: needinfo?(rail)
I tried this with the rust repack that was uploaded in bug 1285323 for Macs (the bug is for Linux, but it looks like Rust repacks for every platform were uploaded?):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b44e2c0f042

Looks like we run into linker errors:

 08:16:28     INFO -  Assertion failed: (_mode == modeFinalAddress), function finalAddress, file /SourceCache/ld64/ld64-123.2.1/src/ld/ld.hpp, line 573.
 08:16:28     INFO -  0  0x10b0d371c  __assert_rtn + 76
 08:16:28     INFO -  1  0x10b14c01c  ld::tool::OutputFile::addressOf(ld::Internal const&, ld::Fixup const*, ld::Atom const**) + 172
 08:16:28     INFO -  2  0x10b14ea25  ld::tool::OutputFile::applyFixUps(ld::Internal&, unsigned long long, ld::Atom const*, unsigned char*) + 3909
 08:16:28     INFO -  3  0x10b14af70  ld::tool::OutputFile::writeOutputFile(ld::Internal&) + 816
 08:16:28     INFO -  4  0x10b143ab9  ld::tool::OutputFile::write(ld::Internal&) + 153
 08:16:28     INFO -  5  0x10b0d3caa  main + 1178
08:16:28 INFO - clang-3.8: error: linker command failed with exit code 1 (use -v to see invocation)

ISTR this linker error coming up in a different context; Ralph, does it ring a bell?  I thought it had something to do with our need to upgrade the Mac toolchain we were using...
Flags: needinfo?(giles)
Ah, it's similar to bug 1165528...but upgrading our Rust version resolved things there. :(
...and the linker error only turns up in debug builds. :(
Yeah, it sounds familiar. I'd been assuming the various errors were different manifestations of the same issue. I had similar problems with the the 1.12-nightly build on try which works on my local MacOS X 10.10.5.

Can we package a newer linker through tooltool?
Flags: needinfo?(giles)
BTW, your commit message doesn't match my understanding of https://github.com/rust-lang/rust/pull/34832

I thought the issue was that prior to 1.12, rust was _reusing_ the C++ eh_personality routines in a way that presented confusing different manifestations, and the fix was to provide entirely separate, rust-specific implementations. Am I confused about the underlying cause?
(In reply to Ralph Giles (:rillian) needinfo me from comment #6)
> Yeah, it sounds familiar. I'd been assuming the various errors were
> different manifestations of the same issue. I had similar problems with the
> the 1.12-nightly build on try which works on my local MacOS X 10.10.5.
> 
> Can we package a newer linker through tooltool?

We don't get the linker through tooltool, AIUI; we get the linker through whatever XCode is installed on the machines.  Compiling the Mac linker is...not straightforward, but possibly doable since we compile our own clang?
Yes, I meant package up the linker from a newer xcode binary, or do a darwin build of whatever the mac-cross build uses. I suppose it might be hard to find one new enough to avoid the error but old enough to run on 10.7.
For the record, using export in mozconfig worked on my local mac, where ac_add_options + mk_add_options didn't.
We build our own linker for the cross-mac builds, it's probably not any harder to build it for our native mac builds.
Relevant information about building ld64 on osx:
https://bugzilla.mozilla.org/show_bug.cgi?id=1262781#c2
The ld that we use for Mac builds is old (Xcode circa OS X 10.7), and
also crashes in various ways when we try to use newer Rust versions
and/or pass options to make the linker work with newer Rust versions.
To mitigate this, let's build with a newer linker, compiled from:

https://github.com/tpoechtrager/cctools-port

We use this port, rather than the packages from opensource.apple.com,
because the packages from Apple have decidely non-intuitive build
systems, and require some hacking to get to build.  This port, in
contrast, is simply built with:

CFLAGS='-mcpu=generic -mtune=generic' ./configure --target=x86_64-apple-darwin11
env MACOSX_DEPLOYMENT_TARGET=10.7 make

and the resulting x86_64-apple-darwin11-ld is renamed as 'ld' and
packaged up for automation's purposes.

However, since this linker is newer, it also produces bits of Mach-O
that our older build tools don't understand.  Fortunately, we can pass
appropriate options to the linker to turn off generation of those Mach-O
bits.
Attachment #8776109 - Flags: review?(mshal)
Current stable versions of Rust use two Rust-specific personality
routines to perform exception handling, which empirically does not play
well with the Mac linker's optimizations for using compact unwind
formats. Nightly Rust has solved this issue, but for now, we'll have to
use -no_compact_unwind to disable the linker optimization.  The size
impact is negligible (0.02%) and will be going away once nightly Rust
becomes stable.
Attachment #8776110 - Flags: review?(mshal)
Attachment #8776109 - Flags: review?(mshal) → review+
Comment on attachment 8776110 [details] [diff] [review]
work around Mac linking failures when Rust is enabled

>Current stable versions of Rust use two Rust-specific personality
>routines to perform exception handling, which empirically does not play
>well with the Mac linker's optimizations for using compact unwind
>formats. Nightly Rust has solved this issue, but for now, we'll have to
>use -no_compact_unwind to disable the linker optimization.  The size
>impact is negligible (0.02%) and will be going away once nightly Rust
>becomes stable.

Are you saying this moz.build change will go away? Or just the performance impact of using this flag? If the former, can you file a follow-up and put the bug number in a comment in the moz.build file?
Attachment #8776110 - Flags: review?(mshal) → review+
Even if it's trivial, it'd be great to have a script in-tree somewhere to reproduce the ld64 build.
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f22763859aae
part 1 - use a newer ld for Mac builds; r=mshal
https://hg.mozilla.org/integration/mozilla-inbound/rev/74922f9ce5c6
part 2 - work around Mac linking failures when Rust is enabled; r=mshal
See Also: → 1291028
(In reply to Wes Kocher (:KWierso) from comment #18)
> Backed out for static build bustage in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a000a5db67aa
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=33032550&repo=mozilla-
> inbound

Sigh, I *really* wish that when you pushed to try -p $PLATFORM, it would run *all* the builds for that platform, so you didn't have to screw around with -p $PLATFORM_WITH_MAGIC_BUILD_TYPE.

I'm unsure of why this is failing...this is just building on a Mac like our other Mac builds!
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #19)
> I'm unsure of why this is failing...this is just building on a Mac like our
> other Mac builds!

Our static analysis builds use a different manifest than our regular builds, so the static analysis build was not downloading the new linker...but it was passing flags that only the new linker understood.

The fix is straightforward: add the cctools tarball to the static analysis tooltool manifest.  Assuming that's really the only problem, everything here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c653626358b

should come back green and I will reland.  I don't think I need to ask for re-review of the manifest update.
If I could pick the right git revisions to push to try, that'd be good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eac8f15a6148
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4cac318f42d
part 1 - use a newer ld for Mac builds; r=mshal
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cda786e6bb6
part 2 - work around Mac linking failures when Rust is enabled; r=mshal
https://hg.mozilla.org/mozilla-central/rev/e4cac318f42d
https://hg.mozilla.org/mozilla-central/rev/3cda786e6bb6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
This broke builds on comm-central as the paths there are slightly different due to the unfortunate split build system.
Attachment #8777618 - Flags: review?(mshal)
Comment on attachment 8777618 [details] [diff] [review]
Followup to fix linker path for comm-* builds

Review of attachment 8777618 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!
Attachment #8777618 - Flags: review?(mshal) → review+
Pushed by aleth@instantbird.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/175e41ede356
Followup to fix linker path for comm-* builds. r=mshal
https://hg.mozilla.org/comm-central/rev/43b0a8b09a294030ab799734e5c2a41d5c8e727a
Bug 1289847 - Followup to fix linker path for comm-* builds. r=mshal a=aleth CLOSED TREE DONTBUILD
Product: Core → Firefox Build System
Blocks: 1471366
You need to log in before you can comment on or make changes to this bug.