[PATCH] D98003: [obj2yaml] Implement parsing sections and auxiliary entries of XCOFF.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/obj2yaml/XCOFF/aix.yaml:1
 # RUN: obj2yaml %S/Inputs/aix_xcoff.o | FileCheck %s
 # Test that we can parse the XCOFF object file correctly.
----------------
Esme wrote:
> jhenderson wrote:
> > Esme wrote:
> > > jhenderson wrote:
> > > > It would be nice if we could replace the canned binary input with a YAML input and then use yaml2obj to produce the YAML for consumption by obj2yaml at some point in the future. That'll allow us proper flexibility to test all code paths.
> > > Yes, I agree with that. However, the yaml2obj is not yet fully implemented and some fields this test need like the symbol auxiliary entries are not yet supported in yaml2obj. I will replace the canned binary input with a YAML input in the future.
> > Is there a particular need for obj2yaml support ahead of yaml2obj support for these things? I'm just wondering whether it would be better to wait.
> There is no particular reason for obj2yaml support to be ahead of yaml2obj, except that if I can use obj2yaml to convert compiled object files to readable yaml files, it will help me to implement yaml2obj.
The problem is that there's currently quite a lot of noise (e.g. the excessive section content) in this test that isn't all that useful. There are also a number of cases that aren't properly covered by this test input (e.g. sections with no relocations, empty sections etc), and you may find it difficult to cover all of them using a canned binary. Better to wait until you've got sufficient yaml2obj support for the basic structure, and then develop the two in tandem (i.e. add a feature to yaml2obj and at the same time, mirror it in obj2yaml).

There's of course nothing stopping you having this patch applied locally to help you in your yaml2obj development.


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:85
+          Obj.getSectionContents(SectionDRI);
+      if (!SecDataRefOrErr)
+        return SecDataRefOrErr.takeError();
----------------
Here and throughout, you need testing for your error cases, i.e. what obj2yaml does when this error is triggered.


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:138
+      // A file auxiliary entry is optional.
+      if (Sym.NumberOfAuxEntries) {
+        const XCOFFFileAuxEnt *FileAuxEntPtr =
----------------
Do you have testing for the case where this is false?


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:143-145
+#ifndef NDEBUG
+        Obj.checkSymbolEntryPointer(reinterpret_cast<uintptr_t>(FileAuxEntPtr));
+#endif
----------------
I'm not convinced this belongs at all. Why do you do this check only for debug builds?


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:157
+    case XCOFF::C_HIDEXT: {
+      // TODO implement function auxiliary entry.
+      // A csect auxiliary entry is required.
----------------
Here and elsewhere - this is the more normal TODO syntax in LLVM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98003