[PATCH] D72357: Fix llvm-objcopy when directory contains "bar".

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test:6
 # RUN: llvm-objcopy -j .other.section %t.o %t2.o
-# RUN: llvm-readobj --symbols %t2.o | FileCheck %s --implicit-check-not=bar
+# RUN: llvm-readobj --symbols %t2.o | FileCheck %s --implicit-check-not=barrr
 
----------------
MaskRay wrote:
> jhenderson wrote:
> > rupprecht wrote:
> > > MaskRay wrote:
> > > > MaskRay wrote:
> > > > > rupprecht wrote:
> > > > > > MaskRay wrote:
> > > > > > > `--implicit-check-not='Name: bar'`
> > > > > > I assume the match you're getting is the first line from llvm-readobj:
> > > > > > 
> > > > > > ```
> > > > > > File: /Users/jlebar/path/to/something.o
> > > > > > Format: ELF64-x86-64
> > > > > > Arch: x86_64
> > > > > > AddressSize: 64bit
> > > > > > ```
> > > > > > 
> > > > > > Piping from stdin will remove that filename; can you instead try:
> > > > > > ```
> > > > > > # RUN: llvm-readobj --symbols < %t2.o | FileCheck %s --implicit-check-not=bar
> > > > > > ```
> > > > > Using `< file` input redirection may leave us in a very inconvenient situation. Maybe we should just be more careful when using `--implicit-check-not`, instead of replacing every single use of `llvm-readobj %t` with `llvm-readobj < %t`.
> > > > I'd like to hear @jhenderson what wants to say here.
> > > When you say it's inconvenient, can you clarify why?
> > > 
> > > The practice of using piping to avoid the filename present is documented here, albeit for a different tool (opt instead of llvm-readobj): http://llvm.org/docs/TestingGuide.html#fragile-tests
> > This isn't likely to affect too many people, but if a test redirects from stdin, it is a pain to rerun a part of it in my default natural environment, Windows PowerShell, because PowerShell doesn't support the '<' redirection command for some unknown reason.
> > 
> > MaskRay's suggestions in this test would seem more preferable to me therefore, and less fragile for other reasons too, I reckon: "Name: bar" is much less likely to appear anywhere in the output beyond the bit being checked. Also, "bar" is a common enough three-letter pattern to appear in another word that even shell redirection won't necessarily solve it in all cases or could prove flaky in the future if more output is added to llvm-readobj.
> @rupprecht Do we have a consensus now?
> This isn't likely to affect too many people, but if a test redirects from stdin, it is a pain to rerun a part of it in my default natural environment, Windows PowerShell, because PowerShell doesn't support the '<' redirection command for some unknown reason.
> 

There seems to be thousands of llvm-based tests already that use use < as a pipe. It's a shame PowerShell doesn't support it.
PowerShell does seem to have a replacement called `Get-Content`. I wonder if we could have lit stdout rewrite the RUN commands so "foo < x y z" is printed out as "Get-Content x | foo y z"?

```
$ find . -type f -path '*test*' -print0 | xargs -0 grep -l 'RUN.*llvm-.*<' | wc -l
3704
```

PowerShell does seem to support the pipe operator, so `cat %t2.o | llvm-readobj --symbols` might also work, although I only see one test that uses cat this way. (I see lots of `cat ... | FileCheck`, but nothing piping into a tool). I think if we want to support PowerShell we should do something about the thousands of tests and the documentation using PowerShell-incompatible features.

Anyway... I'm still slightly leaning toward `RUN: llvm-readobj --symbols < %t2.o` because (1) it matches what the rest of llvm testing does, and (2) it actually prevents full filenames from leaking into the tool output, so you don't have to play games with more advanced FileCheck options. (There is a risk that the filename might find some other way to creep in, but I think that's mostly theoretical).
OTOH, I also don't want these tests to be failing for users based on their username, so either patch is better than nothing & perfect should not be the enemy of good and all that, so if nobody else likes `<` then feel free to submit whichever patch you like.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72357