[lld-macho] Don't put entries with less than 2 usages into the common table.

[oontvoo]

Summary

This matches LD64's handling.

[oontvoo]

lld/test/MachO/fold-common-encoding.s

2

This is only 1 test at the moment, but I have another patch coming which needs two input files - so using split-file in this patch to make the diff easier

17

This used to include the _exception1 entry, too.

[int3]

Does this behavior difference really matter? We're not saving any bytes with this, right

[int3]

That said, I'm not super opposed to making this change. But I'm wondering if we could at least reuse the compact-unwind.s test instead of adding a new file -- this seems like a lot of additional testing for a trivial behavior change. Or will your upcoming patch not fit into that test file?

[oontvoo]

In D136374#3872249, @int3 wrote:

Does this behavior difference really matter? We're not saving any bytes with this, right

This change isn't about saving the bytes - but rather, theoretically the size of the common-encoding table could change the paging logic. I'm still chasing down the missing exception-handling-code bug and noticed that this is one of a few differences.... just trying to narrow down the issue.

But I'm wondering if we could at least reuse the compact-unwind.s test instead of adding a new file -- this seems like a lot of additional testing for a trivial behavior change. Or will your upcoming patch not fit into that test file?

it could kind of fit into compact-unwind.s but will need additional sub-files (or modifying the existing one to have more symbols to demonstrate this change), which I think doesn't really help with reducing complexity.
What is the concern? IMHO, it's easier to debug failures in general when there are multiple separate test files with single-purpose testings rather than trying to pack as many related things as possible into one giant file 🤔

[int3]

What is the concern? IMHO, it's easier to debug failures in general when there are multiple separate test files with single-purpose testings rather than trying to pack as many related things as possible into one giant file 🤔

It's a tension between that vs being able to easily understand what is being tested -- one of the purposes of a test is to serve as a declarative spec of behavior, which are usually easier to understand than the imperative code -- partly because they are terser.

Other reasons include test suite running time, plus the amount of effort needed to update tests when LLD's behavior changes.

Given the tradeoffs, it's a matter of striking a balance. Adding a whole test file to cover a single if statement seems kind of heavy. If we did that for every other if... we would have a lot more tests. It's not always necessarily *wrong* -- if e.g. there's a tricky edge case that only happens for very carefully constructed inputs, then sure, having a separate test file makes sense even if the fix is a single if. But this particular test doesn't seem to fall into that category.

(Also, couldn't the current test be simplified? It seems like you could just vary the .cfi_def_cfa_offset value to get different encodings. Doesn't seem like _main and _exception1 are necessary...)

Plus I noted that the compact-unwind.s test doesn't really test for encodings at all. I'm thinking we could add CHECKs for that, plus a single _unique_encoding function that ensures its uniqueness via the CFA offset.

Feel free to push back, but these are my reasons :)

[Roger]

lld/MachO/UnwindInfoSection.cpp

513

Could we include in the comment that the motivation for this if-statement is to match what ld64 is doing?

[oontvoo]

addressing review comments - so pulling this off the review queue - sorry for the delay in response, was OOO for 2 weeks

lld/MachO/UnwindInfoSection.cpp

void UnwindInfoSectionImpl::finalize() {
   encodePersonalities();
 
   // Count frequencies of the folded encodings
   EncodingMap encodingFrequencies;
   for (size_t idx : cuIndices)
     encodingFrequencies[cuEntries[idx].encoding]++;
 
   // Make a vector of encodings, sorted by descending frequency
-  for (const auto &frequency : encodingFrequencies)
+  for (const auto &frequency : encodingFrequencies) {
+    // Don't put encodings with only 1 usage into the table.
+    if (frequency.second < 2) continue;
+    
     commonEncodings.emplace_back(frequency);
+  }
+  
   llvm::sort(commonEncodings,
              [](const std::pair<compact_unwind_encoding_t, size_t> &a,
                 const std::pair<compact_unwind_encoding_t, size_t> &b) {
                if (a.second == b.second)
                  // When frequencies match, secondarily sort on encoding
                  // to maintain parity with validate-unwind-info.py
                  return a.first > b.first;
                return a.second > b.second;

lld/test/MachO/fold-common-encoding.s

+# REQUIRES: x86
+# RUN: rm -rf %t; split-file %s %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin19.0.0 %t/main.s  -o %t/main.o
+# RUN: %lld -arch x86_64 -lSystem -lc++ %t/main.o  -o %t/a.out  
+
+# RUN: llvm-objdump --macho  --unwind-info %t/a.out | FileCheck %s
+
+CHECK: Contents of __unwind_info section:
+CHECK:  Version:                                   0x1
+CHECK:  Common encodings array section offset:     0x1c
+CHECK:  Number of common encodings in array:       0x1
+CHECK:  Personality function array section offset: 0x20
+CHECK:  Number of personality functions in array:  0x1
+CHECK:  Index array section offset:                0x24
+CHECK:  Number of indices in array:                0x2
+CHECK:  Common encodings: (count = 1)
+CHECK:    encoding[0]: 0x[[#%x, COMMON_ENC:]]
+CHECK:  Second level indices:
+CHECK:    Second level index[0]: offset in section={{.+}}
+CHECK:      Page encodings: (count = 1)
+CHECK:        encoding[1]: 0x[[#%x, SINGLE_ENC:]]
+CHECK:      [0]: function offset=0x{{.+}}, encoding[0]=0x[[#%.8x, COMMON_ENC]]
+CHECK:      [1]: function offset=0x{{.+}}, encoding[0]=0x[[#%.8x, COMMON_ENC]]
+CHECK:      [2]: function offset=0x{{.+}}, encoding[1]=0x[[#%.8x, SINGLE_ENC]]
+
+
+#--- main.s
+.globl _main
+
+.globl _f
+.p2align 4, 0x90
+_f:
+  .cfi_startproc
+  subq $3272, %rsp
+  .cfi_def_cfa_offset 3280
+  addq $3272, %rsp
+  retq
+  .cfi_endproc
+
+.globl _ff
+.p2align 4, 0x90
+_ff:
+  .cfi_startproc
+  subq $3272, %rsp
+  .cfi_def_cfa_offset 3280
+  retq
+  .cfi_endproc
+
+.text
+.p2align 2
+_main:
+  .cfi_startproc
+  callq  _f 
+  callq _ff
+  .cfi_personality 155, ___gxx_personality_v0
+  .cfi_lsda 16, _exception1
+  .cfi_def_cfa_offset 16
+  ret
+  .cfi_endproc
+  ret
+
+
+.globl _exception1
+.section __TEXT,__gcc_except_tab
+_exception1:
+  .space 1
+
+        
+        
+.subsections_via_symbols