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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test:1
+# RUN: not llvm-strip - - < %p/Inputs/dynrel.elf 2>&1 | FileCheck -check-prefix=ERR %s
+# RUN: not llvm-strip - %p/Inputs/dynrel.elf - < %p/Inputs/dynrel.elf 2>&1 \
----------------
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.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test:4
+# RUN:     | FileCheck -check-prefix=ERR %s
+# ERR: error: cannot specify '-' as an input file more than once
+
----------------
I find it a little easier to read if there's a blank line between RUN lines and CHECK lines


================
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 \
----------------
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.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test:10
+# WARN: warning: 'dynrel.elf' was already specified
+# RUN: rm dynrel.elf
----------------
FWIW, you don't need this line anyway, because it will still be created somewhere in the output directory.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:809
+        if (Filename == "-")
+          return createStringError(errc::invalid_argument, "cannot specify '-' as an input file more than once");
+        reportWarning("'" + Filename + "' was already specified");
----------------
Looks like you haven't run clang-format here.


================
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");
+      }
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63122