This is an archive of the discontinued LLVM Phabricator instance.

[Object][XCOFF] Add an XCOFF dumper for llvm-readobj
ClosedPublic

Authored by sfertile on Apr 18 2019, 10:16 AM.

Diff Detail

Event Timeline

sfertile created this revision.Apr 18 2019, 10:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2019, 10:16 AM
sfertile edited the summary of this revision. (Show Details)Apr 22 2019, 1:10 PM
daltenty requested changes to this revision.Apr 23 2019, 8:11 AM

The format of the binaries included in the patch don't seem correct, they create empty files. As a result the corresponding test are failing.

This revision now requires changes to proceed.Apr 23 2019, 8:11 AM

The format of the binaries included in the patch don't seem correct, they create empty files. As a result the corresponding test are failing.

The diff just shows that a binary file was added. If you want the actual binaries let me know and I can put them somewhere you can grab them.

daltenty accepted this revision.Apr 23 2019, 11:54 AM

Retested with binaries. Looks good to me.

This revision is now accepted and ready to land.Apr 23 2019, 11:54 AM
hubert.reinterpretcast requested changes to this revision.Apr 25 2019, 7:00 PM
hubert.reinterpretcast added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
314

The field being returned is unsigned. The return type does not match.

329

The field being returned is 16-bit. The return type does not match.

This revision now requires changes to proceed.Apr 25 2019, 7:00 PM
sfertile updated this revision to Diff 197382.Apr 30 2019, 11:02 AM

Fixed return types to match the type of field being accessed.

This revision is now accepted and ready to land.May 1 2019, 9:42 PM
MaskRay added inline comments.
llvm/test/tools/llvm-readobj/xcoff-basic.test
1

// REQUIRES: powerpc-registered-target ?

arc patch D60878 gives me empty xcoff-basic-neg-time.o. I can't verify their architectures.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
44

Delete the empty line.

llvm/test/tools/llvm-readobj/xcoff-basic.test
1

I believe you asked this before on D60784. The functionality being tested is available even without enabling what the build system considers the PowerPC target-specific code.

@sfertile, I don't know how you generated the diff and whether or not this would work, but we may want to give the --binary option of git diff a try:

diff --git a/llvm/test/tools/obj2yaml/Inputs/aix_xcoff.o b/llvm/test/tools/obj2yaml/Inputs/aix_xcoff.o
new file mode 100644
index 0000000000000000000000000000000000000000..3712f7f853e506d8c0bcf2922e7f61c8b63cffee
GIT binary patch
literal 588
zcmZ8eyH3ME5S()i;-v&rBBVG_LE$O|REb0t5fKVX3R}b=i${YC7a%3#3lMw=AHqKn
z{sUoV_s)r7q+O49XLs-R%qP5y(Lon5D*(1=i1M_^g)A^69n(HE<f#v%agrxWA$hSK
z+B<DV1AO)U*RS&Xty9h(d+Wt?7ntq?M*|q)F9Y%rc-lKD4tofnsXd*55n?rrCkBSB
z3;!<3?%Nusle5$zvjt7sW~)KJJ!g_z*2*)orm~pKS0<gjHlnNSHYF95KWJQ=JblQc
zOB=C1tN)ODa<U5cu77vO{EcYb=>n&X<SJDi^V-MN+tmU!$q)=`pAB0Stl95>jrNgO
nBs{07y-_^x<7Rc1toD3#Y>Di(n&l!v%aYpN7aG?mkbM)srs_94

literal 0
HcmV?d00001
sfertile updated this revision to Diff 197790.May 2 2019, 8:13 AM

Removed extra blank lines and added binaries into the diff.

sfertile marked 4 inline comments as done.May 2 2019, 8:21 AM
llvm/test/tools/llvm-readobj/xcoff-basic.test
1
@MaskRay wrote:

// REQUIRES: powerpc-registered-target ?

No, the tests don't need a registered powerpc target, the only dependence is on XCOFFObjectFile from lib/Object which gets built on all platforms.

we may want to give the --binary option of git diff a try

Thanks, I wasn't aware of this. I'll make sure to include the binaries in diffs from now on.

It looks like (at least this instance of) Phabricator does not do much better with the binary patch. arc export does generate GIT binary patch output, but it does not appear to reflect the intended content.

This revision was automatically updated to reflect the committed changes.