Page MenuHomePhabricator

[llvm-objcopy] - Strip undefined symbols if they are no longer referenced following --only-section
ClosedPublic

Authored by grimar on May 23 2019, 7:34 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.May 23 2019, 7:34 AM

The behaviour you've implemented removes all unreferenced symbols, even if they weren't previously referenced. Is this wise? Also, have you experimented with other stripping options, e.g. removing a section which references a symbol? The bug used --only-section, but there might be other similar cases.

test/tools/llvm-objcopy/ELF/only-sesction-strip-undefined.test
1–2 ↗(On Diff #200972)

the undefined -> an undefined
if the source of reference was -> if all references to it have been

11 ↗(On Diff #200972)

What about a test-case where there are two references to the same symbol, but only one of them is removed?

tools/llvm-objcopy/ELF/ELFObjcopy.cpp
419 ↗(On Diff #200972)

remove the undefined -> remove undefined
the source of reference -like relocation section was stripped > all references have been stripped

grimar updated this revision to Diff 201172.EditedMay 24 2019, 3:05 AM
grimar marked an inline comment as done.
  • Addressed review comments.

The behaviour you've implemented removes all unreferenced symbols, even if they weren't previously referenced. Is this wise?

This behavior follows GNU, i think it is probably OK. Do you think we should change it?

Also, have you experimented with other stripping options, e.g. removing a section which references a symbol?
The bug used --only-section, but there might be other similar cases.

Not yet, but that was my plan (after landing this). I was not going to close the bug before testing
other options and fixing them (I expect to see issues, yes).

test/tools/llvm-objcopy/ELF/only-sesction-strip-undefined.test
11 ↗(On Diff #200972)

Done.

grimar added a comment.EditedMay 24 2019, 3:07 AM

Oh and I also added a test for "The behaviour you've implemented removes all unreferenced symbols, even if they weren't previously referenced"

The behaviour you've implemented removes all unreferenced symbols, even if they weren't previously referenced. Is this wise?

This behavior follows GNU, i think it is probably OK. Do you think we should change it?

No, if it follows GNU, keep it as is.

Also, have you experimented with other stripping options, e.g. removing a section which references a symbol?
The bug used --only-section, but there might be other similar cases.

Not yet, but that was my plan (after landing this). I was not going to close the bug before testing
other options and fixing them (I expect to see issues, yes).

I think you should do that before landing this, because it might mean the implementation is a little simpler (e.g. we simply always remove unreferenced undefined globals, regardless of the switch).

test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test
1 ↗(On Diff #201172)

Maybe worth removing "only-section" from the test title, as I think it would make sense for other options to fall under this test too.

10 ↗(On Diff #201172)

Probably best to put a blank line before the CHECK line, for ease of reading.

14–17 ↗(On Diff #201172)

Nit: remove the extra whitespace here and below.

21–22 ↗(On Diff #201172)

You don't need these fields.

26–29 ↗(On Diff #201172)

You don't need any of these fields. Ditto below.

48 ↗(On Diff #201172)

You don't need this.

56 ↗(On Diff #201172)

symbol -> symbols
it wasn't -> they weren't

72 ↗(On Diff #201172)

You don't need this.

75–77 ↗(On Diff #201172)

I don't think you need this symbol.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp
419 ↗(On Diff #200972)

Apparently I failed at typing here:

remove the undefined -> remove undefined
the source of reference, like relocation section was stripped -> all references have been stripped

grimar added a comment.EditedMay 24 2019, 7:08 AM

I think you should do that before landing this, because it might mean the implementation is a little simpler (e.g. we simply always remove unreferenced undefined globals, regardless of the switch).

Ok, done: imagine we have

.globl bar
.globl foo
.section .text,"ax"
  movl $bar, %edx

I.e. file with .text, .rela.text,
undefined bar, referenced by .rela.text and
undefined unreferenced foo.
(also, GNU as creates zero sized .data section, it is used below)

as in.s -o as.o

GNU objdump behavior:

  1. objcopy in.o out.o:

both foo and bar are in the output.

  1. objcopy in.o out.o -R .text

.text and .rela.text are removed, both symbols are there.

  1. objcopy in.o out.o -j .text -j .rela.text

reports error: objcopy: out.o: symbol bar required but not present
(looks like a bug to me)

  1. objcopy in.o out.o -j .data

.text and .rela.text are removed, both symbols are removed.

I.e. the behavior of this patch matches GNU now,
which does not removes undefined unreferenced symbols when -R is used and
does not remove them regardless of the switch.

Is there anything else you think worth to check?

grimar updated this revision to Diff 201235.May 24 2019, 7:24 AM
grimar marked 9 inline comments as done.
  • Addressed review comments.

Is there anything else you think worth to check?

Nope, what you've done seems fine to me. Please just address my inline comments. Note: I am away from the end of work today, for a week. I'm happy for somebody else to approve if my points are addressed.

test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test
1 ↗(On Diff #201172)

Since this looks to be only for -j behaviour, don't do this after all.

grimar marked an inline comment as done.May 24 2019, 7:29 AM
grimar added inline comments.
test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test
1 ↗(On Diff #201172)

:) I updated a few seconds before your comment. Will upload a new diff in a minute.

grimar updated this revision to Diff 201238.May 24 2019, 7:31 AM
  • Restored the original test case name.
jhenderson added inline comments.May 24 2019, 7:31 AM
test/tools/llvm-objcopy/ELF/strip-undefined.test
22 ↗(On Diff #201235)

You shouldn't need these flags either.

56–57 ↗(On Diff #201235)

Nit: remove some of the spacing here.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp
419 ↗(On Diff #200972)

This comment is still outstanding.

grimar updated this revision to Diff 201242.May 24 2019, 7:38 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.
tools/llvm-objcopy/ELF/ELFObjcopy.cpp
419 ↗(On Diff #200972)

Oops, sorry I missed this one.

This revision is now accepted and ready to land.May 24 2019, 7:50 AM

LGTM.

Thanks for review, James!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2019, 8:06 AM