[PATCH] D133030: [AIX] llvm-readobj support a new option --exception-section for xcoff object file.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I did wonder whether this should really just be reusing the existing --unwind option, but as I understand it, the XCOFF exception section isn't really about unwinding the stack or anything along those lines. Is that correct?

Rather than canned binaries, I think it wouldn't be too hard to add yaml2obj support for exception sections, so that you can create the input at test time.

Your commit message should be a grammatically correct sentence, with leading capital letter (i.e. "Add a new ..."), much like comments.

I've not really looked at the XCOFFObjectFile code changes or the testing just yet. Please don't forget to test error/warning paths, to show that the code can handle malformed inputs.



================
Comment at: llvm/docs/CommandGuide/llvm-readobj.rst:335
+
+  Display exception section entries of XCOFF object file.
+
----------------
I think you can simplify to the inline suggestion.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:94
+template uint32_t ExceptionSectionEntry<support::ubig64_t>::getSymbolIndex() const ;
+template uint64_t ExceptionSectionEntry<support::ubig64_t>::getTrapInstAddr()const;
+template uint8_t ExceptionSectionEntry<support::ubig64_t>::getLangID() const;
----------------
This file hasn't been clang-formatted. Please fix.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1008-1011
+  if (is64Bit())
+    assert(sizeof(ExceptEnt) == sizeof(ExceptionSectionEntry64));
+  else
+    assert(sizeof(ExceptEnt) == sizeof(ExceptionSectionEntry32));
----------------
I recommend folding this `if` into the assertions, possibly like the inline suggestion. Otherwise you have an empty if without assertions, which seems messy (although the optimizer should get rid of it in this case).


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/exception-section.test:9
+# CHECK:       Exception section {
+# CHECK32-NEXT:   Symbol Index: 12 (.bar)
+# CHECK64-NEXT:   Symbol Index: 6 (.bar)
----------------
I think it would be more similar to other dumping formats to print this row as:
`Symbol: .bar (12)`.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:143
+    StringRef SymName =
+        unwrapOrError(Obj.getFileName(), Obj.getSymbolNameByIndex(SymIdx));
+    W.printString("Symbol Index", (Twine(SymIdx) + " (" + SymName + ")").str());
----------------
`unwrapOrError` should be considered deprecated in llvm-readobj, as it stops llvm-readobj from continuing, which is not useful for dumping tools, especially given that the error in this case won't prevent other files or sections from being dumped. Instead, prefer reporting problems as warnings (via `reportUniqueWarning` or equivalent). See the ELF dumper for good examples.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:152
+  ArrayRef<T> ExceptSectEnts =
+      unwrapOrError(Obj.getFileName(), Obj.getExceptionEntries<T>());
+
----------------
Ditto: don't use `unwrapOrError`.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:508-509
+
+  if (Obj.isXCOFF() && opts::XCOFFExceptionSection)
+    Dumper->printExceptionSection();
+
----------------
Is there a reasson you're not putting this up with the other XCOFF-specific dumping option?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133030