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

[EsmeYi via Phabricator via llvm-commits]

Esme added a comment.



In D95505#2750262 <https://reviews.llvm.org/D95505#2750262>, @jhenderson wrote:

> I've taken another look and think you need a fair bit more testing.
>
> 1. You need to show that all the fields can be specified explicitly. Currently, you only do it for a subset of the fields.
> 2. You probably should show that all the flags are supported.
> 3. You need to test your error paths, to show that the errors are reported properly.
> 4. If you don't need the string table support in the initial patch (and by my understanding you don't), you can move all the related logic into another indepenedent patch, and then ensure it is properly tested there.
> 5. I think you need testing for undef and abs symbols too,
> 6. I think you need testing for non data/text/bss sections (with their zero addresses).

Thanks! @jhenderson

1. The test file llvm/test/tools/yaml2obj/XCOFF/full-contents.yaml is added.
2. Addressed.
3. There are two types of errors, one is exceeding the maximum size/index, and the other is writing with offset overlap/redundant data. For the first type of error, it is difficult to write such a large file to show the error message; and for the second type, in order to allow users to write explicitly specifies values (allowing for invalid values), our offsets are derived from the contents, therefore these errors should not occur unless the yaml2obj itself has bugs.
4. I think we should support the string table in the patch, and I have added the long symbol name to test it.
5. Addressed.
6. Addressed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95505