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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a reviewer: MaskRay.
jhenderson added a subscriber: MaskRay.
jhenderson added a comment.

Adding @MaskRay, as along with myself, he has reviewed a lot of yaml2obj code. @grimar is away for several weeks, so won't be able to comment for now - he's the principle maintainer of yaml2obj.

I'm not really familiar with the structure of XCOFF files, but am happy to review this (it might take a me a few days on-and-off). Could you give a very brief overveiw of the key components, and how they are placed in the file, please? That'll help inform my design comments.

Some general suggestions already:

- Where you have an optional key, you need testing that shows what the behaviour is when the key is omitted - i.e. what is the default value? Depending on the input in question, you might want to use yaml2obj's macro syntax, which has the special value `<none>` to indicate to omit the field entirely: see ELF testing for some good examples of this.
- The ideal is usually to make as many fields as possible optional. This helps keep the test input small. Some guidelines are: is there a reasonable default that people would expect? Can the value be the same in nearly all inputs or can it be reasonably inferred from other data? If yes to both of these questions, it probably wants to be optional. A good example is the `Section` data structure. It seems reasonable for the `Flags` key to be omitted, and it to have a value of 0 (i.e. no flags) if it is. Meanwhile, in ELF land, things like section addresses and offsets are also optional - if they are omitted, the values are calculated based on the ends of the previous element. The same likely is the case here.

You've also put that this is for 32-bit only. Have you considered how 64-bit suppor will be added? A lot of the time, it makes more sense for fields internally to be stored as 64-bit, and then truncated when written to the output (possibly with an error or warning to say that this is what is happening). It would be a shame for us to do the 32-bit version but find that it's too inflexible to easily add 64-bit behaviour too.

Do you have any plans for obj2yaml support? As the two share a fair bit of code in places, it's sometimes easier to develop them in tandem. That probably isn't the case for the initial bring up though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95505