[PATCH] D95505: [yaml2obj] Initial support for 32-bit XCOFF in yaml2obj.

[EsmeYi via Phabricator via llvm-commits]

Esme added inline comments.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:55-56
+      {StringRef("N_UNDEF"), XCOFF::N_UNDEF}};
+  XCOFFYAML::FileHeader InitFileHdr = Obj.Header;
+  std::vector<XCOFFYAML::Section> InitSections = Obj.Sections;
+};
----------------
jhenderson wrote:
> Why not just use `Obj.Header` and `Obj.Sections` directly in the referencing code? Why do you need these member variables separately to `Obj`?
`Obj` can't be changed in assignAddressesAndIndices(), because it will be used during writing. The value specified in the YAML has a higher priority to be written than the calculated valued derived from the contents. And only Header and Sections have these number/offset/address fields which need to be calculated, therefore I use InitSections and InitFileHdr to keep these calculated values.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:119
+      CurrentOffset += InitSections[I].SectionData.binary_size();
+      // Ensure the address is aligned to DefaultSectionAlign.
+      CurrentOffset = alignTo(CurrentOffset, DefaultSectionAlign);
----------------
jhenderson wrote:
> Is it definitely the address here that should be being aligned, or the offset? The two are different concepts, and the align function appears to align the offset, not the address.
The offset of data for each section is what I want to align. The comments may be confusing here. 


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:113
+void MappingTraits<XCOFFYAML::Relocation>::mapping(IO &IO, XCOFFYAML::Relocation &R) {
+  IO.mapRequired("Address", R.VirtualAddress);
+  IO.mapRequired("Symbol", R.SymbolIndex);
----------------
jhenderson wrote:
> Esme wrote:
> > jhenderson wrote:
> > > In ELF, the `r_offset` field of a relocation specifies where the relocation will patch, and is relative to either the section start or file start, depending on the context. Often in our test cases, it doesn't really matter where the relocation is patching, only that there is a relocation, hence the offset field is optional there. It seems likely that this will be the case for XCOFF too?
> > I agree that it's unfriendly to require the virtual address of the relocation from users.
> > While this item in XCOFF specifies the virtual address of the value that requires modification by the binder. And the offset to the data in the section will be calculated as follows:
> > offset_in_section = Relocation_Address - Section_Address
> > We can calculate the Section_Address from yaml contents, but hard to determine the Relocation_Address.
> > So this is hardly optional?
> Could 0 be a reasonable default? If not, it's fine. I'm just wondering if it actually matters in all cases what the address is.
As I see, 0 is not a reasonable default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95505