[lld-macho] Ignore debug symbols for now.

[oontvoo]

Summary

Details: see https://bugs.llvm.org/show_bug.cgi?id=50812

[int3]

test plz

(I'm genuinely curious about how these symbols are typically generated)

[oontvoo]

In D105210#2851593, @int3 wrote:

test plz

Added test (with a checked-in .o file, unless someone can tell me a way to produce it within the test :) lld doesn't support -r yet )

(I'm genuinely curious about how these symbols are typically generated)

You can produce it with:

echo "int main() {return 1;}" > test.cc
clang -c -g -o test.o test.c
ld -r -o test2.o test.o;
ld64.lld.darwinnew -platform_version macos 11.3 11.0  -arch x86_64 test2.o

[oontvoo]

updated diff

[int3]

Ah, ld -r... that bit us before in D104502: [lld-macho] Handle non-extern symbols marked as private extern :)

with a checked-in .o file, unless someone can tell me a way to produce it within the test

Let's use yaml2obj instead. Other LLVM tool tests use raw .o files sometimes, but I get the sense that it's a legacy decision and people are pushing back on further additions.

As a bonus, you can inline the YAML within the test file via split-file --no-leading-lines, see e.g. dylink-ordinal.s.

lld/test/MachO/bug_50812.s

6

you're missing a RUN: here (which the test infra has rightly noted)

[int3]

lld/MachO/UnwindInfoSection.cpp

154–164 ↗

(On Diff #356017)

this should be in a separate diff, and also needs a test :)

[oontvoo]

added missing RUN

[oontvoo]

lld/MachO/UnwindInfoSection.cpp

154–164 ↗

(On Diff #356017)

Well, these are separate issue, yes, but without this change the link wouldn't succeed, ie., it'd trip on the assert on line 156 (on the LHS of the diff)
IOWs, all of these changes are necessary for the test to pass

Since these are small enough, can you just let put them in one diff :)

[int3]

lld/MachO/UnwindInfoSection.cpp

154–164 ↗

(On Diff #356017)

could we not simplify bug_50812.o so that it doesn't trip the assert, then? I think it should be fairly straightforward if you convert it to YAML (or even just hand-craft a YAML test case from scratch) -- one of the advantages of yaml tests is that you can delete most of the extraneous stuff and just test the one code path you're interested in.

156–157 ↗

(On Diff #356017)

this isn't necessary, cast<> does the assert in debug mode

[oontvoo]

use yaml

[oontvoo]

removed unnecessary assert()

[int3]

lld/test/MachO/bug_50812.s

44–111

try and see if you can delete one or both of these sections and get yaml2obj to accept it. maybe that will also allow the test to pass w/o the Symbol referent changes being in this diff

118–130

should be able to rm these (just remember to update ncmds above)

in general yaml test cases should be as small as possible. The goal is not to create a working binary, just to exercise the relevant LLD code paths

144

for clarity, can we add a ## comment on whichever symbols are N_STAB, and remove as many of the other symbols as possible?

[oontvoo]

split changes in UnwindInfoSection to a separate diff + update test accordingly

[oontvoo]

lld/test/MachO/bug_50812.s

44–111

(still need the eh_frame)

[oontvoo]

rebase

[oontvoo]

Thanks!

[dblaikie]

lld/test/MachO/bug_50812.s

6

I fixed an issue with this test in bf7f846b683077a8adcb229624082e525870229b by adding -o %t to the lld RUN line - without that it tries to write the linked executable to the current working directory which may not be writable/may not be suitable to write to. (might be the test source directory, for instance)

But also: This test should probably verify something about the output of the lld execution - currently this test checks for "does anything so long as it exits 0" - perhaps it should check the specific behavior that was untested previously? What specific features of the resulting binary were untested previously/hidden behind this incorrect error?

[oontvoo]

lld/test/MachO/bug_50812.s

6

I fixed an issue with this test in bf7f846b683077a8adcb229624082e525870229b by adding -o %t to the lld RUN line - without that it tries to write the linked executable to the current working directory which may not be writable/may not be suitable to write to. (might be the test source directory, for instance)

Thanks! (Yeah, I forgot this had caused test failures before when someone was doing an integrate)

But also: This test should probably verify something about the output of the lld execution - currently this test checks for "does anything so long as it exits 0" - perhaps it should check the specific behavior that was untested previously? What specific features of the resulting binary were untested previously/hidden behind this incorrect error?

This is not a complete binary (because the yaml was trimmed down to make the test small) so the resulting binary wouldn't have run.
The specific bug we were testing was the fact that debug symbols being present in .o files, which wasn't anticipated before.

[dblaikie]

lld/test/MachO/bug_50812.s

6

This is not a complete binary (because the yaml was trimmed down to make the test small) so the resulting binary wouldn't have run.

Tests don't run linked binaries anyway - they might not be built for the host target, etc.

I meant testing some properties of the linked binary - same as other lld tests do. (objdumping them to check certain features are present, etc)

The specific bug we were testing was the fact that debug symbols being present in .o files, which wasn't anticipated before.

So it sounds like maybe this test should be checking that the resulting binary doesn't have any of these debug symbols present?

[MaskRay]

I left two comments about the test bug_50812.s on https://reviews.llvm.org/rGbf7f846b683077a8adcb229624082e525870229b#1009321

I think someone may need to investigate how to improve ergonomics of Mach-O yaml2obj. Currently many properties need to be specified which makes a test very verbose and often hides interesting part among boilerplate.
The "address:" is also an issue. Inserting new code can invalid all address fields, making test updating unwieldly.

lld/MachO/InputFiles.cpp

void ObjFile::parseSymbols(ArrayRef<typename LP::section> sectionHeaders,
                            const char *strtab, bool subsectionsViaSymbols) {
   using NList = typename LP::nlist;
 
   // Groups indices of the symbols by the sections that contain them.
   std::vector<std::vector<uint32_t>> symbolsBySection(subsections.size());
   symbols.resize(nList.size());
   for (uint32_t i = 0; i < nList.size(); ++i) {
     const NList &sym = nList[i];
+
+    // Ignore debug symbols for now.
+    // FIXME: may need special handling.
+    if (sym.n_type & N_STAB)
+      continue;
+
     StringRef name = strtab + sym.n_strx;
     if ((sym.n_type & N_TYPE) == N_SECT) {
       SubsectionMap &subsecMap = subsections[sym.n_sect - 1];
       // parseSections() may have chosen not to parse this section.
       if (subsecMap.empty())
         continue;
       symbolsBySection[sym.n_sect - 1].push_back(i);
     } else {

lld/test/MachO/bug_50812.s

+# REQUIRES: x86
+## FIXME: This yaml is from an object file produced with 'ld -r'
+##        Replace this with "normal" .s test format once lld supports `-r`
+
+# RUN: yaml2obj %s -o %t.o
+# RUN: %lld -lSystem -platform_version macos 11.3 11.0 -arch x86_64 %t.o
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           2
+  sizeofcmds:      384
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         312
+    segname:         ''
+    vmaddr:          0
+    vmsize:          120
+    fileoff:         448
+    filesize:        120
+    maxprot:         7
+    initprot:        7
+    nsects:          2
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        size:            18
+        offset:          0x000001C0
+        align:           4
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         554889E5C745FC00000000B8010000005DC3
+      - sectname:        __eh_frame
+        segname:         __TEXT
+        addr:            0x0000000000000018
+        size:            64
+        offset:          0x000001D8
+        align:           3
+        reloff:          0x00000238
+        nreloc:          4
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         1400000000000000017A520001781001100C0708900100002400000004000000F8FFFFFFFFFFFFFF120000000000000000410E108602430D0600000000000000
+        relocations:
+          - address:         0x0000001C
+            symbolnum:       0
+            pcrel:           false
+            length:          2
+            extern:          true
+            type:            5
+            scattered:       false
+            value:           0
+          - address:         0x0000001C
+            symbolnum:       1
+            pcrel:           false
+            length:          2
+            extern:          true
+            type:            0
+            scattered:       false
+            value:           0
+          - address:         0x00000020
+            symbolnum:       1
+            pcrel:           false
+            length:          3
+            extern:          true
+            type:            5
+            scattered:       false
+            value:           0
+          - address:         0x00000020
+            symbolnum:       10
+            pcrel:           false
+            length:          3
+            extern:          true
+            type:            0
+            scattered:       false
+            value:           0
+  - cmd:             LC_SYMTAB
+    cmdsize:         24
+    symoff:          608
+    nsyms:           11
+    stroff:          784
+    strsize:         72
+LinkEditData:
+  NameList:
+    - n_strx:          8      ## N_STAB sym (in got)
+      n_type:          0x0E
+      n_sect:          2
+      n_desc:          0
+      n_value:         24
+    - n_strx:          18
+      n_type:          0x0E
+      n_sect:          2
+      n_desc:          0
+      n_value:         48
+    - n_strx:          1
+      n_type:          0x4E
+      n_sect:          1
+      n_desc:          0
+      n_value:         18
+    - n_strx:          2          ## _main
+      n_type:          0x0F
+      n_sect:          1
+      n_desc:          0
+      n_value:         0
+  StringTable:
+    - ' '
+    - _main
+    - EH_Frame1
+    - func.eh
+    - '/Users/vyng/'
+    - test.cc
+    - '/Users/vyng/test.o'
+    - _main
+...