[PATCH] D89071: [SystemZ/z/OS] Add GOFFObjectFile class and details of GOFF file format

[Yusra Syeda via Phabricator via llvm-commits]

yusra.syeda added a comment.

In D89071#2614095 <https://reviews.llvm.org/D89071#2614095>, @kpn wrote:

> In D89071#2610685 <https://reviews.llvm.org/D89071#2610685>, @jhenderson wrote:
>
>> The majority of your code is still untested as far as I can see. There appear to be three test cases you have so far:
>>
>> 1. An invalid size for a GOFF object.
>> 2. A valid size for a GOFF object.
>> 3. That `getSymbolName` returns the name of a single symbol in the symbol table.
>>
>> What about all the rest of the functionality that is included in this patch, including, but certainly not limited to, the following?
>>
>> 1. More than one symbol in the symbol table.
>> 2. Other properties of symbols.
>> 3. The various properties of records.
>> 4. Relocations.
>> 5. And so on...
>>
>> For each bit of code you have written, consider whether a test would fail if that bit of code was broken in some way, or didn't exist. If no test fails, then that code needs a new test case of some form. There may also be other cases where testing is appropriate, e.g. where two separate aspects of the same system interact in some way, although those are harder to judge.
>
> Here's an idea: I found, at least when running in batch, that the Binder (linker) will link an object consisting of nothing more than a HDR card, then ESD cards followed by an END card (meaning, just symbols). It will also link an object consisting of HDR, ESD, TXT, and END cards with zero relocations. Does it work that way when not running in batch? Because if it does then it might make sense to split this ticket up into a new ticket with just support for HDR+ESD+END cards.
>
> That would make this patch smaller, and it would reduce the amount of tests that need to be written to get some initial GOFF support into the tree. The tests that @jhenderson requested would still be needed, but you'd only need the ones that were relevant to the smaller amount of code in the new ticket. A new ticket should refer back to this ticket because this ticket shows the direction you are going, and it has a bunch of comments that should probably be left for posterity. Later tickets can build on this foundation.
>
> The LLVM community tends to prefer smaller patches over larger ones. Typically, anyway.
>
> It's an idea. Thoughts?

I agree this would be a quicker way to get some initial GOFF support in. I can break down this patch to support HDR + ESD + END records, and add the relevant tests mentioned by @jhenderson. I'll add support for the remaining records incrementally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89071