[llvm-nm] Add test for `--debug-syms --dynamic`

[Higuoxing]

Summary

This test ensures that llvm-nm will omit NULL symbol.

[Higuoxing]

.

[grimar]

llvm/test/tools/llvm-nm/debug-syms.test

46

I am not an expert in llvm-nm, but is debug-syms.test is a right place for this test?
I'd expect to see a test case for --dynamic/-D, but I've not found.
Seems we want to add one instead?

47

You have already tested --debug-syms/-a at the first lines. No need to test them again.
Here I'd expect to see --dynamic/-D.

66

Usually local symbols go first.
Also, STB_LOCAL is the default binding, so you do not need to set it for locals.

67

Is it important to have "Section"?

70

I think you can add DynamicSymbols to the first description and have a single YAML.

[Higuoxing]

Addressed comment.

• Use a single YAML file.
• Test --dynamic/-D
• Remove unrelated/unwanted fields

[Higuoxing]

Fix Harbormaster build

[Higuoxing]

llvm/test/tools/llvm-nm/debug-syms.test

67

Is it important to have "Section"?

I think it's important. We use --implicit-check-not U to detect NULL symbol, so we should specify section field for symbols.

[MaskRay]

--debug-syms may be legacy cruft of GNU nm. An x86 nm does not hide $a $t mapping symbols. I wonder whether we should drop the auto-hide logic.

[jhenderson]

In D76213#1925340, @MaskRay wrote:

--debug-syms may be legacy cruft of GNU nm. An x86 nm does not hide $a $t mapping symbols. I wonder whether we should drop the auto-hide logic.

Mapping symbols are an ARM thing. If we are dumping an ARM object, we should hide the mapping symbols for compatibility (I see people iterating over lists of symbols dumped by nm, so we can't change this behaviour). Obviously, if this isn't an ARM object, they aren't mapping symbols, and should be dumped.

I'm somewhat concerned by the --implicit-check-not U usage to ensure no undefined symbol is printed, as it feels fragile. I'd prefer a more stringent check like --implicit-check-not={{.}}. That will ensure that there is no output other than the checked output.

llvm/test/tools/llvm-nm/debug-syms.test

5–9

Here and below, please add the letter column. This will help ensure the --implicit-check-not U is clearer.

16

No need for two blank lines.

46

I would suggest that a --dynamic dedicated test file makes sense, but that this test case here in this file also makes sense.

[grimar]

llvm/test/tools/llvm-nm/debug-syms.test

46

I would suggest that a --dynamic dedicated test file makes sense, but that this test case here in this file also makes sense.

Yeah, I've also came to it. Though we probably should not test -D here s it probably enough to test it once in a " --dynamic dedicated test file"

[Higuoxing]

Addressed comments.

• Remove test for --debug-syms -D (Leave it to a --dynamic dedicated test file)
• Use --implicit-check-not {{.}} for a more stringent check.

[grimar]

I have no more comments, this LGTM, thanks! Please wait for other reviewers too.

[jhenderson]

LGTM. @Higuoxing, would you mind writing a separate dedicated test for --dynamic? It would allow for testing the corner cases, which are outside the scope of this test file. It can be a separate patch, no problem.

[Higuoxing]

In D76213#1930891, @jhenderson wrote:

LGTM. @Higuoxing, would you mind writing a separate dedicated test for --dynamic? It would allow for testing the corner cases, which are outside the scope of this test file. It can be a separate patch, no problem.

Of course, I'd like to. Thanks for taking time review!

[MaskRay]

LGTM. You can delete unneeded Phabricator tags with:

% which arcfilter 
arcfilter () {
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend -F -
}

before committing.

[Higuoxing]

In D76213#1962028, @MaskRay wrote:

LGTM. You can delete unneeded Phabricator tags with:

% which arcfilter 
arcfilter () {
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend -F -
}

before committing.

Oh, thanks a lot! I'll try next time 😆

llvm/test/tools/llvm-nm/debug-syms.test

 # RUN: yaml2obj %s -o %t.o
-# RUN: llvm-nm --debug-syms %t.o | FileCheck %s --implicit-check-not U
-# RUN: llvm-nm -a %t.o | FileCheck %s --implicit-check-not U
+# RUN: llvm-nm --debug-syms %t.o | FileCheck %s --implicit-check-not U --check-prefix SYMBOL
+# RUN: llvm-nm -a %t.o | FileCheck %s --implicit-check-not U --check-prefix SYMBOL
+
+# SYMBOL:      $a
+# SYMBOL-NEXT: $d
+# SYMBOL-NEXT: $t
+# SYMBOL-NEXT: file_sym
+# SYMBOL-NEXT: section
+
+# RUN: llvm-nm --debug-syms --dynamic %t.o | FileCheck %s --implicit-check-not U --check-prefix DYNSYM
+# RUN: llvm-nm --debug-syms -D %t.o | FileCheck %s --implicit-check-not U --check-prefix DYNSYM
+
+# DYNSYM:      dynlocal
+# DYNSYM-NEXT: dynglobal
 
 !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_ARM
 Sections:
   - Name: section
     Type: SHT_PROGBITS
 Symbols:
   - Name:    ""
     Type:    STT_SECTION
     Section: section
   - Name:    file_sym
     Type:    STT_FILE
     Index:   SHN_ABS
   - Name:    $d
     Section: section
   - Name:    $t
     Section: section
   - Name:    $a
     Section: section
-
-# CHECK:      $a
-# CHECK-NEXT: $d
-# CHECK-NEXT: $t
-# CHECK-NEXT: file_sym
-# CHECK-NEXT: section
+DynamicSymbols:
+  - Name:    dynlocal
+    Type:    STT_OBJECT
+  - Name:    dynglobal
+    Type:    STT_OBJECT
+    Binding: STB_GLOBAL