[PATCH] D85835: [llvm-readobj/elf] - Refine testing of broken Android's packed relocation sections.

Tue Aug 18 02:24:46 PDT 2020

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with nit.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/packed-relocs-errors.s:8
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/asm1.s -o %t1.o
+# RUN: llvm-readobj --relocations %t1.o 2>&1 | FileCheck %s -DFILE=%t1.o --check-prefix=ERR-HEADER
----------------
grimar wrote:
> jhenderson wrote:
> > I could easily be misunderstanding how the -triple argument works, but don't we need a `-linux` suffix on here to ensure an ELF object is produced (as opposed to e.g. COFF or Mach-O) on all platforms, regardless of their default triple?
> Hmm, good question. Honestly I did not know the answer before, but just few points I'd like to mention:
> 1) I had no intention to change the tripple here, original objects are using `x86_64-pc-linux-gnu` and I think that
>     just truncated the tripple, because copy pasted it from one of LLD(ELF) test cases.
> 2) There are multiple LLD ELF tests that are using `-triple=x86_64`. I've verified that llvm-mc produces ELF inputs when running 
>      from windows environment. (It was very expected, since all these tests are known to pass on windows, but anyways..)
> 3) Finally, I've debugged how tripples are parsed and the default OS for this case is always `ELF`:
>      https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/Triple.cpp#L665
>     I.e. a host OS doesn't matter here and when no OS is specified it fall backs to the default `ELF` one.
> 
> But I think that would be indeed slightly cleaner to specify an expected target OS in the tripple. So I am going to update this invocation to specify the `x86_64-unknown-linux`, which is also very commonly used by LLD tests (another popular variant is `x86_64-pc-linux`, btw). I think it might be slightly better than original `x86_64-pc-linux-gnu`, since there is nothing `GNU` specific in this tests.
> 
> 
> 
Thanks. I was under the impression there was CMake configuration variable to configure the default triple, but maybe that only applies in certain cases, and not here. I know @MaskRay often comments about the extra suffix bit for triples, so maybe he's got something to add.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/packed-relocs-errors.s:37
+.ascii "APS2"
+.sleb128 4 // Number of relocations
+.sleb128 0 // Initial offset
----------------
MaskRay wrote:
> You can unify the comment markers while updating the tests.
I'm assuming this means '//' -> '##' for consistency. If so, +1 to that.

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

https://reviews.llvm.org/D85835