(updated diff - forgot to include LLD's requested LLD change)

Updated diff:

• flip the emit-compact-unwind-non-canonical flag's default value back to false
• have all the generateCompactUnwind...() return MODE_DWARF (instead of 0, that was a bug).

Updated patch:

• added LLD change + test
• flip the new flag's default value to true to preserve current behaviour (easier to flip it to false and do test clean up separately)

addressed review comment: check for isExternal() symbol

New change: added a flag (default to OFF) to allow emit compact-unwinds for non-canonical personalities.

In D142916#4179641, @int3 wrote:

Reverted. A repro would be helpful!

Seems test failures are related :) PTAL

Seems to cause segfaults in a lot of our builds .... :( Should we possibly revert this while investigating?

Adding an optional flag to request compact-unwind for non-canonical personalities.

In D144999#4172970, @int3 wrote:

So in other words for case #1, we'd end up with one "unused" CU slot.

Not sure why you think so -- we have three canonical personalities, not two, right? __gcc_personality_v0, __gxx_personality_v0, and __objc_personality_v0. Why would we end up with an unused slot?

• reverted to original proposal with some modification (only make objc and c++ personality symbols canonical - leaving out gcc)
• addressed review comments

In D144999#4167531, @int3 wrote:

As I understand, ld64's best workaround for this right now is the -no_compact_unwind flag. This would be trivial for us to support but of course not ideal from a binary size standpoint. However, from a UX standpoint, "getting an error when you add an additional object to the link" is basically the current situation. I think it is more important for us to prioritize perf + implementation simplicity over a somewhat rare UX edge case...

In D144999#4165643, @int3 wrote:

I'm also curious about the motivation here; this wouldn't be the most trivial linker change, especially if we are trying not to regress perf. Moreover, ld64 doesn't do this; I'm not even sure it has any flags that let it support more than 3 personalities. Is it just a linker UX thing?

In D144999#4163617, @int3 wrote:

One issue is that we need to emit ONLY dwarf-unwind for those functions which use nonstandard personalities, while femit-dwarf-unwind enables emitting both compact-unwind and dwarf unwind.

So IMO the ideal design is that we emit both and have the linker fall back to DWARF unwind info if the number of personalities is too large.

P.S: Will add tests/whatnot. This is just a draft proposal to get a feel for how folks feel about this change. Thanks

Thanks!

In D142248#4074581, @thevinster wrote:

Also, could we have a test?

I'm not sure of a good way to test this because I'm trying to test the scenario that passing in -L/usr/local/lib that doesn't exist on the host doesn't result in a warning (and when passing -L/usr/local/lib with -Z, it does result in a warning). AFAICT, I can't test the universally test the latter behavior because that relies on having a non-existent /usr/local/lib directory (which most developer machines will almost be guaranteed to have). I can create a test for the former. It'll be no-op for the most developers since developers will have the directory, but if not, the behavior should be the same.

LG

LG

rescinding this in favour of D140491.
Thanks!

In D140225#4011616, @int3 wrote:

I feel bad for all the back-and-forth with @oontvoo's work, but in general I am in favor of things that improve ease of adoption. @keith's solution seems clean enough too. +1

typo

removed redundant whitespaces

Thanks!

addressed review comments:

• put fileloc's at the beginning of the warn msg
• remove ld64.lld from expected warning msgs in tests
• add comments

reformat

added more tests for missing libaries (-l) too. Also included the files where the bad LC_LINKER_OPTION live to help debugging

updated warning msg in test

In D140225#4006999, @int3 wrote:

Okay, you've convinced me. But just to clarify some of what I said earlier:

You've pointed out that we shouldn't require people to turn off warnings, but this proposed change would not introduce more warnings/errors than whatever already exists

Yes, but it still makes turning on -fatal_warnings harder. And I would like to be able to turn it on for most of our builds. (We are already looking at downgrading certain warnings internally to achieve this, but I would like to minimize the number of local patches.)

Updated diff to implement option #2, whic is to make missing frameworks/libraries warnings instead of errors.

In D140225#4006170, @thakis wrote:

Eh, in doubt I'd do what int3 recommends 😅

Hmm, unfortunately doesn't look like we have a majority here ... I guess how about we go with the option that requires the least API change which is #2 (ie., no new flag):

• not-found-errors from LC_LINKER_OPTION are warnings by default
• you can use the existing -ignore_linker_option to silence the warnings if desired.

Reland #3: temporarily disabled checking cpu type after link due to suspected bug (described above).
(The current test that the link succeeded should be sufficient because
the link would have failed prior to this patch).

In D139572#4003191, @thakis wrote:

This broke tests: http://45.33.8.238/macm1/51156/step_10.txt

reland 2nd attempt

From offline discussions, we have a few options: (in no particular order)

In D140225#4001991, @keith wrote:

I wonder if at this point we should just implement LD64's behavior with an off-by-default flag to be more strict? Specifically they ignore these failures and collect the missing ones and only print about them in the case the link fails

@keith Since you initially implemented these LC_LINKER_OPTION support, perhaps you could help reviewing this?
Thx

In D139572#3988704, @oontvoo wrote:

In D139572#3985254, @thakis wrote:

Thanks for the fast revert! :)

@thakis What kind of Mac do you have? Is it arm64? Just curious since I can't seem to reproduce the crash on my machines ( Mac (x86) and Linux)
Thanks!

In D139572#3985254, @thakis wrote:

Thanks for the fast revert! :)

In D139572#3985251, @thakis wrote:

This still breaks tests: http://45.33.8.238/macm1/50587/step_10.txt

updated tests and reland

In D139572#3984264, @thakis wrote:

Also fails on Windows: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8795277354029432113/+/u/package_clang/stdout?format=raw

Thanks for the revert!

Thx!

addressed review comment + rebase

rebase + fix typo

In D139572#3979427, @int3 wrote:

Is there a test we could write here? How do the other bits in cpusubtype get set?

addressed review comments + added missing tests

(Not sure why git-clang-format is failing for the debian build ... probably unrelated?)

In D138891#3955930, @awilfox wrote:

This was my solution to the related changeset D138830. It simplifies the logic a lot compared to that revision while still ensuring no overflow during calculation. I did not see the other overflows so perhaps it may be better to merge the two in some way. I'm not sure how to do that in Phabricator.

updated err msgs and removed a few asserts

addressed review comment:

• refactor code so that the overflow checks can be done in place
• remove redundant check
• fix off-by-one

• updated type
• separate overflow case to different err msg to make debugging easier

thanks!

address review comments + rebase