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

[Esme Yi via Phabricator via llvm-commits]

Esme added a comment.

In D113552#3197920 <https://reviews.llvm.org/D113552#3197920>, @DiggerLin wrote:

> the x_auxtype is only for 64bits. in the implement, it means we always need to specific the AUX_type no matter it is 32bit or 64bits.

Yes, I think it's convenient for users to define an aux symbol with a specified type and it's straightforward for yaml2obj designing.



================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:61
+  AUX_STAT = 249
+};
+
----------------
DiggerLin wrote:
> there is enum define as enum SymbolAuxType in xcoff.h, I think you can use it instead of define a new one.
XCOFF::SymbolAuxType is only for XCOFF64, therefor the AUX_STAT for XCOFF32 is not a member of  XCOFF::SymbolAuxType. But it's required in yaml2obj (as what I have designed) when writing the section auxiliary entry for the C_STAT symbol.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:64
+struct AuxSymbolEnt {
+  AuxSymbolType Type;
+
----------------
DiggerLin wrote:
> please add some comment here " it is for 64bits only"
It's required to determine the aux type when reading the yaml files, even we are not going to write the `_auxtype` field for XCOFF32. So it's not for 64bits only here.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:89
+  Optional<uint32_t> StabInfoIndex;
+  Optional<uint16_t> StabSectNum;
+
----------------
DiggerLin wrote:
> I am prefer to add some comments on the data member to indicate the one is for 32bits or 64bits only, or it is for both.
> and rearrange the member in the order of common field for both 32bits and 64bits first , only for 32bits, only for the 64bits.   
Nice comments, thanks.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:516
+  else
+    ErrHandler("unknown auxiliary symbol type: " + Twine(AuxSym->Type));
+}
----------------
jhenderson wrote:
> I don't think you've got a test case for this case?
In fact, this is a `llvm_unreachable` case because the `XCOFFYAML::AuxSymbolType AuxType` is a required field in Yaml input and a case with `unknown auxiliary symbol type` can't be generated in Yaml.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113552