[PATCH] D60878: [Object][XCOFF] Add an XCOFF dumper for llvm-readobj

Thu May 2 08:22:29 PDT 2019

sfertile marked 4 inline comments as done.
sfertile added a comment.

================
Comment at: llvm/test/tools/llvm-readobj/xcoff-basic.test:1
+# RUN: llvm-readobj --file-header %p/Inputs/xcoff-basic.o | \
+# RUN: FileCheck --check-prefix=FILEHEADER %s
----------------
hubert.reinterpretcast wrote:
> MaskRay wrote:
> > `// REQUIRES: powerpc-registered-target` ?
> > 
> > 
> > `arc patch D60878` gives me empty `xcoff-basic-neg-time.o`. I can't verify their architectures.
> 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
> 
> ```
>>!  @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.

>>! @hubert.reinterpretcast wrote:
> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60878