Page MenuHomePhabricator

[llvm-objcopy] [COFF] Implement --only-section
ClosedPublic

Authored by mstorsjo on Jan 17 2019, 12:09 PM.

Details

Summary

This uses the same input data as for --only-keep-debug. As a contrast, --only-keep-debug truncates the other sections, while --only-section fully removes the other sections.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Jan 17 2019, 12:09 PM

Just to confirm, the desired behaviour of combining certain switches (e.g. --strip-debug and --only-section) is for the explicitly mentioned --only-section section to be kept despite it potentially being removed by the other switch?

Just to confirm, the desired behaviour of combining certain switches (e.g. --strip-debug and --only-section) is for the explicitly mentioned --only-section section to be kept despite it potentially being removed by the other switch?

Good point - that's the impression I got from reading the structure of handleArgs for ELF, and I hadn't doublechecked until now. However this does seem to be wrong - GNU objcopy returns the intersection of the sets of sections kept by the two options.

mstorsjo updated this revision to Diff 182582.Jan 18 2019, 12:28 PM

Updated to test how --only-section combined with --strip-debug behaves - matching GNU objcopy's behaviour.

rupprecht accepted this revision.Jan 18 2019, 1:51 PM
rupprecht added inline comments.
test/tools/llvm-objcopy/COFF/only-section.test
12 ↗(On Diff #182582)

nit: these tests will be a little easier to read if they are indented something like this:

SECTIONS:           xSections:
SECTIONS-NEXT:      xIdx Name
SECTIONS-DEBUG-NEXT:x  .debug_discardable
SECTIONS-TEXT-NEXT: x  .text
SECTIONS-NEXT-EMPTY:x

(x's for clarity)

21 ↗(On Diff #182582)

I'm not too familiar with using -EMPTY in filecheck, but is this supposed to be SYMBOLS-NEXT-EMPTY?

This revision is now accepted and ready to land.Jan 18 2019, 1:51 PM
mstorsjo marked 3 inline comments as done.Jan 18 2019, 1:57 PM
mstorsjo added inline comments.
test/tools/llvm-objcopy/COFF/only-section.test
12 ↗(On Diff #182582)

Ok, I can align them.

21 ↗(On Diff #182582)

-EMPTY essentially implies -NEXT, it requires the next line to either be empty, or there to be EOF here. I haven't intended to use them combined but apparently I did do that in the paragraph above this. I'll change that one for consistency with the rest (that's the only one I have with NEXT-EMPTY at all).

This revision was automatically updated to reflect the committed changes.
mstorsjo marked an inline comment as done.

-EMPTY essentially implies -NEXT, it requires the next line to either be empty, or there to be EOF here. I haven't intended to use them combined but apparently I did do that in the paragraph above this. I'll change that one for consistency with the rest (that's the only one I have with NEXT-EMPTY at all).

For the record, you can't combine FileCheck suffixes (e.g. CHECK-NEXT-NOT is a NOT using a check prefix of CHECK-NEXT). There are some error checks to catch this, but I don't know how comprehensive they are.

By the way, @jakehehrlich may remember exactly what the intended behaviour is in llvm-objcopy for ELF and combining rules. We discussed it a while back, but I can't remember them exactly, but it was something like explicit section keeps trump implicit section strips, and two explicits are an error. You might want to double-check with him.

By the way, @jakehehrlich may remember exactly what the intended behaviour is in llvm-objcopy for ELF and combining rules. We discussed it a while back, but I can't remember them exactly, but it was something like explicit section keeps trump implicit section strips, and two explicits are an error. You might want to double-check with him.

Ok. At least for combining --strip-debug with --only-section, the ELF version doesn't match what GNU objcopy does.

I don't have a concrete use/need for --only-section myself so I haven't invested that much thought into it, I only brought it along from the code from ELFObjcopy.cpp that I based the COFF version on.