[test][lld-macho] Improve LC_FUNCTION_STARTS test coverage

[keith]

Summary

Previously functions that aren't included in the symtab were also
excluded from the function starts. Symbols missing from function starts
degrades the debugger experience in the case you don't have debug info
for them.

[int3]

If I'm understanding correctly, it's not just that Obj-C symbols need to be in LC_FUNCTION_STARTS, but all local symbols in code sections as well. If that's the case, can the test not be objc-specific?

lld/MachO/SyntheticSections.cpp

802–804

803–806

this is existing code, but I think it is redundant... the symbol isLive check above should cover it.

[int3]

Maybe we can reuse the code here: https://github.com/llvm/llvm-project/blob/main/lld/MachO/SyntheticSections.cpp#L952

(should also check if ld64 also skips emitting Lfoo symbols in LC_FUNCTION_STARTS... I would be surprised if it didn't)

[keith]

Improve logic and test

[keith]

In D114275#3143869, @int3 wrote:

If I'm understanding correctly, it's not just that Obj-C symbols need to be in LC_FUNCTION_STARTS, but all local symbols in code sections as well. If that's the case, can the test not be objc-specific?

You're right, updated.

In D114275#3143872, @int3 wrote:

Maybe we can reuse the code here: https://github.com/llvm/llvm-project/blob/main/lld/MachO/SyntheticSections.cpp#L952

The conditional in there is a bit different I guess, are you thinking we should extract the loops into another function and maybe have some callback for each symbol?

(should also check if ld64 also skips emitting Lfoo symbols in LC_FUNCTION_STARTS... I would be surprised if it didn't)

I verified my new test passes with ld64 as well. The relevant logic appears to be here https://github.com/keith/ld64/blob/eddf3fd275669887e70392a1f7d8355222fa295e/src/ld/LinkEdit.hpp#L1978-L1994 although it's not super clear from this snippet that it includes local symbols, but it seems to.

[oontvoo]

lld/MachO/SyntheticSections.cpp

800

Do you happen to have performance data for this change?
seems like it could take a hit here , esp. for cases where the number of input files is large - ie., most of our apps :)

On the other hand, I agree debug experience is important...

[keith]

lld/MachO/SyntheticSections.cpp

800

I've haven't tested this on our large links yet so I'll have to check, I agree, I felt bad writing this

[oontvoo]

lld/MachO/SyntheticSections.cpp

800–813

How would people feel about making this configurable? ( either via making up some new flag or grouping it into an existing one)

[keith]

lld/MachO/SyntheticSections.cpp

800–813

Maybe -no-function-starts is enough for this level of configuration? We actually have that enabled today for our builds for binary size.

[thevinster]

lld/MachO/SyntheticSections.cpp

800

Hmm... I think we can parallelize this. This might offset the need to gate it behind a flag. Although, we can hold off on this until more concrete data on how bad this affects the link speed.

Why can't we add these symbols to the symtab?

[int3]

The conditional in there is a bit different I guess, are you thinking we should extract the loops into another function and maybe have some callback for each symbol?

Yeah I was thinking about a callback, but I'm good with the current implementation too. It's not a lot of code duplication.

I verified my new test passes with ld64 as well.

Nice! The linked code also excludes lfoo and LFoo symbols, though, and I think we should do the same thing here (assuming ld64 does) and test for it.

lld/MachO/SyntheticSections.cpp

800

Why can't we add these symbols to the symtab?

the symtab is for global (aka extern) symbols, which need to have unique names across all object files. local symbols do not have that constraint.

Anyway, this is being done in parallel with other stuff (see finalizeLinkEditSegment()), so its perf impact might actually be negligible in practice

800

I've haven't tested this on our large links yet

We've been using chromium_framework as an ad-hoc benchmark, since it's something that all of us can repro: https://bugs.llvm.org/show_bug.cgi?id=48657#c0

Would be nice to include some numbers for that in the commit message. For running the benchmark, I like to use https://github.com/asayers/cbdr/blob/master/cbdr.md

My usual invocation is cbdr sample "base:~/bench-base/ld64.lld @response.txt" "diff:~/bench-diff/ld64.lld @response.txt" --timeout=300s | tee results.csv | cbdr analyze -s 95

800–813

I would prefer we not add too many config flags. I'm also not sure there's much value in having partially emitted debug info -- I figure you either want or don't want it. -no-function-starts is probably enough for now.

[oontvoo]

lld/MachO/SyntheticSections.cpp

800–813

SG 👍

[oontvoo]

lld/MachO/SyntheticSections.cpp

800

I've haven't tested this on our large links yet

We've been using chromium_framework as an ad-hoc benchmark, since it's something that all of us can repro: https://bugs.llvm.org/show_bug.cgi?id=48657#c0

To be frank, chromium is good for most benchmarking purposes - but it isn't quite representative enough for most of our internal apps (for one, it's much smaller). Unfortunately it's the only thing we have public so ... ¯\_(ツ)_/¯

But anyways, I've run your diff on YT and int3 is right that there's negligible difference in link time.

x ./LLD_YT_released_base.txt
+ ./LLD_TY_released_diff.txt
    N           Min           Max        Median           Avg        Stddev
x   5         13.33         13.72         13.56        13.534    0.15565989
+   5         13.56         13.92         13.79         13.76    0.15636496
No difference proven at 95.0% confidence
(No diff in CPU/SYS time either)

Sorry for the noise here :)

[int3]

(remember to update the diff title/description too)

[keith]

It looks like the logic change from this was committed in https://reviews.llvm.org/D110040, @oontvoo was that intentional or did it sneak in since you tested this? I think we were good to go here, so I'll update this to just include the test, just wanted to make sure I followed what happened

[keith]

Rebase

[oontvoo]

In D114275#3281196, @keith wrote:

It looks like the logic change from this was committed in https://reviews.llvm.org/D110040, @oontvoo was that intentional or did it sneak in since you tested this? I think we were good to go here, so I'll update this to just include the test, just wanted to make sure I followed what happened

Sorry, I'm a bit confused. D110040 was from Sept 2021. This patch was ~Nov 2021. (furthermore D110040 was about the lazy personality symbol, whereas this patch involves the synthetic symbols.)
perhaps quoting the wrong patch? :)

[oontvoo]

P.S: D'oh! Oops! I see it now. Yes, I must've messed up the branches and accidentally included this patch's diff into the personality symbol patch. I'm so sorry!

[keith]

No problem! I think we were good with that version of the change, but lmk if you have any thoughts and I can fix it up.

lld/MachO/SyntheticSections.cpp

800

As far is perf is concerned, when linking the largest lyft ios app, we end up with ~1.1 million function starts and the time difference doesn't seem measurable with this when using this or passing -no_function_starts

[oontvoo]

LG

[MaskRay]

You may consider [test] ... as an alternative to ... NFC.

[test] is clearer that there is no code change.

lld/MachO/SyntheticSections.cpp

 }
 
 FunctionStartsSection::FunctionStartsSection()
     : LinkEditSection(segment_names::linkEdit, section_names::functionStarts) {}
 
 void FunctionStartsSection::finalizeContents() {
   raw_svector_ostream os{contents};
   std::vector<uint64_t> addrs;
-  for (const Symbol *sym : symtab->getSymbols()) {
-    if (const auto *defined = dyn_cast<Defined>(sym)) {
-      if (!defined->isec || !isCodeSection(defined->isec) || !defined->isLive())
-        continue;
-      if (const auto *concatIsec = dyn_cast<ConcatInputSection>(defined->isec))
-        if (concatIsec->shouldOmitFromOutput())
+
+  for (const InputFile *file : inputFiles) {
+    if (auto *objFile = dyn_cast<ObjFile>(file)) {
+      for (const Symbol *sym : objFile->symbols) {
+        if (const auto *defined = dyn_cast_or_null<Defined>(sym)) {
+          if (!defined->isec || !isCodeSection(defined->isec) ||
+              !defined->isLive())
             continue;
           // TODO: Add support for thumbs, in that case
           // the lowest bit of nextAddr needs to be set to 1.
           addrs.push_back(defined->getVA());
         }
       }
+    }
+  }
   llvm::sort(addrs);
   uint64_t addr = in.header->addr;
   for (uint64_t nextAddr : addrs) {
     uint64_t delta = nextAddr - addr;
     if (delta == 0)
       continue;
     encodeULEB128(delta, os);
     addr = nextAddr;

lld/test/MachO/function-starts.s

 # NO-FUNCTION-STARTS: basic-no-function-starts:
 # NO-FUNCTION-STARTS-EMPTY:
 
 # RUN: %lld -lSystem %t/basic.o -no_function_starts -function_starts -o %t/basic-explicit
 # RUN: llvm-objdump --syms %t/basic > %t/objdump
 # RUN: llvm-objdump --macho --function-starts %t/basic >> %t/objdump
 # RUN: FileCheck %s --check-prefix=BASIC < %t/objdump
 
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/local.s -o %t/local.o
+# RUN: %lld -lSystem %t/local.o -o %t/local
+# RUN: llvm-objdump --syms %t/local > %t/objdump
+# RUN: llvm-objdump --macho --function-starts %t/local >> %t/objdump
+# RUN: FileCheck %s --check-prefix=LOCAL < %t/objdump
+
+# LOCAL-LABEL: SYMBOL TABLE:
+# LOCAL-NEXT:  [[#%x,F1:]] l F __TEXT,__text +[Foo bar]
+# LOCAL-NEXT:  [[#%x,F2:]] l F __TEXT,__text _foo
+# LOCAL:       [[#%x,F3:]] g F __TEXT,__text _main
+# LOCAL-LABEL: local:
+# LOCAL:       [[#F1]]
+# LOCAL:       [[#F2]]
+# LOCAL:       [[#F3]]
+
 #--- basic.s
 .section  __TEXT,__text,regular,pure_instructions
 .globl  _f1
 .globl  _f2
 .globl  _main
 _f1:
   retq
 _f2:
   retq
 _main:
   retq
 
 #--- alias.s
 .section  __TEXT,__text,regular,pure_instructions
 .globl  _f1
 .equiv  _f2, _f1
 .globl  _main
 _f1:
   retq
 _main:
   retq
+
+#--- local.s
+.section __TEXT,__text
+"+[Foo bar]":
+  retq
+
+_foo:
+  retq
+
+.globl _main
+_main:
+  retq