Page MenuHomePhabricator

[obj2yaml] Implement parsing sections and auxiliary entries of XCOFF.
Needs ReviewPublic

Authored by Esme on Mar 4 2021, 8:01 PM.



This patch implements parsing sections and auxiliary entries for obj2yaml on AIX.
This patch depends on D95505.

Diff Detail

Event Timeline

Esme created this revision.Mar 4 2021, 8:01 PM
Esme requested review of this revision.Mar 4 2021, 8:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 8:01 PM
Esme added a reviewer: Restricted Project.Mar 4 2021, 8:02 PM
Esme added a comment.Mar 18 2021, 12:04 AM

Gently ping.

qiucf added a subscriber: qiucf.Mar 22 2021, 1:43 AM
qiucf added inline comments.

Better to add brief comment like length or symbol table index, depending on symbol type.? Like D68650.


As I see this test is not auto-generated? So can we reduce the indents to make the diff minimal?


Why not change return type to Error?


Put break inside brace.

Esme updated this revision to Diff 332865.Mar 23 2021, 10:17 PM
Esme added a reviewer: qiucf.

You'll need someone with XCOFF experience to review this too. Generally looks fine from my brief look though.


It would be nice if we could replace the canned binary input with a YAML input and then use yaml2obj to produce the YAML for consumption by obj2yaml at some point in the future. That'll allow us proper flexibility to test all code paths.


Nit: add extra spaces to make things line up nicely.

Should this be a CHECK-NEXT though? If so, delete the blank line above it.


Perhaps report this as a regular error using createStringError()? No need for report_fatal_error when we have an easy way to report the Error after all.


No need for the else here.


This and the similar relocation check below - do you actually need it? If a user had an invalid object, where the section type was different to what it should be (e.g. a bug in an object producer), this check would prevent using obj2yaml, yet in my mind, the YAML could easily represent this state.

Esme updated this revision to Diff 348722.Sun, May 30, 7:58 PM
  1. Address comment.
  2. Rebase on D85774 and D95505.

Yes, I agree with that. However, the yaml2obj is not yet fully implemented and some fields this test need like the symbol auxiliary entries are not yet supported in yaml2obj. I will replace the canned binary input with a YAML input in the future.

jhenderson added inline comments.Mon, Jun 7, 1:43 AM

Is there a particular need for obj2yaml support ahead of yaml2obj support for these things? I'm just wondering whether it would be better to wait.

Esme added inline comments.Tue, Jun 8, 8:27 PM

There is no particular reason for obj2yaml support to be ahead of yaml2obj, except that if I can use obj2yaml to convert compiled object files to readable yaml files, it will help me to implement yaml2obj.

jhenderson added inline comments.Wed, Jun 9, 12:25 AM

The problem is that there's currently quite a lot of noise (e.g. the excessive section content) in this test that isn't all that useful. There are also a number of cases that aren't properly covered by this test input (e.g. sections with no relocations, empty sections etc), and you may find it difficult to cover all of them using a canned binary. Better to wait until you've got sufficient yaml2obj support for the basic structure, and then develop the two in tandem (i.e. add a feature to yaml2obj and at the same time, mirror it in obj2yaml).

There's of course nothing stopping you having this patch applied locally to help you in your yaml2obj development.


Here and throughout, you need testing for your error cases, i.e. what obj2yaml does when this error is triggered.


Do you have testing for the case where this is false?


I'm not convinced this belongs at all. Why do you do this check only for debug builds?


Here and elsewhere - this is the more normal TODO syntax in LLVM.