[PATCH] D54939: [llvm-objcopy] Initial COFF support

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:48
+
+  bool Is64 = false;
+  object::pe32_header PeHeader;
----------------
alexshap wrote:
> btw - I don't have a strong opinion on this yet, 
> but wanted to explain the motivation why we were trying not to keep this type of information (Is64 etc) inside the Object. Basically, the idea was to have all encoding-related logic consolidated inside the Readers/Writers and avoid accidental usage / leaking of this information into the model itself. I don't know if it makes sense for COFF (although yes, to some extent it means that this information needs to be passed around) - what do you think ?
Oh, ok, I see. We could probably quite easily get rid of `IsBigObj` and create a big object whenever it's necessary. (The current design just copies the input struct as such, bringing in the exact values of fields such as Sig1/Sig2/Version/UUID, but if we'd decouple them, we'd probably synthesize a new bigobj header instead.)

Wrt `pe32_header` vs `pe32plus_header`, the latter is almost a superset of the former - it has got a few fields lengthened to 64 bit, but it instead lacks the `BaseOfData` field. With a `pe32plus_header` struct plus a `BaseOfData` field, we could store whichever input data we read, but that on the other hand requires hardcoding which architectures are 32 bit and which are 64 bit, which we right now just take from the input file. (Hardcoding the machine type vs bitness isn't much of a practical issue as there only are 4 architectures in common use these days - but nothing else in the COFF objcopy needs to know about the architecture at all.)


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

https://reviews.llvm.org/D54939