[PATCH] D112450: support xcoff for llvm-nm

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 7 inline comments as done.
DiggerLin added inline comments.


================
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()
----------------
jhenderson wrote:
> Please include a test case that uses an archive, and produces this warning, to show that the archive name is printed.
I rollback to previous version, it do not use archive here, I will add archive name for the warn function in "create export list" patch. and there will be a test case.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:266-267
     return false;
+  if (XCOFFObjectFile *XCOFFObj = dyn_cast<XCOFFObjectFile>(&Obj))
+    return XCOFFObj->is64Bit();
+
----------------
jhenderson wrote:
> 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.
add a test case to the 8 byte address are print out in 64bits object file.


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


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:959
+
+  return '?';
+}
----------------
jhenderson wrote:
> Ditto.
add symbol in .except to test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112450