Page MenuHomePhabricator

[llvm-objcopy] [COFF] Implement --only-keep-debug

Authored by mstorsjo on Jan 17 2019, 3:26 AM.

Diff Detail


Event Timeline

mstorsjo created this revision.Jan 17 2019, 3:26 AM
jhenderson added inline comments.Jan 17 2019, 7:06 AM
4 ↗(On Diff #182243)

-sections -> --sections

34–36 ↗(On Diff #182243)

I think you should just inline this.

39 ↗(On Diff #182243)

Would it make more sense for this to be after section removal, so that it has less work to do?

41 ↗(On Diff #182243)

Remove the word "data" here, unless "content data" is a COFF phrasing.

mstorsjo updated this revision to Diff 182370.Jan 17 2019, 12:07 PM
mstorsjo marked 4 inline comments as done.

Applied @jhenderson's suggestions. But I also split the input data into a separate yaml file in Inputs, as I'm going to use the same input for another test.

jhenderson added inline comments.Jan 18 2019, 6:17 AM
1 ↗(On Diff #182370)

If this yaml file is going to be used in other tests, it should be more generically named (unless the other tests also involve "only-keep-sections").

1 ↗(On Diff #182370)

Wait, never mind. I got myself mixed up about the switch name and this!

7–9 ↗(On Diff #182370)

This comment is a little clunky. Perhaps rephrase it to something like:

Check that all non-debug/buildid sections with IMAGE_SCN_CNT_CODE or IMAGE_SCN_CNT_INITIALIZED_DATA are truncated, and none others.

51–53 ↗(On Diff #182370)

Nit of nits: don't shorten comment lines unnecessarily. clang-format will insert new lines to make them conform to the character width, but won't make them longer if they are unnecessarily short.

mstorsjo updated this revision to Diff 182524.Jan 18 2019, 8:26 AM
mstorsjo marked 2 inline comments as done.

Applied @jhenderson's suggestions.

jhenderson accepted this revision.Jan 18 2019, 8:49 AM

LGTM, assuming this matches GNU objcopy.

5 ↗(On Diff #182524)

You don't need to change this, but the GNU style of llvm-readobj also provides symbol dumping in the style you are looking at. It may be useful to use that instead, if you want to keep things to one run of llvm-readobj.

8 ↗(On Diff #182524)

Oops, that should be "no others".

This revision is now accepted and ready to land.Jan 18 2019, 8:49 AM
mstorsjo marked 3 inline comments as done.Jan 18 2019, 12:53 PM
mstorsjo added inline comments.
5 ↗(On Diff #182524)

Hmm, how do you activate that? Do you mean --elf-output-style=GNU, or something else? That one doesn't seem to have any effect when inspecting a COFF file.

8 ↗(On Diff #182524)

Oops, will fix before committing.

rupprecht accepted this revision.Jan 18 2019, 2:16 PM
rupprecht added inline comments.
5 ↗(On Diff #182524)

Yes, or invoke "llvm-readelf" instead of "llvm-readobj" sets that flag (among a few other minor tweaks). But it looks like that flag is only used in ELFDumper which explains why nothing changes for COFF. If you're feeling adventurous, you might want to handle that in COFFDumper, but that would be a major patch.

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

Oops, yeah, I forgot for a moment that GNU output style is (currently) ELF-specific.