[PATCH] D112450: support xcoff for llvm-nm

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

This is my last day working before Christmas. If I find time to respond to any updates/comments before the end of my work day, I'll do so, but otherwise, don't expect further responses from me until at least the 4th of January (likely a day or two after that).



================
Comment at: llvm/test/tools/llvm-nm/XCOFF/basic.test:2-11
+## Test: local symbol in .text section, show type char 't'
+## Test: local symbol in .data section, show type char 'd'
+## Test: symbol in .bss section, show type char 'b'
+## Test: symbol in .debug section, show type char 'N'
+## Test: symbol .file show tpye char 'f'
+## Test: global symbol in .text section, show type char 'T'
+## Test: global symbol in .data section, show type char 'D'
----------------
I think you could simplify these slightly by doing the following:

1) Move each individual line down to the symbol you are checking (either in the YAML or the CHECK lines, so that you have something like
```
## Comment
# CHECK: symbol check line
## Comment
# CHECK: symbol check line
```
2) No need for "Test:" at the start of the statement.
3) End with a "." like other comments.
4) You can drop the "show type char 'X'" part of each comment, because this is implicit by what the test actually does. The bit that's important is what properties of the symbol are significant.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/basic.test:13
+
+# RUN: yaml2obj --docnum=1 %s -o %t.o
+# RUN: llvm-nm  %t.o | FileCheck --check-prefix=NM-SYM --match-full-lines %s
----------------
No need for `--docnum` with only one YAML document in the file.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/basic.test:14
+# RUN: yaml2obj --docnum=1 %s -o %t.o
+# RUN: llvm-nm  %t.o | FileCheck --check-prefix=NM-SYM --match-full-lines %s
+
----------------
Use the default CHECK, since you only have one test case in this file.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/invalid-section-index.test:3-7
+# RUN: yaml2obj --docnum=1 %s -o %t.o
+# RUN: llvm-nm %t.o 2>&1 | FileCheck %s -DFILE=%t.o --check-prefix=NM
+
+# NM:      {{.*}}llvm-nm{{(\.exe)?}}: warning: [[FILE]]: for symbol with index 0: the section index (4) is invalid
+# NM-NEXT: 00000000 ? .text 
----------------
1) Don't use `--docnum`/`--check-prefix` unnecessarily.
2) `{{.*}}` is unnecessary at the start of a CHECK line, if not using `--match-full-lines`.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/invalid-section-index.test:17
+    Value:              0x0
+    SectionIndex: 4
+    Type:               0x0
----------------
Line this up with the other entries in the block.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:141
+    WithColor::warning(errs(), ToolName)
+        << (Archive.str().empty() ? FileName : Archive + "(" + FileName + ")")
+        << ": " << (Context.str().empty() ? "" : Context + ": ") << EI.message()
----------------
Please include a test case that uses an archive, and produces this warning, to show that the archive name is printed.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:266-267
     return false;
+  if (XCOFFObjectFile *XCOFFObj = dyn_cast<XCOFFObjectFile>(&Obj))
+    return XCOFFObj->is64Bit();
+
----------------
You need a test case that uses a 64-bit input, and one that uses a 32-bit input, and then demonstrate how this part of the patch impacts the behaviour.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:944-945
+
+  if (SecIter == Obj.section_end())
+    return '?';
+
----------------
I'm not sure I see a test case for this particular line?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:959
+
+  return '?';
+}
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112450