[PATCH] D114275: [lld-macho] Include Objective-C functions in LC_FUNCTION_STARTS

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

> 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.



================
Comment at: lld/MachO/SyntheticSections.cpp:800
+
+  for (const InputFile *file : inputFiles) {
+    if (auto *objFile = dyn_cast<ObjFile>(file)) {
----------------
thevinster wrote:
> keith wrote:
> > oontvoo wrote:
> > > keith wrote:
> > > > oontvoo wrote:
> > > > > 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... 
> > > > > 
> > > > I've haven't tested this on our large links yet so I'll have to check, I agree, I felt bad writing this
> > > How would people feel about making this configurable? ( either via making up some new flag or grouping it into an existing one)
> > Maybe -no-function-starts is enough for this level of configuration? We actually have that enabled today for our builds for binary size.
> 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?
> 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


================
Comment at: lld/MachO/SyntheticSections.cpp:800
+
+  for (const InputFile *file : inputFiles) {
+    if (auto *objFile = dyn_cast<ObjFile>(file)) {
----------------
int3 wrote:
> int3 wrote:
> > thevinster wrote:
> > > keith wrote:
> > > > oontvoo wrote:
> > > > > keith wrote:
> > > > > > oontvoo wrote:
> > > > > > > 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... 
> > > > > > > 
> > > > > > I've haven't tested this on our large links yet so I'll have to check, I agree, I felt bad writing this
> > > > > How would people feel about making this configurable? ( either via making up some new flag or grouping it into an existing one)
> > > > Maybe -no-function-starts is enough for this level of configuration? We actually have that enabled today for our builds for binary size.
> > > 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?
> > > 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
> 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.
> 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`


================
Comment at: lld/MachO/SyntheticSections.cpp:800-813
+  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;
----------------
int3 wrote:
> thevinster wrote:
> > keith wrote:
> > > oontvoo wrote:
> > > > keith wrote:
> > > > > oontvoo wrote:
> > > > > > 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... 
> > > > > > 
> > > > > I've haven't tested this on our large links yet so I'll have to check, I agree, I felt bad writing this
> > > > How would people feel about making this configurable? ( either via making up some new flag or grouping it into an existing one)
> > > Maybe -no-function-starts is enough for this level of configuration? We actually have that enabled today for our builds for binary size.
> > 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?
> > 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
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114275/new/

https://reviews.llvm.org/D114275