This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Add support for --add-section for COFF
ClosedPublic

Authored by sdmitriev on Jul 20 2019, 7:48 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sdmitriev created this revision.Jul 20 2019, 7:48 AM

Please make sure to update the documentation at llvm/docs/CommandGuide/llvm-objcopy.rst to move this switch out of the ELF-specific directory.

Please add a test case for trying to add a section with contents from a non-existent file. Also a test case where the argument format is invalid, e.g. --add-section foo (i.e. no '=' sign).

llvm/test/tools/llvm-objcopy/COFF/add-section.test
6–12 ↗(On Diff #210962)

Nit: It would be a bit easier to read if these had some extra spaces to make the whole lot line up with the block below. Also, the Characteristics block being indented further seems weird to me. I think the following would look better:

# CHECK:      Name: .test.section
# CHECK:      Characteristics [
# CHECK-NEXT:   IMAGE_SCN_ALIGN_1BYTES
# CHECK-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
# CHECK-NEXT: ]
16 ↗(On Diff #210962)

Same comment applies here as above.

26–32 ↗(On Diff #210962)

I suspect you can delete all of this. Let's keep the YAML to the minimum required for this test.

33–36 ↗(On Diff #210962)

You can probably delete this and most other pre-existing sections.

45 ↗(On Diff #210962)

I'm guessing you don't need these symbols for this test case.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp
175 ↗(On Diff #210962)

Rather than having a big block of code that is broadly similar to the addGnuDebugLink code, I wonder if some of the body of this loop and that function could be factored out into a function, which takes an Object, some section contents, and maybe one or two other parameters to do with addresses and adds a section to that Object.

Addressed review comments.

sdmitriev marked 6 inline comments as done.Jul 22 2019, 7:52 PM
sdmitriev added inline comments.
llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp
175 ↗(On Diff #210962)

I have refactored this code into an addSection function. Please review revised changes.

Thanks. Just a few minor things mostly in the test left.

llvm/test/tools/llvm-objcopy/COFF/add-section.test
3 ↗(On Diff #211154)

A number of newer tests in the tools use '##' to distinguish comments from lit/FileCheck commands. Please update this and the other comments accordingly.

Also "objcopy" -> "llvm-objcopy", "adds section" -> "adds a section".

8 ↗(On Diff #211154)

"if file" -> "if the file"

13 ↗(On Diff #211154)

This isn't quite testing what it should be testing, since the command-line parser is interpreting %t2 as a positional argument. You should add just remove %t2 from your command-line here, and then you should get the error for the bad format of --add-section.

15 ↗(On Diff #211154)

I find it easier to follow the test if the FileCheck CHECK lines are immediately following the test case they are used for, so it would look something like:

# RUN: ...

# CHECK1: ...
# CHECK1: ...

# RUN: ...

# CHECK2: ...
# CHECK2: ...
26 ↗(On Diff #211154)

Can you use FileCheck's -D option here to check the file name in the error message?

RUN: ... FileCheck -DFile=%t2

CHECK: ... error: [[FILE]] ...
38 ↗(On Diff #211154)

Does yaml2obj allow this line to be deleted?

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp
95 ↗(On Diff #211154)

You can delete the empty lines before and after this line. Also, what is the type of Contents here? LLVM style is to not use auto in this sort of situation, as the type is not obvious from context.

Additional changes to address review comments.

sdmitriev marked 7 inline comments as done.Jul 23 2019, 10:30 AM
sdmitriev added inline comments.
llvm/test/tools/llvm-objcopy/COFF/add-section.test
38 ↗(On Diff #211154)

No, 'symbols' is a required keyword.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp
95 ↗(On Diff #211154)

Sure, I have deleted empty lines. 'Contents' has type std::vector<uint8_t> since it is a result of the createGnuDebugLinkSectionContents() function call which is defined several lines above, but, anyway, I have replaced auto with explicit type.

jhenderson added inline comments.Jul 24 2019, 4:04 AM
llvm/test/tools/llvm-objcopy/COFF/add-section.test
29 ↗(On Diff #211326)

Use the FileCheck -D option here too instead of {{.+}}

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp
189 ↗(On Diff #211326)

Does COFF allow for empty section names? If it does, you need to distinguish the case where there is no '=' at all in the parameter from it producing an empty string, i.e. "--add-sections==foo.txt" might be allowable if COFF allows empty section names (but not "--add-section=foo.txt".

sdmitriev updated this revision to Diff 211530.Jul 24 2019, 9:35 AM

Another update to address review comments.

sdmitriev marked 2 inline comments as done.Jul 24 2019, 9:42 AM
sdmitriev added inline comments.
llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp
189 ↗(On Diff #211326)

I do not see anything in the COFF spec that would disallow sections with empty names. But, as it turned out, such sections were not handled correctly by the COFF/Writer, so I had to make a small change there to fix the problem. I have also added an additional test to the lit test that tests adding a section with empty name.

jhenderson added inline comments.Jul 25 2019, 2:33 AM
llvm/test/tools/llvm-objcopy/COFF/add-section.test
20 ↗(On Diff #211530)

Nit: "with an empty name"

44 ↗(On Diff #211530)

What does the "{{.+}}" here (and in the case above) capture? I'm not sure they're useful?

sdmitriev updated this revision to Diff 211764.Jul 25 2019, 8:56 AM

I realised that the --add-section argument string should probably be parsed in CopyConfig.cpp rather than in ELFObjcopy.cpp and COFFObjcopy.cpp, since the syntax is the same. As such, the error handling should happen there too. It looks like we're missing this test case in the ELF tests, which is why the problem with the file name in the error message wasn't picked up earlier.

Are you happy moving the parsing out of the COFF and ELF-specific bits? In this case, the CopyConfig struct should probably contain a pair of strings - the file name and the new section name, rather than a single string. An ELF-side test-case should probably be added for the missing file too.

llvm/test/tools/llvm-objcopy/COFF/add-section.test
39 ↗(On Diff #211764)

"No such file" -> "{{[Nn]}}o such file"

(double check this against other tests, but if I remember rightly, the message case depends on the platform)

44 ↗(On Diff #211764)

Sorry, I should have realised this earlier: it doesn't make sense for the error message to include the file name because the error is in the command-line parameters in this case.

See my out-of-line comment for more details.

sdmitriev marked an inline comment as done.Jul 25 2019, 10:41 AM

Sure, I can move --add-section argument string parsing out of ELFObjcopy.cpp and COFFObjcopy.cpp to the CopyConfig.cpp and add necessary test cases. I just suggest to do it as a followup change set instead of doing as part of this patch.

jhenderson accepted this revision.Jul 26 2019, 3:45 AM

Sure, I can move --add-section argument string parsing out of ELFObjcopy.cpp and COFFObjcopy.cpp to the CopyConfig.cpp and add necessary test cases. I just suggest to do it as a followup change set instead of doing as part of this patch.

Okay, sounds good. LGTM.

This revision is now accepted and ready to land.Jul 26 2019, 3:45 AM
This revision was automatically updated to reflect the committed changes.