[llvm-strip] Error when using stdin twice

[abrachet]

Summary

Implements bug 42204. llvm-strip now warns when the same input file is used more than once, and errors when stdin is used more than once.

[abrachet]

llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test

7

I cp to file in the current directory because doing '%p/Inputs/dynrel.elf' on line 9 did not expand the %p.

llvm/tools/llvm-objcopy/CopyConfig.cpp

816–826

I don't know why this is the case but the compiler was not content with just StringSet and asked if I meant StringRef. But adding the <> fixed this.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

87

The objcopy::error functions explicitly flush errs, I don't know why though because it appears to be unbuffered, but I figured I would follow how the rest of the code does it.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test

2

A comment at the start of the test briefly explaining its purpose would be useful.

Rather than relying on a pre-canned binary throughout this test, use a temporary object created by yaml2obj, as we want to get rid of all such objects if possible. Additionally, with llvm-strip tests in particular NEVER used pre-canned objects without copying them to a temporary location first, because llvm-strip modifies in place and could corrupt your test input in some way.

5

I find it a little easier to read if there's a blank line between RUN lines and CHECK lines

7

'%p' is a lit substitution for RUN lines only, whereas line 9 is a piece of text consumed by FileCheck, which doesn't know about lit substitutions. Instead, you can use FileCheck's -D option to define a variable that you can search for. See https://llvm.org/docs/CommandGuide/FileCheck.html#cmdoption-d-var. This then changes your FileCheck command and WARN check line to something like:

# RUN: ... | FileCheck -check-prefix=WARN %s -DFILE=%p/Inputs/dynrel.elf
# WARN: warning: '[[FILE]]' was already specified

You shouldn't copy a file to an unknown directory, because it's possible that another test is doing the same thing resulting in race conditions and flaky tests. If you ever do need to copy a file, copy it to a path involving %t.

11

FWIW, you don't need this line anyway, because it will still be created somewhere in the output directory.

llvm/tools/llvm-objcopy/CopyConfig.cpp

820

Looks like you haven't run clang-format here.

821

As you are planning on librarifying llvm-objcopy, be VERY wary about adding warnings directly in the code that might be eventually used in the library. If you haven't already, take a look at my lightning talk from last year's US Developers' meeting, which includes some tips and gotchas on this sort of thing:

https://www.youtube.com/watch?v=YSEY4pg1YB0

[rupprecht]

llvm/tools/llvm-objcopy/CopyConfig.cpp

816–826

What about using a StringMap to track the actual counts so you only report the first time a file is included multiple times?

StringMap<int> InputFileMap;
for (StringRef Filename : Positional)
  if (InputFileMap[Filename]++ == 1) {
    if (Filename == "-") return createStringError(...);
    reportWarning(...); // Only reported the first time we see a dupe file
  }

818

llvm::is_contained(InputFiles, Filename)

821

+1, if possible we should not be calling back into llvm-objcopy.cpp

[abrachet]

Update test case. Only warn the first time a file is recognized again.

[abrachet]

llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test

7

Very helpful, thank you!

llvm/tools/llvm-objcopy/CopyConfig.cpp

816–826

Thanks!

821

@jhenderson How would you recommend dealing with warnings in this case? I definitely agree with your sentiment and think failing at the local site is generally bad. Not sure how to deal with warnings though, llvm::Error is pretty binary it seems to me, either error or success.

[abrachet]

Added period at the end of comment.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test

2

strip -> llvm-strip

and add a brief explanation of what is expected.

18–21

Nit: remove the extra whitespace here and below so that each block has a lined-up column as far to the left as possible, i.e:

Class:   ELFCLASS64
Data:    ELFDATA2LSB
Type:    ET_REL
Machine: EM_X86_64

llvm/tools/llvm-objcopy/CopyConfig.cpp

821

The exact method for warnings probably depends on what you are planning on doing. In this particular case, I'd recommend probably passing in some form of handler (e.g. a callback function) which is defined by the caller. It would take a string (or possibly an llvm::Error) and report the warning in a manner applicable to the specific application. I'd then leave reportWarning in the original location and pass it in.

If you went the llvm::Error route, you could have the callback return an llvm::Error containing any unhandled issues, which is checked in this code to see if it should continue or not.

[rupprecht]

llvm/tools/llvm-objcopy/CopyConfig.cpp

821

+1 to a callback that logs an Error, and use createFileError instead of manually crafting the filename into the error string

The callback mechanism described in the talk can be seen here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp#L837

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

87

yep, stderr is unbuffered and doesn't require flushing -- it could be removed from there too

[abrachet]

Report warnings through a callback instead of locally.

[abrachet]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

56

I removed this in this patch because Jordan said it could be removed. Is it okay to sneak this one in this patch or should I separate this?

[abrachet]

Switched from passing createFileError to createStringError to the callback. The string created from from toString(Error) by an Error created from createFileError includes the corresponding string of the error_code. But createStringError omits it. Now when warning about the same file twice, the warning will be "warning: '[[FILE]]' was already specified" not "warning: '[[FILE]]' was already specified: Invalid argument"

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test

25

Not that it matters, but I'd usually line up each block independently (i.e. I think you could remove an extra space from everything inside "Sections").

llvm/tools/llvm-objcopy/CopyConfig.h

193

I'd probably change the name to just "ErrorCallback", and put a note in the comment explaining what this function is supposed to do. Since it's recoverable, and the program might be able to continue, I think it makes sense to return Error, and let the parseStripOptions code check the return value to decide if it is safe to continue or not.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

56

Whilst I'd probably do it, strictly it should probably be in its own micro-patch that doesn't need reviewing (as long as you build and run the tests).

[abrachet]

Propagate error values returned from recoverable error callback to parseStripOptions

[abrachet]

llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test

25

Wasn't quite sure which white space you were referring to so I copied this from one of your tests.

llvm/tools/llvm-objcopy/CopyConfig.h

193

How does this look? It is used down on line 816 of CopyConfig.cpp

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

56

You're right. I put it back.

[jhenderson]

Looks good, aside from the comment.

llvm/tools/llvm-objcopy/CopyConfig.cpp

726–729

Maybe rephrase this slightly:

"handle recoverable errors. An Error returned by the callback aborts the parsing and is then returned by this function"

I'd also move the ErrorCallback description up to its declaration.

[abrachet]

Changed comment explaining warning callback.

[jhenderson]

LGTM, aside from one comment.

llvm/tools/llvm-objcopy/CopyConfig.cpp

723

This comment is identical to its declaration, which seems wrong. Could it just be deleted entirely?

[abrachet]

llvm/tools/llvm-objcopy/CopyConfig.cpp

723

I agree. It was like this before so I figured I would just follow how it was. Should I remove my part or the comment entirely?

[rupprecht]

llvm/tools/llvm-objcopy/CopyConfig.cpp

723

• The method is already commented in the header file; the .cpp files don't need comments. (Same for parseObjcopyOptions). They will inevitably go out of sync.
• IMO it's fine to just not update the comment in this patch, and in a separate commit cleanup the header comments.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

85

This doesn't need to have a return value, it should be void.

[abrachet]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

85

This was per @jhenderson’s suggestion, it lets the callback decide wether to continue parsing or not. Of course this one doesn’t do that right now, but allows for it in the future. I don’t have strong feelings either way.

[rupprecht]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

85

No, rereading that thread, I agree with how it's written. Makes sense to return Error so a different caller could treat warnings as fatal.

[abrachet]

Removed comment in source file, keeping only the comment in the header.

llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test

+## Test llvm-strip using the same input file more than once.
+## When using stdin ('-') more than once llvm-strip should give an error
+## while a file more than once should be simply a warning.
+
+# RUN: yaml2obj %s -o %t
+
+# RUN: not llvm-strip - - < %t 2>&1 | FileCheck -check-prefix=ERR %s
+# RUN: not llvm-strip - %t - < %t 2>&1 | FileCheck -check-prefix=ERR %s
+
+# ERR: error: cannot specify '-' as an input file more than once
+
+# RUN: llvm-strip %t %t 2>&1 | FileCheck -check-prefix=WARN %s -DFILE=%t
+# RUN: llvm-strip %t %t %t 2>&1 | FileCheck -check-prefix=WARN %s -DFILE=%t
+
+# WARN: warning: '[[FILE]]' was already specified
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name:  .alloc
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_ALLOC ]

llvm/tools/llvm-objcopy/CopyConfig.h

 
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
 Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr);
 
 // ParseStripOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseStripOptions will print the help messege and
-// exit.
-Expected<DriverConfig> parseStripOptions(ArrayRef<const char *> ArgsArr);
+// exit. ErrorCallback is used to handle recoverable errors. An Error returned
+// by the callback aborts the parsing and is then returned by this function.
+Expected<DriverConfig>
+parseStripOptions(ArrayRef<const char *> ArgsArr,
+                  std::function<Error(Error)> ErrorCallback);
 
 } // namespace objcopy
 } // namespace llvm
 
 #endif

llvm/tools/llvm-objcopy/CopyConfig.cpp

 //===- CopyConfig.cpp -----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CopyConfig.h"
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/JamCRC.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/StringSaver.h"
if (Config.ExtractPartition && Config.ExtractMainPartition)
     return createStringError(errc::invalid_argument,
                              "cannot specify --extract-partition together with "
                              "--extract-main-partition");
 
   DC.CopyConfigs.push_back(std::move(Config));
   return std::move(DC);
 }
 
 // ParseStripOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseStripOptions will print the help messege and
 // exit.
-Expected<DriverConfig> parseStripOptions(ArrayRef<const char *> ArgsArr) {
+Expected<DriverConfig>
+parseStripOptions(ArrayRef<const char *> ArgsArr,
+                  std::function<Error(Error)> ErrorCallback) {
   StripOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
     T.PrintHelp(errs(), "llvm-strip [options] file...", "strip tool");
     exit(1);
   }
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 {
+    StringMap<unsigned> InputFiles;
     for (StringRef Filename : Positional) {
+      if (InputFiles[Filename]++ == 1) {
+        if (Filename == "-")
+          return createStringError(
+              errc::invalid_argument,
+              "cannot specify '-' as an input file more than once");
+        if (Error E = ErrorCallback(createStringError(
+                errc::invalid_argument, "'%s' was already specified",
+                Filename.str().c_str())))
+          return std::move(E);
+      }
       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

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

 
 namespace llvm {
 namespace objcopy {
 
 // The name this program was invoked as.
 StringRef ToolName;
 
 LLVM_ATTRIBUTE_NORETURN void error(Twine Message) {
   WithColor::error(errs(), ToolName) << Message << "\n";
   exit(1);
 }
 
 LLVM_ATTRIBUTE_NORETURN void error(Error E) {
   assert(E);
   std::string Buf;
   raw_string_ostream OS(Buf);
   logAllUnhandledErrors(std::move(E), OS);
LLVM_ATTRIBUTE_NORETURN void reportError(StringRef File, Error E) {
   std::string Buf;
   raw_string_ostream OS(Buf);
   logAllUnhandledErrors(std::move(E), OS);
   OS.flush();
   WithColor::error(errs(), ToolName) << "'" << File << "': " << Buf;
   exit(1);
 }
 
+ErrorSuccess reportWarning(Error E) {
+  assert(E);
+  WithColor::warning(errs(), ToolName) << toString(std::move(E));
+  return Error::success();
+}
+
 } // end namespace objcopy
 } // end namespace llvm
 
 using namespace llvm;
 using namespace llvm::object;
 using namespace llvm::objcopy;
 
 // For regular archives this function simply calls llvm::writeArchive,
static Error executeObjcopy(const CopyConfig &Config) {
   return Error::success();
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];
   bool IsStrip = sys::path::stem(ToolName).contains("strip");
   Expected<DriverConfig> DriverConfig =
-      IsStrip ? parseStripOptions(makeArrayRef(argv + 1, argc))
+      IsStrip ? parseStripOptions(makeArrayRef(argv + 1, argc), reportWarning)
               : parseObjcopyOptions(makeArrayRef(argv + 1, argc));
   if (!DriverConfig) {
     logAllUnhandledErrors(DriverConfig.takeError(),
                           WithColor::error(errs(), ToolName));
     return 1;
   }
   for (const CopyConfig &CopyConfig : DriverConfig->CopyConfigs) {
     if (Error E = executeObjcopy(CopyConfig)) {
       logAllUnhandledErrors(std::move(E), WithColor::error(errs(), ToolName));
       return 1;
     }
   }
 
   return 0;
 }