We added some linker hacks in bug 1289847 to enable stable versions of Rust to be used on our Mac builds. Once Rust releases 1.12 or thereabouts, said hacks should no longer be necessary and we can remove them.

We require Rust 1.13 now, so we can remove these hacks.

Apparently we still need the hacks and/or the newer linker that we built so we could do the hack: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a830c764082cac7965e31b60d4c60521298ebc80&selectedJob=76867026 Might just be simpler to wait for OS X cross builds to become the One True OS X Build at this point.

:(

I came across an old comment in toolkit/library/moz.build pointing to this bug. Is this still blocked on anything?

It was blocked on comment 2. You could see if https://hg.mozilla.org/try/rev/a830c764082cac7965e31b60d4c60521298ebc80 applies and if everything still builds correctly.

It still fails: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f87c1cdeb8fddfbc918a7809d6909f8ff99b4295 I'll walk away at this point. I was just hoping I could remove some easy dead code. Guess not.

These linker flags cause crashes like bug 1471366 because we can no longer handle native exceptions. Will push a patch to try shortly to see if there is anything left to fix before we can remove these flags.

This seems to build properly (see try build in comment 8). However, I'm not sure what the issues were in the previous attempts at removing this code. Are we in the clear?

... and are we able to remove `-lresolv` from bug 1367932 now, or is this still needed?

Comment on attachment 8992428 [details] [diff] [review] Patch Review of attachment 8992428 [details] [diff] [review]: ----------------------------------------------------------------- I don't recall what the issues were, and of course the try builds are no longer around to examine what those problems were, and I was too shortsighted to copy the errors into a bugzilla comment. =/ I think if it builds locally for you and builds on try, we are good to go. Please be mindful of other people having slightly different OS X builds than you do, and be alert for local-only Mac build bustage happening as a result of this. If you happen to have a couple different Mac environments sitting around, I'd request successful builds if that's not too much trouble, but I'm definitely not going to insist on it. I do think we should just remove the -Wl,-no_compact_unwind bit here, and fix the other things in separate followup bugs, but I'm open to alternative arguments on that front. ::: build/macosx/local-mozconfig.common @@ -16,5 @@ > export DSYMUTIL=$topsrcdir/clang/bin/llvm-dsymutil > - # Use an updated linker. > - ldflags="-B$topsrcdir/cctools/bin" > - export AR=$topsrcdir/cctools/bin/ar > - export RANLIB=$topsrcdir/cctools/bin/ranlib Why are we removing this hunk? I see that it builds OK on try, but it seems better to remove these bits in a separate patch. I guess you're removing it because it landed in the same bug that landed -no_compact-unwind? I don't recall the two patches being connected; they were landed in the same bug for purposes of just getting everything to build. @@ -26,5 @@ > export DSYMUTIL=$topsrcdir/../clang/bin/llvm-dsymutil > - # Use an updated linker. > - ldflags="-B$topsrcdir/../cctools/bin" > - export AR=$topsrcdir/../cctools/bin/ar > - export RANLIB=$topsrcdir/../cctools/bin/ranlib Likewise for this bit. @@ -32,5 @@ > > -# Ensure the updated linker doesn't generate things our older build tools > -# don't understand. > -ldflags="$ldflags -Wl,-no_data_in_code_info" > -export LDFLAGS="$ldflags" Again, not completely sure this needs to be removed in this patch (though given that we have gotten cross-compiles working and have somewhat more control over our build tools, perhaps this bit is not needed as well). ::: toolkit/library/moz.build @@ -76,5 @@ > - # This option should go away in bug 1290972, but we need to wait until > - # Rust 1.12 has been released. > - # We're also linking against libresolv to solve bug 1367932. > - if CONFIG['OS_ARCH'] == 'Darwin': > - LDFLAGS += ['-Wl,-no_compact_unwind,-lresolv'] It looks like -lresolv was added here to deal with issues with earlier versions of Rust; perhaps Rust has fixed whatever problem led to requiring this? The -no_compact_unwind bit was added for similar reasons, so presumably that issue has been solved as well.

Comment on attachment 8992428 [details] [diff] [review] Patch My apologies, I should have pointed out that this is essentially the same patch that landed on try in comment 6. Not knowing the history very well here, I assumed that this was essentially the equivalent of the changeset in your comment 5. I'm happy to change this patch to a one-line fix to LDFLAGS that only removes the `-Wl,-no_compact_unwind` flags if you would prefer. I can also confirm that this builds successfully on macOS 10.13 with the 10.13 SDK as well as on macOS 10.14 with both the 10.13 and 10.14 SDK. Successful builds with the 10.11 SDK appear to be confirmed by our try builds.

Comment on attachment 8992428 [details] [diff] [review] Patch Review of attachment 8992428 [details] [diff] [review]: ----------------------------------------------------------------- Heh, my patch was https://hg.mozilla.org/try/rev/a830c764082cac7965e31b60d4c60521298ebc80 and just removed the local-mozconfig.common bits and didn't touch the -no_compact_unwind bits (possibly because the Rust compiler hadn't been all the way fixed, or something?)! So even I wasn't removing the -no_compact_unwind bits. dmajor went farther in comment 6 with https://hg.mozilla.org/try/rev/f87c1cdeb8fddfbc918a7809d6909f8ff99b4295, producing your patch, but that didn't work then, and I'm not sure what's changed in the interim... Let's do the one-line fix to remove -Wl,-no_compact_unwind (I trust you to produce the correct patch) to ensure that change fixes the issue, and we can fix the other issues in other bugs. A smaller change will also be easier, and less risky, to uplift to Beta if we want to go that route (and I think we do?).

Reduced patch to the one line change, as discussed. Carrying over r+. Try run in comment 14 for good measure. (In reply to Nathan Froyd [:froydnj] from comment #13) > Let's do the one-line fix to remove -Wl,-no_compact_unwind (I trust you to > produce the correct patch) to ensure that change fixes the issue, and we can > fix the other issues in other bugs. A smaller change will also be easier, > and less risky, to uplift to Beta if we want to go that route (and I think > we do?). Done. And yes, I agree that we will probably want to uplift this as far as possible. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/e22ea0aa41ef0fec73d550c09834bcf10e362318 Bug 1290972: Remove linker flags for macOS that are no longer necessary and cause crashes such as bug 1471366 due to an inability to handle native exceptions when these flags are used. r=froydnj

[Tracking Requested - why for this release]: Linker flags added in bug 1289847 disabled all of our native exception handling on macOS. Some of these exceptions are expected and we rely on them for proper behavior. Since we're no longer able to handle them, we crash instead (such as in bug 1471366). We should remove these linker flags and uplift as far as we can.

It looks like the crashes due to these linker flags only started occurring after bug 1270217 landed, which ships in 62. So the situation is not quite as dire as first feared.

Did you want to request uplift to 62?

Comment on attachment 8992511 [details] [diff] [review] Patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1289847 added linker flags that started to cause crashes when bug 1270217 landed. [User impact if declined]: Various crashes, such as bug 1471366 (see list of dependent bugs for a full list). [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Firefox used to ship without the linker flags added in bug 1289847. Furthermore, this patch has been on nightly for about a week with no issues. The only known risk is if beta is built with an older version of Rust that requires these linker flags. This would cause build issues that we would need to resolve. I don't believe this to be the case. [String changes made/needed]: none

OK, correct me if I am wrong but I think we are using 1.24.0 in beta builds. Is that too old? It's 1.27.2 in nightly.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22) > OK, correct me if I am wrong but I think we are using 1.24.0 in beta > builds. Is that too old? It's 1.27.2 in nightly. I believe you're correct. Beta seems to be using 1.24.0. I just built mozilla-beta locally using 1.24.0 and this patch applied. The build was successful and the build runs as expected. I believe this should give us sufficient confidence that this will build as expected on mozilla-beta.

Comment on attachment 8992511 [details] [diff] [review] Patch Crash fix, let's uplift for beta 12.