This is an archive of the discontinued LLVM Phabricator instance.

Differential D97664

[llvm-objcopy][test] Fix test that could have passed spuriously
ClosedPublic

Authored by jhenderson on Mar 1 2021, 1:12 AM.

Details

Reviewers
rupprecht
MaskRay
grimar
alexander-shaposhnikov
Commits
rG47c343d7685f: [llvm-objcopy][test] Fix test that could have passed spuriously
Summary

The test was showing that when --strip-unneeded is specified for an executable, all the symbols are stripped. However, the set of symbols used in the test would be stripped by --strip-unneeded for an ET_REL object too. Fix this by adding additional symbols that aren't normally stripped by --strip-unneeded.

Diff Detail

Unit TestsFailed

TimeTest
40 msx64 debian > Flang.Semantics::resolve102.f90

Event Timeline

jhenderson created this revision.Mar 1 2021, 1:12 AM
Herald added subscribers: abrachet, emaste. · View Herald TranscriptMar 1 2021, 1:12 AM
jhenderson requested review of this revision.Mar 1 2021, 1:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2021, 1:12 AM
Harbormaster completed remote builds in B91290: Diff 327045.Mar 1 2021, 1:55 AM
MaskRay added inline comments.Mar 1 2021, 11:27 AM
llvm/test/tools/llvm-objcopy/ELF/strip-unneeded-all-symbols.test
14

If I change ET_EXEC to ET_REL, the test fails as intended.

Adding more tests is fine. Perhaps we need two tests (use something like Type: [[type]]).

MaskRay accepted this revision.Mar 1 2021, 11:27 AM
This revision is now accepted and ready to land.Mar 1 2021, 11:27 AM
jhenderson added inline comments.Mar 4 2021, 2:59 AM
llvm/test/tools/llvm-objcopy/ELF/strip-unneeded-all-symbols.test
14

I can add a test case here that shows the same input (but ET_REL instead) does produce a symbol table, if you want, to act as a validity test, but that overlaps other tests (see for example strip-unneeded.test). I think there is some benefit still, because it ensures the CHECK-NOT with no other checks is somewhat safe.

jhenderson updated this revision to Diff 328103.Mar 4 2021, 3:35 AM

Add second test case to show the check fails if the input object is ET_REL.

jhenderson added a comment.Mar 4 2021, 3:36 AM

@MaskRay, can you confirm you are happy with the updated test?

Harbormaster completed remote builds in B92024: Diff 328103.Mar 4 2021, 1:09 PM
MaskRay accepted this revision.Mar 4 2021, 2:01 PM

LGTM.

This revision was landed with ongoing or failed builds.Mar 5 2021, 1:03 AM
Closed by commit rG47c343d7685f: [llvm-objcopy][test] Fix test that could have passed spuriously (authored by jhenderson). · Explain Why
This revision was automatically updated to reflect the committed changes.
jhenderson added a commit: rG47c343d7685f: [llvm-objcopy][test] Fix test that could have passed spuriously.

Revision Contents

PathSize
llvm/
test/
tools/
llvm-objcopy/
ELF/
12 lines

Diff 327045

llvm/test/tools/llvm-objcopy/ELF/strip-unneeded-all-symbols.test

## Stripping unneeded symbols from execuatble/DSO should ## Stripping unneeded symbols from execuatble/DSO should
## eliminate the static symbol table, because it's not used ## eliminate the static symbol table, because it's not used
## by the dynamic loader. ## by the dynamic loader.
# RUN: yaml2obj %s -o %t # RUN: yaml2obj %s -o %t
# RUN: cp %t %t1 # RUN: cp %t %t1
# RUN: llvm-objcopy --strip-unneeded %t %t2 # RUN: llvm-objcopy --strip-unneeded %t %t2
# RUN: llvm-readobj --section-headers %t2 | FileCheck %s # RUN: llvm-readobj --section-headers %t2 | FileCheck %s
!ELF !ELF
FileHeader: FileHeader:
Class: ELFCLASS64 Class: ELFCLASS64
Data: ELFDATA2LSB Data: ELFDATA2LSB
Type: ET_EXEC Type: ET_EXEC
MaskRayUnsubmitted
Not Done
ReplyInline Actions

If I change ET_EXEC to ET_REL, the test fails as intended.

Adding more tests is fine. Perhaps we need two tests (use something like Type: [[type]]).

MaskRay: If I change `ET_EXEC` to `ET_REL`, the test fails as intended. Adding more tests is fine.
jhendersonAuthorUnsubmitted
Done
ReplyInline Actions

I can add a test case here that shows the same input (but ET_REL instead) does produce a symbol table, if you want, to act as a validity test, but that overlaps other tests (see for example strip-unneeded.test). I think there is some benefit still, because it ensures the CHECK-NOT with no other checks is somewhat safe.

jhenderson: I can add a test case here that shows the same input (but ET_REL instead) does produce a symbol…
Machine: EM_X86_64 Machine: EM_X86_64
Sections:
- Name: .text
Type: SHT_PROGBITS
Symbols: Symbols:
- Name: bar - Name: local_undef
- Name: foo - Name: local_def
Section: .text
- Name: global_undef
Binding: STB_GLOBAL
- Name: global_def
Section: .text
Binding: STB_GLOBAL Binding: STB_GLOBAL
# CHECK-NOT: .symtab # CHECK-NOT: .symtab