[PATCH] D112450: support xcoff for llvm-nm

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D112450#3104128 <https://reviews.llvm.org/D112450#3104128>, @DiggerLin wrote:

> In D112450#3102179 <https://reviews.llvm.org/D112450#3102179>, @jhenderson wrote:
>
>> Let's keep this bare-bones for now, and split off the size extraction and name demangling into separate patches.
>
> I agree with you that it may be better to split the patch into three at begin . Since I have implemented all these functionality together and there is not a lot of code change in the "size extraction" and "name demangling" ,  I do not think we can get a lot of benefit from putting effort to split the patch into three now.

Please split it up. It will allow me to review each part in isolation. It will also allow for finer grained control of the patches, e.g. because there is an issue with the size one and it needs reverting.



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:938-941
+  if (!SymSecOrErr) {
+    consumeError(SymSecOrErr.takeError());
+    return '?';
+  }
----------------
jhenderson wrote:
> 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.
As suggested in my original comment, this should be a warning rather than an error: in general, dumping tools should warn and then do what they can to continue. The warning will describe why the symbols are `?` labelled, but you'll still be able to see the list of symbols (which may be of some use).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112450