[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

812–820

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

88 ↗

(On Diff #203980)

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

816

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

817

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

812–820

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
  }

814

llvm::is_contained(InputFiles, Filename)

817

+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

812–820

Thanks!

817

@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

1

strip -> llvm-strip

and add a brief explanation of what is expected.

17–20

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

817

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

817

+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

88 ↗

(On Diff #203980)

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

57 ↗

(On Diff #204417)

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

194 ↗

(On Diff #204431)

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

57 ↗

(On Diff #204417)

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

194 ↗

(On Diff #204431)

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

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

57 ↗

(On Diff #204417)

You're right. I put it back.

[jhenderson]

Looks good, aside from the comment.

llvm/tools/llvm-objcopy/CopyConfig.cpp

725–726

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

722

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

[abrachet]

llvm/tools/llvm-objcopy/CopyConfig.cpp

722

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

722

• 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 ↗

(On Diff #204898)

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

[abrachet]

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

85 ↗

(On Diff #204898)

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 ↗

(On Diff #204898)

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 strip using the same input file more than once.
+
+# 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:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]

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"
+#include "llvm/Support/WithColor.h"
 #include <memory>
 
 namespace llvm {
 namespace objcopy {
 
+extern StringRef ToolName;
+
+static void reportWarning(Twine Message) {
+  WithColor::warning(errs(), ToolName) << Message << "\n";
+  errs().flush();
+}
+
 namespace {
 enum ObjcopyID {
   OBJCOPY_INVALID = 0, // This is not an option ID.
 #define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
                HELPTEXT, METAVAR, VALUES)                                      \
   OBJCOPY_##ID,
 #include "ObjcopyOpts.inc"
 #undef OPTION
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) {
   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);
   }
 
   if (InputArgs.hasArg(STRIP_help)) {
     T.PrintHelp(outs(), "llvm-strip [options] file...", "strip tool");
     exit(0);
   }
 
   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) {
+    StringMap<int> 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");
+        reportWarning("'" + Filename + "' was already specified");
+      }
       Config.InputFilename = Filename;
       Config.OutputFilename = Filename;
       DC.CopyConfigs.push_back(Config);
     }
   }
 
   return std::move(DC);
 }
 
 } // namespace objcopy
 } // namespace llvm