This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Reorder --dump-section for MachO.
ClosedPublic

Authored by sameerarora101 on Jun 3 2020, 4:33 PM.

Details

Summary

Reorder DumpSection under handleArgs in
file MachOObjcopy.cpp. The operation to dump
a section is now performed before both add and remove
section operations for MachO file format.

Change for the ELF format at D81097. Together fixes https://bugs.llvm.org/show_bug.cgi?id=44283

Diff Detail

Event Timeline

sameerarora101 created this revision.Jun 3 2020, 4:33 PM

When I originally filed the bug, this was an issue in ELF too. Could you check the other formats, specifically ELF, but other formats too if they support --dump-section, to make sure they behave the right way, please? If not, they'll want fixing, but you can do that in a separate patch (but the bug isn't resolved until it works for all formats).

llvm/test/tools/llvm-objcopy/MachO/dump-before-add-remove.test
3

We prefer to use '##' for comments in newer tools tests, to make them stand out from check and run lines. Please update.

5

If you make the section content something ASCII, you could use FileCheck directly on the file to avoid the need for od and wc:

# FileCheck %s --input-file=%t1.dump.txt --check-prefix=CONTENTS --implicit-check-not={{.}}
# CONTENTS: abcd
11

NEW -> newly

Also, I don't think you need the extra emphasis of captialisation of NEW/NOT.

14

As you're not reusing the NODUMP prefix, you can hard code the value of SECTION into the check directly rather than passing it in as a variable.

When I originally filed the bug, this was an issue in ELF too. Could you check the other formats, specifically ELF, but other formats too if they support --dump-section, to make sure they behave the right way, please? If not, they'll want fixing, but you can do that in a separate patch (but the bug isn't resolved until it works for all formats).

Yup, I have a patch for ELF at D81097
For COFF: DumpSection options isn't supported and
for wasm: DumpSection is already being performed before add/remove.

So I think D81123 (MachO) and D81097 (ELF) together fix the bug?

sameerarora101 marked 5 inline comments as done.Jun 4 2020, 8:14 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-objcopy/MachO/dump-before-add-remove.test
5

Really cool, thanks 😊 .

sameerarora101 marked an inline comment as done.

Incorporating inline comments above and from D81097.

Mostly good

llvm/test/tools/llvm-objcopy/MachO/dump-section-before-add-remove.test
4 ↗(On Diff #268484)

There is no need to use different numbering (%t1 %t2) for the same command.
Actually, it is preferred to use the same numbering for associated output (%t1.dump.text and %t2) in this case.

.dump.text can be simplified to .dump . You don't need two extensions.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
190

The two lines should be joined.

clang-format can format that for you.

195

'='

. Fixes PR44283

You can move this part from the subject to the body.

sameerarora101 retitled this revision from [llvm-objcopy] Reorder --dump-section for MachO. Fixes PR44283 to [llvm-objcopy] Reorder --dump-section for MachO. .Jun 4 2020, 11:51 AM
sameerarora101 edited the summary of this revision. (Show Details)
sameerarora101 marked 3 inline comments as done.Jun 4 2020, 1:18 PM
sameerarora101 added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
190

Do you mean on the same line like // Dump sections before add/remove for compatibility with GNU objcopy. ?

sameerarora101 retitled this revision from [llvm-objcopy] Reorder --dump-section for MachO. to [llvm-objcopy] Reorder --dump-section for MachO..
sameerarora101 edited the summary of this revision. (Show Details)

Updating %ts to use the same number for a single command.

Fixing previous update

Harbormaster completed remote builds in B59140: Diff 268579.
This revision is now accepted and ready to land.Jun 5 2020, 12:04 AM

Actually, one request for the commit message: could you change "PR44283" to an actual URL, please (i.e. https://bugs.llvm.org/show_bug.cgi?id=44283). It makes it easier to look up the bug in the future. Same goes in the ELF case.

sameerarora101 edited the summary of this revision. (Show Details)Jun 5 2020, 6:09 AM

Updating comment: "before it is removed".

jhenderson accepted this revision.Jun 5 2020, 7:39 AM
This revision was automatically updated to reflect the committed changes.