This is an archive of the discontinued LLVM Phabricator instance.

Differential D85835

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

Authored by grimar on Aug 12 2020, 7:02 AM.

Details

Reviewers
jhenderson
MaskRay
• espindola
Commits
rG740332b6cce3: [llvm-readobj/elf] - Refine testing of broken Android's packed relocation…
Summary

This uses modern split-file tool to merge 5 packed-relocs-error*.s tests to a
new packed-relocs-errors.s and adds testing for GNU style.

Diff Detail

Event Timeline

grimar created this revision.Aug 12 2020, 7:02 AM
Herald added a reviewer: • espindola. · View Herald TranscriptAug 12 2020, 7:02 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: rupprecht, emaste. · View Herald Transcript
grimar requested review of this revision.Aug 12 2020, 7:02 AM
MaskRay accepted this revision.Aug 13 2020, 9:31 PM

LGTM

llvm/test/tools/llvm-readobj/ELF/packed-relocs-errors.s
38

You can unify the comment markers while updating the tests.

This revision is now accepted and ready to land.Aug 13 2020, 9:31 PM
jhenderson added inline comments.Aug 14 2020, 12:54 AM
llvm/test/tools/llvm-readobj/ELF/packed-relocs-errors.s
9

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?

grimar added inline comments.Aug 17 2020, 4:25 AM
llvm/test/tools/llvm-readobj/ELF/packed-relocs-errors.s
9

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.

grimar updated this revision to Diff 285964.Aug 17 2020, 4:37 AM
  • Use x86_64-unknown-linux tripple.
jhenderson accepted this revision.Aug 18 2020, 2:24 AM

LGTM, with nit.

llvm/test/tools/llvm-readobj/ELF/packed-relocs-errors.s
9

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.

38

I'm assuming this means '//' -> '##' for consistency. If so, +1 to that.

grimar added inline comments.Aug 18 2020, 2:25 AM
llvm/test/tools/llvm-readobj/ELF/packed-relocs-errors.s
38

Oops. I missed this one. Will do.

Closed by commit rG740332b6cce3: [llvm-readobj/elf] - Refine testing of broken Android's packed relocation… (authored by grimar). · Explain WhyAug 18 2020, 6:24 AM
This revision was automatically updated to reflect the committed changes.
grimar added a commit: rG740332b6cce3: [llvm-readobj/elf] - Refine testing of broken Android's packed relocation….

Revision Contents

Diff 285964

llvm/test/tools/llvm-readobj/ELF/packed-relocs-error1.s

  • This file was deleted.
// REQUIRES: x86-registered-target
// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o %t
// RUN: llvm-readobj --relocations %t 2>&1 | FileCheck %s -DFILE=%t
// CHECK: warning: '[[FILE]]': unable to read relocations from SHT_ANDROID_REL section with index 3: invalid packed relocation header
.section .rela.dyn, "a", @0x60000001
.ascii "APS9"

llvm/test/tools/llvm-readobj/ELF/packed-relocs-error2.s

  • This file was deleted.
// REQUIRES: x86-registered-target
// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o %t
// RUN: llvm-readobj --relocations %t 2>&1 | FileCheck %s -DFILE=%t
// CHECK: warning: '[[FILE]]': unable to read relocations from SHT_ANDROID_REL section with index 3: malformed sleb128, extends past end
.section .rela.dyn, "a", @0x60000001
.ascii "APS2"

llvm/test/tools/llvm-readobj/ELF/packed-relocs-error3.s

  • This file was deleted.
// REQUIRES: x86-registered-target
// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o %t
// RUN: llvm-readobj --relocations %t 2>&1 | FileCheck %s -DFILE=%t
// CHECK: warning: '[[FILE]]': unable to read relocations from SHT_ANDROID_REL section with index 3: malformed sleb128, extends past end
.section .rela.dyn, "a", @0x60000001
.ascii "APS2"
.sleb128 4 // Number of relocations
.sleb128 0 // Initial offset

llvm/test/tools/llvm-readobj/ELF/packed-relocs-error4.s

  • This file was deleted.
// REQUIRES: x86-registered-target
// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o %t
// RUN: llvm-readobj --relocations %t 2>&1 | FileCheck %s -DFILE=%t
// CHECK: warning: '[[FILE]]': unable to read relocations from SHT_ANDROID_REL section with index 3: malformed sleb128, extends past end
.section .rela.dyn, "a", @0x60000001
.ascii "APS2"
.sleb128 4 // Number of relocations
.sleb128 0 // Initial offset
.sleb128 2 // Number of relocations in group
.sleb128 2 // RELOCATION_GROUPED_BY_OFFSET_DELTA_FLAG
.sleb128 8 // offset delta

llvm/test/tools/llvm-readobj/ELF/packed-relocs-error5.s

  • This file was deleted.
// REQUIRES: x86-registered-target
// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o %t
// RUN: llvm-readobj --relocations %t 2>&1 | FileCheck %s -DFILE=%t
// CHECK: warning: '[[FILE]]': unable to read relocations from SHT_ANDROID_REL section with index 3: relocation group unexpectedly large
.section .rela.dyn, "a", @0x60000001
.ascii "APS2"
.sleb128 4 // Number of relocations
.sleb128 0 // Initial offset
.sleb128 5 // Number of relocations in group
.sleb128 2 // RELOCATION_GROUPED_BY_OFFSET_DELTA_FLAG
.sleb128 8 // offset delta

llvm/test/tools/llvm-readobj/ELF/packed-relocs-errors.s

  • This file was added.
# REQUIRES: x86-registered-target
## Test that we report meaningful warnings/errors when dumping
## broken Android's packed relocation sections.
# RUN: split-file %s %t
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %t/asm1.s -o %t1.o
# RUN: llvm-readobj --relocations %t1.o 2>&1 | FileCheck %s -DFILE=%t1.o --check-prefix=ERR-HEADER
jhendersonUnsubmitted
Not Done
ReplyInline Actions

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?

jhenderson: I could easily be misunderstanding how the -triple argument works, but don't we need a `-linux`…
grimarAuthorUnsubmitted
Done
ReplyInline Actions

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.

grimar: Hmm, good question. Honestly I did not know the answer before, but just few points I'd like to…
jhendersonUnsubmitted
Not Done
ReplyInline Actions

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.

jhenderson: Thanks. I was under the impression there was CMake configuration variable to configure the…
# RUN: not llvm-readelf --relocations %t1.o 2>&1 | FileCheck %s -DFILE=%t1.o --check-prefix=ERR-HEADER-GNU
#--- asm1.s
.section .rela.dyn, "a", @0x60000001
.ascii "APS9"
# ERR-HEADER: warning: '[[FILE]]': unable to read relocations from SHT_ANDROID_REL section with index 3: invalid packed relocation header
# ERR-HEADER-GNU: error: '[[FILE]]': invalid packed relocation header
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %t/asm2.s -o %t2.o
# RUN: llvm-readobj --relocations %t2.o 2>&1 | FileCheck %s -DFILE=%t2.o --check-prefix=ERR-PAST-END
# RUN: not llvm-readelf --relocations %t2.o 2>&1 | FileCheck %s -DFILE=%t2.o --check-prefix=ERR-PAST-END-GNU
#--- asm2.s
.section .rela.dyn, "a", @0x60000001
.ascii "APS2"
# ERR-PAST-END: warning: '[[FILE]]': unable to read relocations from SHT_ANDROID_REL section with index 3: malformed sleb128, extends past end
# ERR-PAST-END-GNU: error: '[[FILE]]': malformed sleb128, extends past end
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %t/asm3.s -o %t3.o
# RUN: llvm-readobj --relocations %t3.o 2>&1 | FileCheck %s -DFILE=%t3.o --check-prefix=ERR-PAST-END
# RUN: not llvm-readelf --relocations %t3.o 2>&1 | FileCheck %s -DFILE=%t3.o --check-prefix=ERR-PAST-END-GNU
#--- asm3.s
.section .rela.dyn, "a", @0x60000001
.ascii "APS2"
.sleb128 4 // Number of relocations
.sleb128 0 // Initial offset
MaskRayUnsubmitted
Not Done
ReplyInline Actions

You can unify the comment markers while updating the tests.

MaskRay: You can unify the comment markers while updating the tests.
jhendersonUnsubmitted
Not Done
ReplyInline Actions

I'm assuming this means '//' -> '##' for consistency. If so, +1 to that.

jhenderson: I'm assuming this means '//' -> '##' for consistency. If so, +1 to that.
grimarAuthorUnsubmitted
Done
ReplyInline Actions

Oops. I missed this one. Will do.

grimar: Oops. I missed this one. Will do.
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %t/asm4.s -o %t4.o
# RUN: llvm-readobj --relocations %t4.o 2>&1 | FileCheck %s -DFILE=%t4.o --check-prefix=ERR-PAST-END
# RUN: not llvm-readelf --relocations %t4.o 2>&1 | FileCheck %s -DFILE=%t4.o --check-prefix=ERR-PAST-END-GNU
#--- asm4.s
.section .rela.dyn, "a", @0x60000001
.ascii "APS2"
.sleb128 4 // Number of relocations
.sleb128 0 // Initial offset
.sleb128 2 // Number of relocations in group
.sleb128 2 // RELOCATION_GROUPED_BY_OFFSET_DELTA_FLAG
.sleb128 8 // offset delta
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %t/asm5.s -o %t5.o
# RUN: llvm-readobj --relocations %t5.o 2>&1 | FileCheck %s -DFILE=%t5.o --check-prefix=ERR-LARGE
# RUN: not llvm-readelf --relocations %t5.o 2>&1 | FileCheck %s -DFILE=%t5.o --check-prefix=ERR-LARGE-GNU
# ERR-LARGE: warning: '[[FILE]]': unable to read relocations from SHT_ANDROID_REL section with index 3: relocation group unexpectedly large
# ERR-LARGE-GNU: error: '[[FILE]]': relocation group unexpectedly large
#--- asm5.s
.section .rela.dyn, "a", @0x60000001
.ascii "APS2"
.sleb128 4 // Number of relocations
.sleb128 0 // Initial offset
.sleb128 5 // Number of relocations in group
.sleb128 2 // RELOCATION_GROUPED_BY_OFFSET_DELTA_FLAG
.sleb128 8 // offset delta
llvm/test/tools/llvm-readobj/ELF/packed-relocs-error2.s