[PATCH] D112450: support xcoff for llvm-nm

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/xcoff_nm_print_symbol.test:2
+; Test the llvm-nm for xcoff object files.
+; RUN: llvm-nm  %p/Inputs/test_gcc.o | FileCheck --check-prefix=NM-SYM %s
+; RUN: llvm-nm  --print-size --demangle %p/Inputs/test_gcc.o | FileCheck --check-prefix=NM-SIZE-DEMANGLE %s
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Can't we use yaml2obj here now? That would allow for much better targeted testing.
> I do not think I can use the yaml2obj now, the yaml2obj do not support [[ https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__c0f91ad419jbau | "csect Auxiliary Entry for C_EXT, C_WEAKEXT, and C_HIDEXT Symbols"  ]] .  in the xcoff , some important symbol attributes are stored in the csect Auxiliary Entry" , @esms, if you have time , would you like to consider to implement the functionality into yaml2obj ?
@Esme


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:938-941
+  if (!SymSecOrErr) {
+    consumeError(SymSecOrErr.takeError());
+    return '?';
+  }
----------------
DiggerLin wrote:
> jhenderson wrote:
> > I know that other formats throw away errors, but I would suggest that's a mistake - how is a user supposed to know why a given symbol results in `?`? I'd recommend reporting the `Error` as a warning, rather than just throwing it away.
> I agree with you, But I am prefer to keep the same code style and error handling with other object file format in the patch. and  fixing the error handling for all the other object file format  in a  separate patch.
You're suggesting the following path:

XCOFF with bad error handling -> Fix handling for all formats (including XCOFF)

I think it would be better to avoid ever having a state where XCOFF has bad handling, so I'd prefer one of the two following:

Land XCOFF with good error handling -> Fix handling of other formats.
Fix handling of other formats -> Land XCOFF with good error handling.

I don't mind if you want to change the other formats up-front, but beware that there is a risk this will be more complicated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112450