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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:83
+struct Section {
+  object::coff_section Header;
+  ArrayRef<uint8_t> Contents;
----------------
jakehehrlich wrote:
> mstorsjo wrote:
> > jakehehrlich wrote:
> > > It might be useful to declare this as the OriginalHeader and have a second Header. We discovered overtime that this would have been a good structure in the ELF case but currently we have a horrible hodgepodge of fields prefixed with "Original" and some without. Also, the name "Contents" doesn't necessarily need to change but it should be clear that such a thing is the original contents and nothing more.
> > So far, I don't keep a copy of the original header anywhere but I just patch things in this one copy (marginally when just doing a plain copy, patching more when actually changing things) before finally writing it to the output with one memcpy, so there's no distinction between original and current.
> > 
> > If we need to keep the original header separately later, couldn't we add the separate OriginalHeader field at that point?
> I'd say if you ever have to keep a single "Original" field then you should keep the whole original header.
Right - well I don't keep any "original" fields in the sense that it is the original unmodified value from the input file, but it's the actual header as it will be written to the destination file in the same struct form, with values updated and filled in along the objcopy process.

So I don't break out all the individual fields but keep them as they are on disk. The only exception is the Name field that I break out into a separate StringRef, as the Name field of the `coff_section` header only makes sense in the context of a full file.

As for the field `Contents`, why would I need to make a distinction that it is the original contents? In the follow-up parts where I synthesize a `.gnu_debuglink` section, `Contents` won't be any original contents but the newly created. (In that case I add a separate field to actually own the allocation as the `ArrayRef` either points to the original input file contents or newly synthesized contents.)


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

https://reviews.llvm.org/D54939