This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Changed command line parsing errors
ClosedPublic

Authored by abrachet on Jun 6 2019, 12:30 PM.

Details

Summary

Tidied up errors during command line parsing to be more consistent with the rest of llvm-objcopy errors.

Event Timeline

abrachet created this revision.Jun 6 2019, 12:30 PM
abrachet updated this revision to Diff 203421.Jun 6 2019, 12:41 PM

Added a test case for --preserve-dates

abrachet updated this revision to Diff 203422.Jun 6 2019, 12:45 PM

Fixed invalid-preserve-dates test case

rupprecht added inline comments.Jun 6 2019, 1:06 PM
llvm/test/tools/llvm-objcopy/ELF/preserve-dates.test
2

This test is redundant w/ strip-preserve-atime.test and strip-preserve-mtime.test, so you can remove it.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
210–211

Flag validation errors should go in CopyConfig.cpp whenever possible so we can do simple validation as early as possible.

abrachet updated this revision to Diff 203436.Jun 6 2019, 1:40 PM

Moved the check to preserve dates in parseXXOptions(). Fixed error messages to be lower case start, no period at the end.

abrachet updated this revision to Diff 203437.Jun 6 2019, 1:41 PM

removed redundant test

abrachet retitled this revision from [llvm-objcopy] Error when --preserve-dates used on standard streams to [llvm-objcopy] Changed command line parsing errors.Jun 6 2019, 1:46 PM
abrachet edited the summary of this revision. (Show Details)
abrachet updated this revision to Diff 203440.Jun 6 2019, 1:48 PM
abrachet marked 2 inline comments as done.

Ran clang-format

abrachet updated this revision to Diff 203443.Jun 6 2019, 2:03 PM
abrachet marked an inline comment as done.

Updated test cases and changed poorly worded error

rupprecht added inline comments.Jun 6 2019, 2:27 PM
llvm/tools/llvm-objcopy/CopyConfig.cpp
666–667

These can be combined, i.e. if (Config.PreserveDates && (Config.OutputFilename == "-" || Config.InputFilename == "-"))

805–806

strip is a little more exotic in the way it accepts args; objcopy only supports:

$ llvm-objcopy foo  # "copies" in place
$ llvm-objcopy foo bar
$ llvm-objcopy foo bar baz # not valid

Whereas strip supports a couple variants, including N args:

$ llvm-strip foo  # strips in place
$ llvm-strip foo bar baz ... # also strips N objects in place
$ llvm-strip foo -o bar # strips foo, saving it as bar

and the way it's handled above (with DriverConfig) is that we create one CopyConfig with all the non-filename args, and then modify it in-place when we copy it into DriverConfig. So, Config.OutputFilename here is really just the *last* filename. (e.g. try this: llvm-strip -p - foo).

An alternative for strip (and maybe we could use the same thing for objcopy, I don't have a preference) would be to just check directly against the positional args:

if (Config.PreserveDates && (llvm::is_contained(Positional, "-") || InputArgs.getLastArgValue(STRIP_output) == "-"))

(see include/llvm/ADT/STLExtras.h)

abrachet added inline comments.Jun 6 2019, 2:27 PM
llvm/tools/llvm-objcopy/CopyConfig.cpp
483

No quotes around the format because it can only be zlib or zlib-gnu neither of which have spaces. Can be easily added though.

abrachet updated this revision to Diff 203451.Jun 6 2019, 2:36 PM

Account for strip handling of input files

abrachet marked 2 inline comments as done.Jun 6 2019, 2:37 PM
abrachet updated this revision to Diff 203458.Jun 6 2019, 3:13 PM

Small change in executeObjcopyOnBinary()

rupprecht accepted this revision.Jun 6 2019, 3:36 PM

I'm good with this after these two comments:

llvm/test/tools/llvm-objcopy/ELF/invalid-preserve-dates.test
3

This actually isn't being run because there is no FileCheck command above. The commands should be like:

# RUN: not llvm-objcopy --preserve-dates - %t < %p/Inputs/alloc-symtab.o 2>&1 | FileCheck %s
5

Can you add some cases for strip?

# Testing N args
not llvm-strip --preserve-dates -
not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o -
not llvm-strip --preserve-dates - %p/Inputs/alloc-symtab.o
not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o - %p/Inputs/alloc-symtab.o

# Testing -o
not llvm-strip --preserve-dates - -o %p/Inputs/alloc-symtab.o
not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o -o -

(and others if you think of any)

This revision is now accepted and ready to land.Jun 6 2019, 3:36 PM

Tidied up errors during command line parsing to be more consistent with the rest of llvm-objcopy errors. Also, ensure --preserve-dates is only ever used when both input and output files are regular files.

I haven't looked in depth, and I expect there's nothing wrong with these changes, but really they should be two separate commits. One for "tidying up error messages" and the other for the --preserve-dates check. Could you split them up before committing, please?

Also a quick tip for when writing tests for bug fixes: make sure the test fails prior to your change in the way you'd expect it to. This would have picked up the missing FileCheck, above I believe.

abrachet updated this revision to Diff 203716.Jun 8 2019, 10:15 PM

Moved --preserve-dates error handling to a new patch

rupprecht accepted this revision.Jun 10 2019, 1:41 PM

Thanks for splitting up the patch! I should have requested that earlier.

jhenderson accepted this revision.Jun 11 2019, 2:00 AM

LGTM too.

abrachet edited the summary of this revision. (Show Details)Jun 11 2019, 3:27 PM
This revision was automatically updated to reflect the committed changes.