[objcopy] Error when --preserve-dates is specified with standard streams

[abrachet]

Summary

llvm-objcopy/strip now error when -p is specified when reading from stdin or writing to stdout

[abrachet]

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

8 ↗

(On Diff #203875)

These lines were getting very long. I tried to split it up with '\' but it didn't work. Is there another way to do this or is it fine to have very long lines?

llvm/tools/llvm-objcopy/CopyConfig.cpp

743 ↗

(On Diff #203875)

I switched this to StringRef so std::find isn't comparing pointers

[rupprecht]

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

8 ↗

(On Diff #203875)

You should be able to use \. Maybe you forgot to start the continuation line with RUN:? That's what was happening to me when I couldn't get it to work. See bad-output-format.test as an example.

There's no rule for short lines in tests, but when they get too long (>100 columns), I find it more readable to split them up around 80 columns.

[abrachet]

Split up long lines in invalid-preserve-dates.test

[abrachet]

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

8 ↗

(On Diff #203875)

Yup, that's what I was missing. Thanks, Jordan!

[jhenderson]

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

1 ↗

(On Diff #203884)

A brief explanation at the start of the file as to what this test file is covering would be useful.

4 ↗

(On Diff #203884)

Add a full stop to the end of comments, and use '##' to indicate they are comments for new tests.

7 ↗

(On Diff #203884)

I find it easier to read if the follow-on lines are indented for a long RUN command like this:

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

Depending on the length of the main command, it is often easier to reflow the lines at the end of a command too:

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

[abrachet]

Added a comment to test case explaining its purpose. Also reformatted long RUN: lines.

[jhenderson]

LGTM.

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

+## This tests for an expected error when --preserve dates is
+## specified at the same time as using stdin or stdout as input or
+## output files.
+
+# RUN: not llvm-objcopy --preserve-dates - %t 2>&1 | FileCheck %s
+# RUN: not llvm-objcopy --preserve-dates %p/Inputs/alloc-symtab.o - 2>&1 | FileCheck %s
+
+## Testing N args.
+# RUN: not llvm-strip --preserve-dates - < %p/Inputs/alloc-symtab.o 2>&1 | FileCheck %s
+# RUN: not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o - < \
+# RUN:      %p/Inputs/alloc-symtab.o 2>&1 | FileCheck %s
+# RUN: not llvm-strip --preserve-dates - %p/Inputs/alloc-symtab.o < \
+# RUN:      %p/Inputs/alloc-symtab.o 2>&1 | FileCheck %s
+# RUN: not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o - \
+# RUN:      %p/Inputs/alloc-symtab.o < %p/Inputs/alloc-symtab.o 2>&1 | FileCheck %s
+
+## Testing -o.
+# RUN: not llvm-strip --preserve-dates - -o %p/Inputs/alloc-symtab.o < \
+# RUN:      %p/Inputs/alloc-symtab.o 2>&1 | FileCheck %s
+# RUN: not llvm-strip --preserve-dates %p/Inputs/alloc-symtab.o -o - < \
+# RUN:      %p/Inputs/alloc-symtab.o 2>&1 | FileCheck %s
+
+# CHECK: error: --preserve-dates requires a file

llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp

Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   Config.AllowBrokenLinks = InputArgs.hasArg(OBJCOPY_allow_broken_links);
 
   Config.DeterministicArchives = InputArgs.hasFlag(
       OBJCOPY_enable_deterministic_archives,
       OBJCOPY_disable_deterministic_archives, /*default=*/true);
 
   Config.PreserveDates = InputArgs.hasArg(OBJCOPY_preserve_dates);
 
+  if (Config.PreserveDates &&
+      (Config.OutputFilename == "-" || Config.InputFilename == "-"))
+    return createStringError(errc::invalid_argument,
+                             "--preserve-dates requires a file");
+
   for (auto Arg : InputArgs)
     if (Arg->getOption().matches(OBJCOPY_set_start)) {
       auto EAddr = getAsInteger<uint64_t>(Arg->getValue());
       if (!EAddr)
         return createStringError(
             EAddr.getError(), "bad entry point address: '%s'", Arg->getValue());
 
       Config.EntryExpr = [EAddr](uint64_t) { return *EAddr; };
Expected<DriverConfig> parseStripOptions(ArrayRef<const char *> ArgsArr) {
   }
 
   if (InputArgs.hasArg(STRIP_version)) {
     outs() << "llvm-strip, compatible with GNU strip\n";
     cl::PrintVersionMessage();
     exit(0);
   }
 
-  SmallVector<const char *, 2> Positional;
+  SmallVector<StringRef, 2> Positional;
   for (auto Arg : InputArgs.filtered(STRIP_UNKNOWN))
     return createStringError(errc::invalid_argument, "unknown argument '%s'",
                              Arg->getAsString(InputArgs).c_str());
   for (auto Arg : InputArgs.filtered(STRIP_INPUT))
     Positional.push_back(Arg->getValue());
 
   if (Positional.empty())
     return createStringError(errc::invalid_argument, "no input file specified");
Expected<DriverConfig> parseStripOptions(ArrayRef<const char *> ArgsArr) {
 
   DriverConfig DC;
   if (Positional.size() == 1) {
     Config.InputFilename = Positional[0];
     Config.OutputFilename =
         InputArgs.getLastArgValue(STRIP_output, Positional[0]);
     DC.CopyConfigs.push_back(std::move(Config));
   } else {
-    for (const char *Filename : Positional) {
+    for (StringRef Filename : Positional) {
       Config.InputFilename = Filename;
       Config.OutputFilename = Filename;
       DC.CopyConfigs.push_back(Config);
     }
   }
 
+  if (Config.PreserveDates && (is_contained(Positional, "-") ||
+                               InputArgs.getLastArgValue(STRIP_output) == "-"))
+    return createStringError(errc::invalid_argument,
+                             "--preserve-dates requires a file");
+
   return std::move(DC);
 }
 
 } // namespace objcopy
 } // namespace llvm