[llvm-nm] Fix handling of symbol types 't' 'd' 'r'

[MaskRay]

Summary

This restores part of r359311 that was reverted by r359830.

Rewrite the symbol types to fix several issues.

Notable difference is that the type of __init_array_start changes from
't' to 'd'.

GNU nm used to mark ELF symbols relative to .init_array as 't'
https://sourceware.org/bugzilla/show_bug.cgi?id=24505 (before 2.33)
because ".init" is the prefix. The bug was copied by r287803.

[jhenderson]

Since you're using yaml2obj, can these tests be pulled up out of the X86 directory, into the top-level llvm-nm test directory? That would allow for testing even if X86 is unsupported.

test/tools/llvm-nm/X86/bss.test

9–12 ↗

(On Diff #198133)

Nit: not that it really matters, but you could reduce the amount of padding here a bit. Similar comments apply to the other tests.

test/tools/llvm-nm/X86/data.test

1 ↗

(On Diff #198133)

This test seems needlessly complicated for what it's testing (I think). I think it would also be nice to name the symbols and sections according to what is actually important for this test case (i.e. it's not the name ".data" that is important, it's the section flags/type etc).

How about splitting off the read-only (r/R) symbols into a different test case?

38–39 ↗

(On Diff #198133)

Do you really need this symbol and the .eh_frame section for this test? What is it showing different to the regular data symbols?

42–45 ↗

(On Diff #198133)

These symbols should be STT_TLS or not exist at all, as otherwise they aren't showing us anything interesting.

test/tools/llvm-nm/X86/linker-synthesized.test

1 ↗

(On Diff #198133)

What is this test actually testing that is not tested by other tests?

test/tools/llvm-nm/X86/nonalloc.test

3 ↗

(On Diff #198133)

Nit: new line before the yaml blob for readability.

[MaskRay]

Address review comments

[MaskRay]

test/tools/llvm-nm/X86/data.test

1 ↗

(On Diff #198133)

Created readonly.test

38–39 ↗

(On Diff #198133)

Deleted .eh_frame

42–45 ↗

(On Diff #198133)

These symbols have no types. Only their sections matter.

test/tools/llvm-nm/X86/linker-synthesized.test

1 ↗

(On Diff #198133)

This can be seen as a generalized init_fini.test. It is to prevent D26937. I can delete it if you think it not useful.

[jhenderson]

Requesting changes due to the difference versus GNU.

test/tools/llvm-nm/bss.test

1 ↗

(On Diff #198451)

This test should probably be renamed to nobits.test, since that's what is important. It might also be worth showing the behaviour with and without SHF_ALLOC and SHF_EXECINSTR, to show that the section type has precedence here.

test/tools/llvm-nm/data.test

1 ↗

(On Diff #198451)

I'd still like the names of symbols and sections in this test to be more clearly arbitrary, since the code doesn't care what the names are (but a naive user might think that it does). For example, perhaps you could name your sections simply .foo, .bar, etc or .nobits_tls_alloc_write or something, depending on whether you think the name should reflect the flags or not.

The same goes for names in the other tests.

20 ↗

(On Diff #198451)

These should be SHF_TLS, right? Otherwise, what's the point in testing them in addition to .data?

28–29 ↗

(On Diff #198451)

Delete this symbol.

37–39 ↗

(On Diff #198451)

Delete this symbol.

test/tools/llvm-nm/linker-synthesized.test

1 ↗

(On Diff #198451)

In particular in this test, it would be helpful to have a comment explaining why this test is needed. It may also be useful to have similar comments in the others, although those are clearer.

3 ↗

(On Diff #198451)

Couple of nits here:

1. Blank line between the RUN and yaml blocks, for readability.
2. You might want to consider moving the checks before the yaml to be consistent with the other tests you wrote.

test/tools/llvm-nm/nonalloc.test

1 ↗

(On Diff #198451)

Up to you whether you think it is, but perhaps it would be good to test other debug sections with different flags, e.g. SHF_WRITE?

test/tools/llvm-nm/readonly.test

14–16 ↗

(On Diff #198451)

I'd be inclined to have a couple of different rodata sections, with different combinations of flags (i.e. one with just SHF_ALLOC, another with SHF_MERGE and SHF_ALLOC etc).

tools/llvm-nm/llvm-nm.cpp

908–909 ↗

(On Diff #198451)

After some experimentation, I discovered that GNU nm (at least version 2.24) has SHF_EXECINSTR as higher priority than the SHT_NOBITS section type, so this should be above the previous if.

[MaskRay]

Improve tests suggested by jhenderson

[MaskRay]

test/tools/llvm-nm/bss.test

1 ↗

(On Diff #198451)

Good point to rename it to nobits.test, but I don't think it should test SHF_ALLOC or SHF_EXECINSTR. Their combination with SHT_NOBITS is unrealistic. We can check the precedence here, but I think we should only do it when a use case rises.

test/tools/llvm-nm/data.test

1 ↗

(On Diff #198451)

I've thought it for a while. Since we use the real names data.test readonly.test etc for the tests, if the symbols/sections in the tests are realistic seem less important. So I'll go with your suggestion.

test/tools/llvm-nm/nonalloc.test

1 ↗

(On Diff #198451)

That will actually belong to my next patch :) I'll try coming up with some tests though I can't think of realistic use cases of symbols pointing to non-allocated non-debug sections.

tools/llvm-nm/llvm-nm.cpp

908–909 ↗

(On Diff #198451)

I agree but I'm not sure whether we should check the precedence. SHF_EXECINST+SHT_NOBITS doesn't seem realistic.

[jhenderson]

test/tools/llvm-nm/bss.test

1 ↗

(On Diff #198451)

One of the problems we have is that we don't know if the use case exists. I'm inclined to agree with you that the combination is realistic though.

test/tools/llvm-nm/linker-synthesized.test

4 ↗

(On Diff #198617)

I think it would be better if the comment is at the top of the file.

Up to you, but I've also seen some people recently using '##' to clearly indicate comments versus lit/FileCheck stuff. It might be nice if you add them, but I don't mind if you don't want to.

If you've got a reference to a reported bug, it would be great to add that.

6–7 ↗

(On Diff #198617)

This comment doesn't line up with the behaviour we're testing below...

test/tools/llvm-nm/nobits.test

22 ↗

(On Diff #198617)

Perhaps for completeness sake, it's worth adding a local TLS symbol too.

test/tools/llvm-nm/readonly.test

33–35 ↗

(On Diff #198617)

Again for completeness, it might be worth adding global versions of all the local symbols. I don't mind too much though.

[MaskRay]

test/tools/llvm-nm/linker-synthesized.test

4 ↗

(On Diff #198617)

## is a great idea! It highlights the comments part.

I think it would be better if the comment is at the top of the file.

Perhaps I'm too used to lld tests and there these comments are usually below the RUN lines... I know comments before RUN lines can be regarded as file comments while the comments above may describe the individual tests. In practice, I think there isn't a dichotomy of these two types of comments. I'm ok with either one but inclined to keep it unchanged unless you have strong opinion on this.

test/tools/llvm-nm/nobits.test

22 ↗

(On Diff #198617)

I was on the fence about the choice. Since you suggest we go for completeness, I'll do that.

[MaskRay]

Address comments

[jhenderson]

LGTM. Thanks!

llvm/trunk/test/ThinLTO/X86/strong_non_prevailing.ll

 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/strong_non_prevailing.ll -o %t2.bc
 
 ; RUN: llvm-lto -thinlto-action=run %t.bc %t2.bc -exported-symbol=__llvm_profile_filename
 ; RUN: llvm-nm -o - < %t.bc.thinlto.o | FileCheck %s --check-prefix=EXPORTED
 ; RUN: llvm-nm -o - < %t2.bc.thinlto.o 2>&1 | FileCheck %s --check-prefix=NOT_EXPORTED
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 $__llvm_profile_filename = comdat any
 
 @__llvm_profile_filename = constant [19 x i8] c"default_%m.profraw\00", comdat
 
-; EXPORTED: N __llvm_profile_filename
-; NOT_EXPORTED-NOT: N __llvm_profile_filename
+; EXPORTED: R __llvm_profile_filename
+; NOT_EXPORTED-NOT: R __llvm_profile_filename

llvm/trunk/test/tools/llvm-nm/X86/init-fini.test

-# RUN: llvm-nm -B -S %p/Inputs/init-fini.out.elf-x86_64 | FileCheck --match-full-lines %s
-
-CHECK: 00000000006000c2 0000000000000000 T __bss_start
-CHECK: 00000000006000c2 0000000000000000 t __init_array_end
-CHECK: 00000000006000ba 0000000000000000 t __init_array_start
-CHECK: 00000000006000c2 0000000000000000 T _edata
-CHECK: 00000000006000c8 0000000000000000 T _end
-CHECK: 00000000004000b0 0000000000000000 T _start

llvm/trunk/test/tools/llvm-nm/data.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
+
+# CHECK:      b mybss_local
+# CHECK-NEXT: d mydata_local
+# CHECK-NEXT: d mytdata_local
+# CHECK-NEXT: B mybss_global
+# CHECK-NEXT: D mydata_global
+# CHECK-NEXT: D mytdata_global
+
+!ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: mybss
+    Type: SHT_NOBITS
+    Flags: [ SHF_ALLOC, SHF_WRITE ]
+  - Name: mydata
+    Type: SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_WRITE ]
+  - Name: mytdata
+    Type: SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_WRITE, SHF_TLS ]
+Symbols:
+  - Name:    mybss_local
+    Section: mybss
+  - Name:    mydata_local
+    Section: mydata
+  - Name:    mytdata_local
+    Section: mytdata
+
+  - Name:    mybss_global
+    Binding: STB_GLOBAL
+    Section: mybss
+  - Name:    mydata_global
+    Binding: STB_GLOBAL
+    Section: mydata
+  - Name:    mytdata_global
+    Binding: STB_GLOBAL
+    Section: mytdata

llvm/trunk/test/tools/llvm-nm/linker-synthesized.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
+
+## We used to be wrong with some linker synthesized symbols.
+
+## We mark __init_array_start as 'd', as consistent with GNU nm >= 2.33 (older GNU
+## nm marks it as 't'). See https://sourceware.org/bugzilla/show_bug.cgi?id=24511
+
+# CHECK: d __fini_array_start
+# CHECK: d __init_array_start
+# CHECK: d __preinit_array_start
+# CHECK: B __bss_start
+# CHECK: D _edata
+# CHECK: B _end
+
+!ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: .text
+    Type: SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+  - Name: .fini_array
+    Type: SHT_FINI_ARRAY
+    Flags: [ SHF_ALLOC, SHF_WRITE ]
+  - Name: .init_array
+    Type: SHT_INIT_ARRAY
+    Flags: [ SHF_ALLOC, SHF_WRITE ]
+  - Name: .preinit_array
+    Type: SHT_PREINIT_ARRAY
+    Flags: [ SHF_ALLOC, SHF_WRITE ]
+  - Name: .data
+    Type: SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_WRITE ]
+  - Name: .bss
+    Type: SHT_NOBITS
+    Flags: [ SHF_ALLOC, SHF_WRITE ]
+Symbols:
+  - Name:    __fini_array_start
+    Section: .fini_array
+  - Name:    __init_array_start
+    Section: .init_array
+  - Name:    __preinit_array_start
+    Section: .preinit_array
+
+  - Name:    __bss_start
+    Section: .bss
+    Binding: STB_GLOBAL
+  - Name:    _edata
+    Section: .data
+    Binding: STB_GLOBAL
+  - Name:    _end
+    Section: .bss
+    Binding: STB_GLOBAL

llvm/trunk/test/tools/llvm-nm/nobits.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
+
+# CHECK: b mybss_local
+# CHECK: b mytbss_local
+# CHECK: B mybss_global
+# CHECK: B mytbss_global
+
+!ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: mybss
+    Type: SHT_NOBITS
+    Flags: [ SHF_ALLOC, SHF_WRITE ]
+  - Name: mytbss
+    Type: SHT_NOBITS
+    Flags: [ SHF_ALLOC, SHF_WRITE, SHF_TLS ]
+Symbols:
+  - Name:    mybss_local
+    Binding: STB_LOCAL
+    Section: mybss
+  - Name:    mytbss_local
+    Binding: STB_LOCAL
+    Section: mytbss
+
+  - Name:    mybss_global
+    Binding: STB_GLOBAL
+    Section: mybss
+  - Name:    mytbss_global
+    Binding: STB_GLOBAL
+    Section: mytbss

llvm/trunk/test/tools/llvm-nm/nonalloc.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
+
+# CHECK: n debug_info_main
+
+!ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .debug_info
+    Type: SHT_PROGBITS
+Symbols:
+  - Name:    debug_info_main
+    Section: .debug_info

llvm/trunk/test/tools/llvm-nm/readonly.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
+
+# CHECK:      r myrodata0_local
+# CHECK-NEXT: r myrodata1_local
+# CHECK-NEXT: r myrodata2_local
+# CHECK-NEXT: R myrodata0_global
+# CHECK-NEXT: R myrodata1_global
+# CHECK-NEXT: R myrodata2_global
+
+!ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: myrodata0
+    Type: SHT_PROGBITS
+    Flags: [ SHF_ALLOC ]
+  - Name: myrodata1
+    Type: SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_MERGE ]
+  - Name: myrodata2
+    Type: SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_MERGE, SHF_STRINGS ]
+Symbols:
+  - Name:    myrodata0_local
+    Section: myrodata0
+  - Name:    myrodata1_local
+    Section: myrodata1
+  - Name:    myrodata2_local
+    Section: myrodata2
+
+  - Name:    myrodata0_global
+    Binding: STB_GLOBAL
+    Section: myrodata0
+  - Name:    myrodata1_global
+    Binding: STB_GLOBAL
+    Section: myrodata1
+  - Name:    myrodata2_global
+    Binding: STB_GLOBAL
+    Section: myrodata2

llvm/trunk/tools/llvm-nm/llvm-nm.cpp

static char getSymbolNMTypeChar(ELFObjectFileBase &Obj,
   Expected<elf_section_iterator> SecIOrErr = SymI->getSection();
   if (!SecIOrErr) {
     consumeError(SecIOrErr.takeError());
     return '?';
   }
 
   elf_section_iterator SecI = *SecIOrErr;
   if (SecI != Obj.section_end()) {
-    switch (SecI->getType()) {
-    case ELF::SHT_PROGBITS:
-    case ELF::SHT_DYNAMIC:
-      switch (SecI->getFlags()) {
-      case (ELF::SHF_ALLOC | ELF::SHF_EXECINSTR):
-        return 't';
-      case (ELF::SHF_TLS | ELF::SHF_ALLOC | ELF::SHF_WRITE):
-      case (ELF::SHF_ALLOC | ELF::SHF_WRITE):
-        return 'd';
-      case ELF::SHF_ALLOC:
-      case (ELF::SHF_ALLOC | ELF::SHF_MERGE):
-      case (ELF::SHF_ALLOC | ELF::SHF_MERGE | ELF::SHF_STRINGS):
-        return 'r';
-      }
-      break;
-    case ELF::SHT_NOBITS:
+    uint32_t Type = SecI->getType();
+    uint64_t Flags = SecI->getFlags();
+    if (Flags & ELF::SHF_EXECINSTR)
+    if (Type == ELF::SHT_NOBITS)
       return 'b';
-    case ELF::SHT_INIT_ARRAY:
-    case ELF::SHT_FINI_ARRAY:
-      return 't';
-    }
+    if (Flags & ELF::SHF_ALLOC)
+      return Flags & ELF::SHF_WRITE ? 'd' : 'r';
   }
 
   if (SymI->getELFType() == ELF::STT_SECTION) {
     Expected<StringRef> Name = SymI->getName();
     if (!Name) {
       consumeError(Name.takeError());
       return '?';
     }