Closed Bug 1290972 Opened 4 years ago Closed 2 years ago

remove linker hacks for OS X + Rust builds


(Firefox Build System :: General, defect, P1)



(firefox51 wontfix, firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62+ fixed, firefox63+ fixed)

Tracking Status
firefox51 --- wontfix
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 + fixed
firefox63 + fixed


(Reporter: froydnj, Assigned: spohl)




(1 file, 1 obsolete file)

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.
Blocks: oxidation
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:

Might just be simpler to wait for OS X cross builds to become the One True OS X Build at this point.
Blocks: 1392431
I came across an old comment in toolkit/library/ pointing to this bug. Is this still blocked on anything?
Flags: needinfo?(nfroyd)
It was blocked on comment 2.  You could see if applies and if everything still builds correctly.
Flags: needinfo?(nfroyd)
It still fails:

I'll walk away at this point. I was just hoping I could remove some easy dead code. Guess not.
Product: Core → Firefox Build System
Depends on: 1471366
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.
Assignee: nfroyd → spohl.mozilla.bugs
Priority: -- → P1
Attached patch Patch (obsolete) — Splinter Review
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?
Attachment #8992428 - Flags: review?(nfroyd)
... and are we able to remove `-lresolv` from bug 1367932 now, or is this still needed?
Comment on attachment 8992428 [details] [diff] [review]

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/
@@ -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.
Attachment #8992428 - Flags: review?(nfroyd) → review+
Comment on attachment 8992428 [details] [diff] [review]

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.
Attachment #8992428 - Flags: review+ → review?(nfroyd)
Comment on attachment 8992428 [details] [diff] [review]

Review of attachment 8992428 [details] [diff] [review]:

Heh, my patch was 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, 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?).
Attachment #8992428 - Flags: review?(nfroyd) → review+
Attached patch PatchSplinter Review
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!
Attachment #8992511 - Flags: review+
Attachment #8992428 - Attachment is obsolete: true
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.
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1476508
Did you want to request uplift to 62?
Flags: needinfo?(spohl.mozilla.bugs)
Depends on: 1455364
Depends on: 1471363
Depends on: 1461871
Depends on: 1471504
Depends on: 1467582
Comment on attachment 8992511 [details] [diff] [review]

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
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8992511 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(spohl.mozilla.bugs)
(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.
Flags: needinfo?(spohl.mozilla.bugs)
Comment on attachment 8992511 [details] [diff] [review]

Crash fix, let's uplift for beta 12.
Attachment #8992511 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.