[PATCH] D67689: [llvm-objcopy] Add support for --gap-fill and --pad-to options

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thinks for the patch! I haven't had time to review the meat of it yet, but will try to get back to it in the next couple of days. One quick question in the meantime: what's the motivation behind this? Is it to improve GNU compatibility?



================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:81
+
+ Pad the last loadable section to the pad-to address using a value of zero or
+ the value specified by ``--gap-fill``.
----------------
I think you can change this slightly to "... to the ''<address>'' ...", using the formatting of the reference to <section> in --only-section above (i.e. back ticks instead of apostrophes).


================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:82
+ Pad the last loadable section to the pad-to address using a value of zero or
+ the value specified by ``--gap-fill``.
+
----------------
`:option:''--gap-fill''` (using backticks instead of apostrophes)


================
Comment at: llvm/include/llvm/Object/ELF.h:408
                        " has a sh_offset (0x" + Twine::utohexstr(Offset) +
-                       ") + sh_size (0x" + Twine(Size) +
+                       ") + sh_size (0x" + Twine::utohexstr(Size) +
                        ") that cannot be represented");
----------------
This and a couple of tests look like an unrelated change?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:4
+#
+# clang-7 gapfillpadto.cpp -o gapfillpadto.elf
+# void swap(int *a, int *b) {
----------------
Hui wrote:
> asmith wrote:
> > seiya wrote:
> > > Is it possible to use yaml2obj instead of adding a binary file? I think we just need few sections to make sure that this patch works well. For example (not tested):
> > > 
> > > ```
> > > !ELF
> > > FileHeader:
> > >   Class:           ELFCLASS64
> > >   Data:            ELFDATA2LSB
> > >   Type:            ET_EXEC
> > >   Machine:         EM_X86_64
> > > Sections:
> > >   - Name:            .text
> > >     Type:            SHT_PROGBITS
> > >     Content:         "AABBCCDD"
> > >   - Name:            .foo
> > >     Type:            SHT_PROGBITS
> > >     Content:         "EEFF"
> > >   - Name:            .bar
> > >     Type:            SHT_NOBITS
> > > ```
> > That's a good suggestion. Will look into that.
> This patch resizes the section and its containing segments. (Parent or child segment). One main part of the test is to check the section to segment mapping from PHDR. However yam2obj can't keep program headers.  
> This patch resizes the section and its containing segments. (Parent or child segment). One main part of the test is to check the section to segment mapping from PHDR. However yam2obj can't keep program headers.

I'm not sure I understand what you mean here? yaml2obj supports program headers (it has done for a couple of years now I think). See for example llvm/test/tools/yaml2obj/program-header.yaml etc as well as various llvm-objcopy tests using program headers (e.g. llvm/test/tools/llvm-objcopy/segment-test-remove-section.test).


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:1
+# REQUIRES: shell
+# Tests for --gap-fill option.
----------------
Why does this and the other tests require shell? I don't see anything in here that requires it (od and cmp are both available in GnuWin32 tools for example).


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/gap-fill.test:2
+# REQUIRES: shell
+# Tests for --gap-fill option.
+#
----------------
We tend in the newer binary tools tests to use '##' for comments to separate them from test commands. Please could you update.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:566
+  if (auto Arg = InputArgs.getLastArg(OBJCOPY_pad_to)) {
+    auto addr = getAsInteger<uint64_t>(Arg->getValue());
+    if (!addr)
----------------
addr -> Addr

I'm guessing this and the above error return Expected. Would you mind not using auto here to make it clear, please?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67689/new/

https://reviews.llvm.org/D67689