lgtm

Thanks :D

Thank you! :D

I agree it's more readable but following ld64 is more useful for us 😅

Thanks for looking into this!

Thanks!

update

I think both linkers expect the EH frame section to be a contiguous array of CFI entries. Using the .space 1000 thing causes ld64 to crash and LLD to silently drop all subsequent EH frames

Wanna stamp D147230 too? Otherwise you'll see spurious warnings when I land this :p

P.S: Wait, this should be stacked on top of the previous patch, yes? (the one that supported emitting the warnings?)

@oontvoo wanna test this on the previous crashes you were seeing before I land it?

JFYI I have a repro of this -- will put up the fix soon (I found other issues along the way)

move logic to handleExplicitExports; also add test to show -unexported_symbol has no effect on dylibs

I don't really think a test for that is super necessary, the -exported_symbols_list is simply command-line sugar over -exported_symbol, and I don't think every variation of how -exported_symbol gets used needs to have a corresponding _list test...

I'm trying to add similar support to lld-macho, hence the question :)

Turns out D145455: [lld-macho] Coalesce local symbol aliases along with their aliased weak def is a massive perf win because the old 'solution' was actually a perf regression... heh

update commit message with perf numbers

Thanks!

address comment

make static

Oh, I think I know what is happening -- this diff in my local repo is rebased upon D145455: [lld-macho] Coalesce local symbol aliases along with their aliased weak def, which switches up the order of the symbols. I will land them all EOW when I have had a chance to run benchmarks on the other diff

add comments

fix double-unwindEntry assertion

Reverted. A repro would be helpful!

clang-format

update

As promised: D145455: [lld-macho] Coalesce local symbol aliases along with their aliased weak def

TODO: add tests for the non-subsections-via-symbols case

With a goal of simplicity, I think we should just go back to the earlier proposal: Clang emits all functions with a personality other than the 3 canonical personalities as DWARF unwind ONLY. Agreed we can have a flag to opt-into compact unwind here, if a user knows it'll be ok in their build.

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

Thanks!! I think we should follow LLD-ELF whenever reasonable/possible, so I like this better

Thanks Keith!

Huh, I'm surprised that setting -object_path_lto has a functional difference; I thought it was only to make debugging the intermediate object files easier. Did you figure out why?

Yeah sorry been a bit swamped but I'll try to put up something by Monday. Basically my plan is to sort the symbols such that weak defs appear after all other definitions, so that when we do weak def coalescing we can bring along all the other local symbols as well. I think it should work, just a matter of how messy the implementation is...

Just noticed I'd messed up the author attribution in the commit. Sorry ):

Oops, I'd glossed over that in the commit message. (Also, wow, that is a *long* bug discussion.)

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?

For gxx_personality_v0 and objc_personality_v0 (but not __gcc_personality_v0), emit only compact unwind (or only DWARF where compact-unwind isn't possible) for each function -- not both. (This saves object-file size/build time for the most common languages).

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.

Why not let the end user decide whether to enable DWARF unwind by passing -femit-dwarf-unwind?

1.5% seems kinda significant. I wonder if I should rethink my approach in D139069... let me try some things