[PATCH] D63122: [llvm-strip] Error when using stdin twice

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 11 inline comments as done.
abrachet added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test:6
+
+# RUN: cp %p/Inputs/dynrel.elf dynrel.elf
+# RUN: llvm-strip dynrel.elf dynrel.elf 2>&1 \
----------------
jhenderson wrote:
> abrachet wrote:
> > I cp to file in the current directory because doing '%p/Inputs/dynrel.elf' on line 9 did not expand the %p.
> '%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.
Very helpful, thank you!


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:805
   } else {
-    for (const char *Filename : Positional) {
+    StringSet<> InputFiles;
+    for (StringRef Filename : Positional) {
----------------
rupprecht wrote:
> abrachet wrote:
> > 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. 
> 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
>   }
> ```
Thanks!


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:810
+          return createStringError(errc::invalid_argument, "cannot specify '-' as an input file more than once");
+        reportWarning("'" + Filename + "' was already specified");
+      }
----------------
rupprecht wrote:
> jhenderson wrote:
> > 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
> +1, if possible we should not be calling back into llvm-objcopy.cpp
@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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63122/new/

https://reviews.llvm.org/D63122