Page MenuHomePhabricator

[llvm-objcopy] Add support for response files in llvm-strip and llvm-objcopy
ClosedPublic

Authored by mmpozulp on Jul 27 2019, 6:01 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mmpozulp created this revision.Jul 27 2019, 6:01 PM
mmpozulp marked an inline comment as done.Jul 27 2019, 6:12 PM
mmpozulp added inline comments.
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
316–326 ↗(On Diff #212074)

I copied these lines from lib/Support/CommandLine.cpp where they appear as

bool CommandLineParser::ParseCommandLineOptions(int argc,
                                                const char *const *argv,
                                                StringRef Overview,
                                                raw_ostream *Errs,
                                                bool LongOptionsUseDoubleDash) {
  assert(hasOptions() && "No options specified!");

  // Expand response files.
  SmallVector<const char *, 20> newArgv(argv, argv + argc);
  BumpPtrAllocator A;
  StringSaver Saver(A);
  ExpandResponseFiles(Saver,
         Triple(sys::getProcessTriple()).isOSWindows() ?
         cl::TokenizeWindowsCommandLine : cl::TokenizeGNUCommandLine,
         newArgv);
  argv = &newArgv[0];
  argc = static_cast<int>(newArgv.size());
  ...

Should these lines be a separate function in the CommandLine library, or is it better to be repetitive here?

mmpozulp marked an inline comment as not done.Jul 27 2019, 6:13 PM
jhenderson added inline comments.Jul 29 2019, 4:42 AM
llvm/test/tools/llvm-objcopy/ELF/response-file.test
3 ↗(On Diff #212074)

I see what you're trying to do here, but I don't think this is a good way of testing response files, because an invalid command-line might lead to the tool printing its help/usage text, which could include "@FILE" in it. I'd recommend showing that a basic operation can be performed (e.g. a copy with --strip-debug).

It might be worth a test showing that "@FILE" appears when --help is explicitly passed on the command-line however.

llvm/tools/llvm-objcopy/CopyConfig.cpp
411 ↗(On Diff #212074)

name -> Name (or possibly even "ToolName").

412 ↗(On Diff #212074)

I'd pass them in as llvm-objcopy and llvm-strip here.

412–413 ↗(On Diff #212074)

Twines are only intended to be used for temporaries essentially (or as function parameters which are passed in as temporaries). I believe what you are doing here may not be safe. You should do it inline at the usage point, since you only use them once..

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
316–326 ↗(On Diff #212074)

It looks like there are quite a few tools that call cl::ExpandResponseFiles directly themselves. It seems logical to refactor them all at once to use the same version. You might want to see if you can do that, rather than just copying this code here.

329–331 ↗(On Diff #212074)

Is this change related? It looks like it's just formatting, so you shouldn't change it as part of this change.

seiya added a subscriber: seiya.Aug 1 2019, 5:43 AM
mmpozulp updated this revision to Diff 214295.Aug 8 2019, 10:14 PM

Incorporate feedback from @jhenderson. I still need to try refactoring tools which call cl::ExpandResponseFiles to avoid code duplication. If successful then we won't have to continue the code duplication trend here.

mmpozulp updated this revision to Diff 214297.Aug 8 2019, 10:17 PM

Capitalize variable newArgv -> NewArgv

mmpozulp marked an inline comment as done.Aug 8 2019, 10:31 PM
mmpozulp added inline comments.
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
329–331 ↗(On Diff #212074)

Yes. Without it a bunch of tests fail with the message error: too many positional arguments. In the calls to parseStripOptions and parseObjcopyOptions we were passing argc instead of argc - 1 which was a bug. It didn't manifest because argv[argc] was a null pointer. Now argv[argc] is junk after we set argv:

   311 	int main(int argc, char **argv) {
-> 312 	  InitLLVM X(argc, argv);
   313 	  ToolName = argv[0];
   314 	  bool IsStrip = sys::path::stem(ToolName).contains("strip");
   315 	
(lldb) p argv[argc]
(char *) $0 = 0x0000000000000000
(lldb) cont
Process 8459 resuming
Process 8459 stopped
* thread #1, name = 'llvm-objcopy', stop reason = breakpoint 2.1
    frame #0: 0x00005555555f3bcf llvm-objcopy`main(argc=9, argv=0x00007fffffffe400) at llvm-objcopy.cpp:326:39
   323 	                              : cl::TokenizeGNUCommandLine,
   324 	                          NewArgv);
   325 	  argv = const_cast<char **>(&NewArgv[0]);
-> 326 	  argc = static_cast<int>(NewArgv.size());
   327 	
   328 	  Expected<DriverConfig> DriverConfig =
   329 	      IsStrip
(lldb) p argv[argc]
(char *) $1 = 0x00007fffffffe470 "\xa8\xaa=VUU"

We see the junk and think its another positional argument... unless the junk string starts with a dash in which case the tool might complain that it doesn't understand the extra flag :)

Hi @mmpozulp,

When you've addressed review comments, it's helpful if you mark the "Done" box in the relevant comment. That way it's obvious what hasn't been addressed. If you do it before you upload, then they will get submitted when the patch gets updated.

It might be worth a test showing that "@FILE" appears when --help is explicitly passed on the command-line however.

Having thought about it, I definitely think I'd like this please. If it's not in the help text, it might as well not exist. You also need to update the documentation to show that it is supported (see similar changes I made recently to other tools).

llvm/test/tools/llvm-objcopy/ELF/response-file.test
7–8 ↗(On Diff #214297)

This is a minor point, but we've tended to use llvm-readelf/llvm-readobj to check sections, IIRC. Also, it's probably worth doing a cmp to show that the two output files are identical.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
11 ↗(On Diff #214297)

Could you do this header reordering in a separate NFC commit please (no need to get a review).

mmpozulp updated this revision to Diff 214439.Aug 9 2019, 1:22 PM
mmpozulp marked 7 inline comments as done.

Rebaseline to get the new llvm-strip.rst docs page.
Incorporate feedback from @jhenderson to

  1. Click "Done" boxes in phabricator. Thanks :)
  2. Add @FILE check to help-message.test
  3. Update llvm-strip.rst and llvm-objcopy.rst docs with @FILE option.
  4. Remove the header re-ordering. I like it the way it was before.
  5. Change response-file.test to a) use llvm-readobj to check sections,

and b) run cmp to show the output files are identical.
TODO: Try refactoring tools which call cl::ExpandResponseFiles to
avoid copy-pasting code from lib/Support/CommandLine.cpp like I did here.

jhenderson accepted this revision.Aug 12 2019, 2:02 AM

LGTM, with a couple of nits. I'm okay with the refactoring being left to a different change, as long as you don't leave it a long time.

llvm/test/tools/llvm-objcopy/ELF/response-file.test
8 ↗(On Diff #214439)

Strictly, you no longer need the second llvm-readobj line, since the cmp makes it redundant.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
316 ↗(On Diff #214439)

Perhaps worth adding a TODO note saying that this is duplicated code.

This revision is now accepted and ready to land.Aug 12 2019, 2:02 AM
rupprecht added inline comments.Aug 12 2019, 12:48 PM
llvm/tools/llvm-objcopy/CopyConfig.cpp
414 ↗(On Diff #214439)

This method is a bit of a hack, as the older library has a cl::extrahelp class that's usually used to support this [1], but libOption doesn't have yet. We should probably add one. Can you add a comment here to remove this once something like that is supported?

[1] e.g. https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-size/llvm-size.cpp#L102

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
325–331 ↗(On Diff #214439)

Instead of manipulating argc/argv directly, you can convert NewArgv directly to an array ref to pass to parse*Options, e.g.

auto Args = makeArrayRef(NewArgv).drop_front();
Expected<DriverConfig> DriverConfig =
    IsStrip ? parseStripOptions(Args, reportWarning)
            : parseObjcopyOptions(Args);

Ping

Thanks for the ping :) I'll try to update the patch this weekend.

mmpozulp updated this revision to Diff 220193.Fri, Sep 13, 4:22 PM
mmpozulp marked 4 inline comments as done.

Incorporate feedback from @jhenderson and @rupprecht

This revision was automatically updated to reflect the committed changes.