Page MenuHomePhabricator

[llvm-objcopy][WebAssembly] Add dump/add/remove-section support
ClosedPublic

Authored by dschuff on Dec 3 2019, 10:24 AM.

Details

Summary

Add support for adding, removing, and dumping wasm sections to objcopy.

Depends on D70930

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
dschuff updated this revision to Diff 232389.Dec 5 2019, 10:33 AM
  • Split tests
dschuff updated this revision to Diff 232390.Dec 5 2019, 10:35 AM
  • fix newline

I believe this patch is now also ready, please take a look.

jhenderson added inline comments.Dec 9 2019, 2:15 AM
llvm/test/tools/llvm-objcopy/wasm/add-section.test
3

'#' -> '##'

5–6

Where does %t2 come from? I think this test is missing a line to remove the section after the dump?

I'd expect to see something like:

# RUN: llvm-objcopy --dump-section=producers=%t.sec --remove-section=producers %t %t2
# RUN: llvm-objcopy --add-section=producers=%t.sec %t2 %t3
70–74

I generally find it easier to follow the test if CHECKs are before the YAML, especially where the YAML is large.

llvm/test/tools/llvm-objcopy/wasm/dump-section.test
2

You need a test showing what happens when you attempt to dump a non-existent section.

25

Missing trailing full stop.

26–27

Same comment as add-section.test

llvm/test/tools/llvm-objcopy/wasm/remove-section.test
2

Missing trailing full stop.

4

You need a test showing what happens when you attempt to remove a non-existent section.

24–26

Same comment as in other test (move earlier).

Also, this is insufficient to show that the producers section doesn't exist. This only shows that it doesn't exist AFTER the TYPE section. You either want CHECK-NOT lines before and after the CHECK: TYPE line, or you want to add --implicit-check-not=producers to your FileCheck line.

llvm/tools/llvm-objcopy/wasm/Object.cpp
28

Nit: missing trailing full stop.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
36

What is the type of Sec here (please don't use auto unless the type is obvious)? Also, should it be a const &?

41–42

If there's an easy way, this should be tested. Example is that the output file is a directory.

45

If there's an easy way, this should be tested too.

52

I'd prefer this not to be auto.

55

Ditto.

65

Ditto.

dschuff marked 18 inline comments as done.Dec 11 2019, 2:10 PM
dschuff added inline comments.
llvm/test/tools/llvm-objcopy/wasm/add-section.test
5–6

Yeah, my original version tried to avoid using dump/remove in the test, but this ended up a lot simpler this way.
I updated the test as suggested. Your suggestion doesn't work as-written because section removal happens before section dumping (and I kept it that way for consistency with the other targets), but I just did it in a separate invocation.

llvm/test/tools/llvm-objcopy/wasm/remove-section.test
4

This is actually a more interesting question than I realized. The current behavior is to error on dumping a nonexistent section but will silently ignore request to remove nonexistent sections, which matches ELF objcopy. I can see why one might want that behavior, but is that in your opinion a good thing to duplicate from ELF?

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
45

I couldn't find any way to cause error this without trying to do racy things like changing things about the filesystem while llvm-objcopy is running.

dschuff updated this revision to Diff 233450.Dec 11 2019, 2:19 PM
  • review comments
jhenderson added inline comments.Dec 12 2019, 1:58 AM
llvm/test/tools/llvm-objcopy/wasm/add-section.test
5–6

Your suggestion doesn't work as-written because section removal happens before section dumping (and I kept it that way for consistency with the other targets), but I just did it in a separate invocation.

It turns out that this is a bug compared with GNU objcopy for ELF (and I'm guessing other formats too): --dump-section should be runnable in the same operation as the section is removed. I've filed PR44283 for that.

llvm/test/tools/llvm-objcopy/wasm/dump-section.test
8

I'd recommend changing this to "%t.dir/bar" or similar, since theoretically something else might create "foo" (by accident or intentionally, it doesn't matter).

15

I'd include the bit of the path that provides context in this check - I'm guessing something like error: {{.*}}/bar: {{[nN]}}o such file or directory would work.

llvm/test/tools/llvm-objcopy/wasm/remove-section.test
4

Most of the llvm-objcopy behaviour for ELF at least is intended to replicate what GNU objcopy does, for compatibility reasons. I'm guessing there isn't another version of objcopy for wasm yet? If there is, we should try to follow that. Otherwise, I think for consistency it probably makes sense to follow the general guidelines of what ELF does, as if wasm is support is eventually added to GNU objcopy, it's likely to follow its own behaviour for other formats.

8

I think you mean here to write llvm-objcopy --remove-section=nonexistent %t?

It's probably a good idea to check that there are no warnings produced as part of this operation.

10–11

You probably want to move this to immediately after the corresponding FileCheck run to make it a little clearer what is using it.

13

Unnecessary?

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
52

No need for a StringRef to be a const &, since it is already implicitly.

55

clang-format?

65

See my above comment re. StringRef

dschuff updated this revision to Diff 234204.Dec 16 2019, 5:36 PM
  • Merge branch 'objcopy-wasm' into objcopy-add-remove
  • Make Object own the OwnedContents instead of Section
dschuff updated this revision to Diff 239709.Jan 22 2020, 3:02 PM
  • Rebase on master and initial llvm-objcopy patch

Unit tests: pass. 62119 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 2 errors and 6 warnings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

dschuff updated this revision to Diff 241518.Jan 30 2020, 11:02 AM
  • Merge branch 'master' into objcopy-add-remove
  • Update for fixes in previous CL

Unit tests: fail. 62355 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

dschuff updated this revision to Diff 241593.Jan 30 2020, 2:55 PM
dschuff marked 12 inline comments as done.
  • comments, and fix file errors
  • dump section before removal

Rebased against master, and addressed all the comments; please take a look.

llvm/test/tools/llvm-objcopy/wasm/add-section.test
5–6

Fixed for wasm in this patchset, and updated the test as suggested.

llvm/test/tools/llvm-objcopy/wasm/dump-section.test
15

This actually exposed a bug; namely, that filename in the error message was that of the input file (%t) instead of the section dump output file (%t/bar). This same bug is seems to be present in the ELF objcopy; my fix is making handleArgs create the FileError itself (with either the input or output file) rather than having its caller executeObjcopyOnBinary always wrap the return value. ELF objcopy has a lot more functionality, I don't know if the same approach would work there.

Unit tests: fail. 62355 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jhenderson added inline comments.Jan 31 2020, 1:49 AM
llvm/test/tools/llvm-objcopy/wasm/add-section.test
2

dumps -> dumps and removes

10–13

The whitespace doesn't need to exactly match what obj2yaml produces, so it might look a little nicer and easier to read by reformatting this slightly:

# CHECK:      Name: producers
# CHECK-NEXT: Tools:
# CHECK-NEXT:   - Name:    clang
# CHECK-NEXT:     Version: 9.0.0
73–75

Nit: I think these last three lines (two blank) can be deleted.

llvm/test/tools/llvm-objcopy/wasm/dump-section.test
15

Seems reasonable. I think the same approach for ELF should work, if you don't mind fixing it up in a separate patch.

33–34

You can delete these two lines.

llvm/test/tools/llvm-objcopy/wasm/remove-section.test
10

It's probably a good idea to check that there are no warnings produced as part of this operation.

I think you missed this part. You can check for warnings by doing something like:
# RUN: llvm-objcopy --remove-section=nonexistent=%t.sec %t 2&>1 | count 0

llvm/tools/llvm-objcopy/wasm/Object.h
32

I don't think this comment is useful. It's obvious from the function name that this adds a section.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
44

Check against GNU objcopy for ELF, but I suspect --add-section is applied after removal and dumping. This would allow replacing a section to be done by --remove-section foo --add-section foo=foo.bin or something along those lines.

70

Either make this a TODO, or just remove the line, since the check below shows we don't support OnlySection.

dschuff updated this revision to Diff 241767.Jan 31 2020, 10:37 AM
dschuff marked 9 inline comments as done.
  • address review comments, clang-format
llvm/test/tools/llvm-objcopy/wasm/remove-section.test
10

Ah yes I did miss this one.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
44

Confirmed that GNU objdump does dump, remove, then add. Also added a test here.

Unit tests: fail. 62355 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Looks good. Perhaps worth a test case for --dump-section + --remove-section for the same section? That feels like an important interaction to me.

llvm/test/tools/llvm-objcopy/wasm/add-section.test
21–22

Nit: delete a space on these two lines so that Type/Name/Payload all line up.

dschuff updated this revision to Diff 242699.Feb 5 2020, 10:59 AM
dschuff marked 2 inline comments as done.
  • Merge branch 'master' into objcopy-add-remove
  • Address comment, add test for dump+remove

Unit tests: pass. 62516 tests passed, 0 failed and 844 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Fixed the nit and added the test for dump-section and remove-section. The clang-format bot seems very confused, but I ran clang-format locally.

jhenderson accepted this revision.Feb 6 2020, 12:47 AM

LGTM, once a wasm expert gives the thumbs up.

This revision is now accepted and ready to land.Feb 6 2020, 12:47 AM
sbc100 accepted this revision.Feb 11 2020, 11:47 AM
sbc100 added inline comments.
llvm/test/tools/llvm-objcopy/wasm/add-section.test
3

Would it be worth checking that the initial and final objects are byte-for-byte identical somehow?

llvm/utils/gn/secondary/llvm/tools/llvm-objcopy/BUILD.gn
75

Is it now the expectation that we update .gn files along with cmake? (BTW, do you use gn locally?)

dschuff marked 2 inline comments as done.Feb 11 2020, 3:10 PM
dschuff added inline comments.
llvm/test/tools/llvm-objcopy/wasm/add-section.test
3

Currently they aren't identical because they don't encode all of the LEBs identically.
Also obj2yaml puts the reloc.CODE section at the end after the producers section, but it gets added to the end by objcopy. So currently it's not actually a goal to have them be bit-identical. If we wanted we could do that in a future patch.

llvm/utils/gn/secondary/llvm/tools/llvm-objcopy/BUILD.gn
75

No, that is explicitly not an expectation (https://github.com/llvm/llvm-project/blob/master/llvm/utils/gn/README.rst). But I do sometimes use GN, so I ended up doing it in this case.

This revision was automatically updated to reflect the committed changes.