[PATCH] D113552: [yaml2obj][XCOFF] parsing auxiliary symbols.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:118
+  }
+}; // 64-bit XCOFF file only.
+
----------------
I'm not sure that these comments are really useful. I'd just get rid of them.


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:263-264
+    if (!Is64)
+      IO.setError("the exception auxiliary symbol table entry is defined in "
+                  "XCOFF64 only");
+    AuxSym.reset(new XCOFFYAML::ExcpetionAuxEnt());
----------------
Should this really be an error? Could you potentially have an AUX_EXCEPT symbol in a 32-bit object (albeit a malformed one)? Or is it fundamentally impossible to write such a thing? Same goes for the AUX_STAT symbol below.

Remember, in yaml2obj, we want to be as permissive as possible. If you could imagine something existing, it should be allowed, even if the file format techincally prohibits it. This then allows you to write testing for code that reads the file (e.g. llvm-readobj) to ensure it properly handles such cases.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-symbols-defaults.yaml:3
+
+## AUX_CSECT symbol in XCOFF32
+# RUN: yaml2obj %s --docnum=1 -o %t1
----------------
Rather than have one test-case per aux entry type, is it possible to have one which contains all entry types, either with a single symbol, or multiple symbols, as needed?

In the mulitple symbol case, name the symbols after their aux entry.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-symbols-defaults.yaml:38
+
+## AUX_CSECT symbol in XCOFF64
+# RUN: yaml2obj %s --docnum=1 -DMAGIC=0x1F7 -o %t2
----------------



================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-symbols-full-contents.yaml:1
+## Test that we can explicitly specify all fields for auxiliary symbols.
+
----------------
Rather than needing to duplicate the YAML between this and the defaults test case, you could consider implementing support for (if needed) and using the special `[[MACRO=<none>]]`  syntax in the YAML. For ELF, this is equivalent to omitting the field, if no value is specified for it, which allows you to test default behaviour, using the same YAML as non-default behaviour.


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:136-142
+    // TODO:: dump auxiliary symbols.
+    for (size_t I = 0; I < *Sym.NumberOfAuxEntries; I++) {
+      std::unique_ptr<XCOFFYAML::AuxSymbolEnt> AuxSym;
+      Sym.AuxEntries.push_back(std::move(AuxSym));
+    }
+
+    Symbols.push_back(std::move(Sym));
----------------
Maybe spin this off into another patch? Seems unrelated to getting yaml2obj to actually produce the aux symbols, and needs its own testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113552