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

[Esme Yi via Phabricator via llvm-commits]

Esme added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:160
+  Optional<uint8_t> NumberOfAuxEntries;
+  std::vector<std::unique_ptr<AuxSymbolEnt>> AuxEntries;
 };
----------------
Higuoxing wrote:
> Question 1: Why we need `std::unique_ptr<>` here?
> 
> Question 2: It looks that `AuxEntries` is optional in the symbol entry. Can we wrap it with `Optional<>`.
I think there is no harm in using `std::unique_ptr<>` since it has less overhead. While wrapping it with `Optional<>` will cause failure in `mapOptional()`.


================
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());
----------------
jhenderson wrote:
> 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.
I think this should be an error.
1. When parsing a 32-bit object, the AuxSymbol type is determined by the Value and StorageClass of the symbol to which it belongs. Even if we intentionally write a case of `AUX_EXCEPT symbol in XCOFF32`, it cannot be parsed as AUX_EXCEPT, but as some other types, and it makes sense to parse it this way.
2. As for the 64-bit object, the AuxSymbol type can also be recognized by the AuxSymbolType field, but the type XCOFF::AUX_STAT does not exist, so it is fundamentally impossible to write such a thing.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-symbols-defaults.yaml:3
+
+## AUX_CSECT symbol in XCOFF32
+# RUN: yaml2obj %s --docnum=1 -o %t1
----------------
jhenderson wrote:
> 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.
Thanks!


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-symbols-full-contents.yaml:1
+## Test that we can explicitly specify all fields for auxiliary symbols.
+
----------------
jhenderson wrote:
> 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.
The `[[MACRO=<none>]]` syntax will work well to reduce duplication when the same field is used for multiple assignments. But in this test, most of the fields only need to be tested once. 
Besides, 32-bit and 64-bit may have different fields for the same aux symbol type, so I have to use two YAMLs. I don't think using [[MACRO=<none>]] here will simplify the test.
And I think it would be easier to review if I separate the default and non-default behaviors?


================
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));
----------------
jhenderson wrote:
> Maybe spin this off into another patch? Seems unrelated to getting yaml2obj to actually produce the aux symbols, and needs its own testing.
Agree, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113552