Fix llvm-objcopy when directory contains "bar".

[jlebar]

Summary

My home directory, "/Users/jlebar", appears in the output of many tools.
Tests which naively check that "bar" does not appear in a tools' output
will fail for me.

I've "fixed" two llvm-objcopy tests here. It's pretty hacky, and if
anyone has "barrr" in their username, they'll hit the same problem.

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

11

# BAR: Name: bar

[rupprecht]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

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

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

--implicit-check-not='Name: bar'

6

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.

6

I'd like to hear @jhenderson what wants to say here.

[rupprecht]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

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

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

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.

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

@rupprecht Do we have a consensus now?

[rupprecht]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

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.

[dblaikie]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

Yeah - please stick with "<" given the precedent in the project already. If there's a systemic issue with that that could be addressed by a similarly legible alternative or improvements to Lit, I think that's worth discussing separately.

(though it's not clear to me if @MaskRay's "Using < file input redirection may leave us in a very inconvenient situation. " refers to the issue that @jhenderson mentioned, or was alluding to some other, as yet unclarified, inconvenience - if there's some other unknowns here, happy to discuss them - though, again, given the precedent, please go ahead with using "<" (if it works as well here as in the many other tests it's used in) and start a separate llvm-dev discussion, perhaps, on addressing those other issues)

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

I should have been clearer. Some GNU binutils (nm, objdump, size) have a default file name: "a.out", when no filename is specified. llvm-nm, llvm-objdump, llvm-size and llvm-dwarfdump learned this rule from GNU binutils. I have a weak preference not doing this, but I don't have a strong enough motivation to change it, and I feel someone may report bugs if LLVM binary utilities don't have the default file name.

For such utilities, llvm-nm < %s will not read from stdin. If we want to change some utilities (llvm-objcopy in question) to follow the convention of traditional LLVM tools (llvm-as/llc/opt/etc), the aforementioned tools will inevitably cause inconsistency which cannot be fixed.

A balance between "don't let users report bugs" and "don't let such behavior get in our way" is not changing our current practice, but making the tests a bit more robust (D72358). By doing that, we can circumvent the problem we encountered by @jlebar and @bbaren. I did worry whether that would bring more mental burden when writing tests, but from my experience in D72358 it does not look that bad.

[dblaikie]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

If any of our tools have such default behavior and need it, they should have that behavior - our testing strategy shouldn't dictate feature usability.

But I'd still advocate for the same testing strategy using such a feature, using "toolname - < inputname" to tell the tool to read from stdin, then pipe the file into stdin. The existence of a "no file name means a.out" doesn't change the general approach, in my opinion.

I'd rather not circumvent only the current problem - all sorts of other substrings may appear in a user's path that might confuse tests & we should avoid having the path in the test at all. (yeah, we don't do this for all tests - but I think when it comes up it's worth fixing firmly & avoiding the input name appearing anywhere in the checked output)

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

% size - < a
size: '-': No such file

Well, this can be a feature request to GNU binutils.

Doing things differently for llvm-size - < a and llvm-readobj -d < a seem a bit inconvenient.

For consistency, we may need to change every binary utilities use case to either - < a or < a. I acknowledge the filename-in-output problem, but am worried that the proposed fix is causing too much burden.

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

We have 700+ occurrences of llvm-objcopy, nears 4000 occurrences of llvm-readobj in tests. Very few of them have the problem. Adapting every test case to be consistent with 2 llvm-objcopy tests is too big a hammer.

[dblaikie]

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

6

I'm not suggesting changing all occurrences - though I am suggesting that piping input is best practice in LLVM for writing input tests (& this extends beyond objcopy and readobj - similarly for LLVM IR tests too, etc - piping input ensures the on-disk path doesn't appear in the output in a number of different tools) & the preferred solution when tests end up having these sorts of problems.

[jhenderson]

(taking the discussion out-of-line, so that it's easier to read and quote)

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"?

FWIW, I don't think complicating lit to support one specific shell (PowerShell) is that great an idea, given there's a straightforward enough workaround for me (drop down to cmd). It's inconvenient but I can handle it where it is needed (but I wouldn't want it to be more widespread than necessary).

I actually find something like yaml2obj %s | llvm-readobj --symbols - (or similar) more inconvenient, since it means there's no object on disk that can be easily inspected to see what the problem is when maintaining the test. I don't think this is being suggested here though.

Some GNU binutils (nm, objdump, size) have a default file name: "a.out", when no filename is specified. llvm-nm, llvm-objdump, llvm-size and llvm-dwarfdump learned this rule from GNU binutils. I have a weak preference not doing this, but I don't have a strong enough motivation to change it, and I feel someone may report bugs if LLVM binary utilities don't have the default file name.

We shouldn't change the behaviour of tools with regards to a.out. This was attempted last year by @abrachet and reverted after various objections and problems, since there are configure scripts out there that assume the name is that.

Checks like # BAR: bar should be avoided regardless of path issues, as they have too great a chance of matching something else spuriously (there are plenty of words in the English language that have "bar" as a substring). Changing to redirecting via stdin doesn't solve this. Using something like barrr is less of an issue, but providing the context of the Name: field name makes the test more robust and more readable in my opinion. The chance of "Name: bar" appearing in somebody's path is minimal at best, and is even less likely if you add the end of line {{$}} pattern to the check.

[jlebar]

Thank you for the reviews, and I'm sorry I dropped off the face of the earth here! I was messing with my phab settings and I think I somehow disabled all emails. Oops.

Looks like Ray fixed this in the meantime (D72358), so dropping this change.

llvm/test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test

 ## Here we want to check that llvm-objcopy removes an undefined symbol
 ## if all references to it have been stripped.
 
 # RUN: yaml2obj --docnum=1 %s -o %t.o
 # 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
 
 # RUN: llvm-objcopy -j .text -j .rela.text1 %t.o %t2.o
 # RUN: llvm-readobj --symbols %t2.o | FileCheck %s --check-prefix=BAR
 
-# BAR: bar
+# BAR: barrr
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
   - Name:        .text
     Type:        SHT_PROGBITS
   - Name:        .rela.text1
     Type:        SHT_RELA
     Relocations:
       - Offset:  0x0000000000000001
-        Symbol:  bar
+        Symbol:  barrr
         Type:    R_X86_64_32
   - Name:        .rela.text2
     Type:        SHT_RELA
     Relocations:
       - Offset:  0x0000000000000001
-        Symbol:  bar
+        Symbol:  barrr
         Type:    R_X86_64_32
   - Name:        .other.section
     Type:        SHT_PROGBITS
 Symbols:
-  - Name:    bar
+  - Name:    barrr
     Binding: STB_GLOBAL
 ...
 
 ## Check we remove unreferenced undefined symbols, even if
 ## they weren't previously referenced. This follows GNU.
 
 # RUN: yaml2obj --docnum=2 %s -o %t.o
 # RUN: llvm-objcopy -j .keep_me %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
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
   - Name: .keep_me
     Type: SHT_PROGBITS
 Symbols:
-  - Name:    bar
+  - Name:    barrr
     Binding: STB_GLOBAL
 ...

llvm/test/tools/llvm-objcopy/ELF/tail-merged-string-tables.test

 ## This test shows that when symbol/section names are optimized in the string
 ## table to share the same entry, due to tail merging, llvm-objcopy operations
 ## function correctly without corrupting the names of unchanged sections/symbols.
 
 # RUN: yaml2obj %s -o %t.o
 ## Sanity check that the strings have been pooled.
 # RUN: llvm-readobj %t.o --string-dump .strtab --string-dump .shstrtab \
-# RUN:   | FileCheck %s --check-prefix=VALIDATE --implicit-check-not=bar --implicit-check-not=.blag
+# RUN:   | FileCheck %s --check-prefix=VALIDATE --implicit-check-not=barrr --implicit-check-not=.blag
 
 # VALIDATE: String dump of section '.strtab':
-# VALIDATE: bazfoobar
+# VALIDATE: bazfoobarrr
 # VALIDATE: String dump of section '.shstrtab':
 # VALIDATE: .blam.blah.blag
 
 ## Case 1: Basic copy.
 # RUN: llvm-objcopy %t.o %t.copy
 # RUN: llvm-readobj --section-headers --symbols %t.copy | FileCheck %s --check-prefix=COPY
 
 # COPY: Sections [
 # COPY:   Name: .blam.blah.blag (
 # COPY:   Name: .blah.blag (
 # COPY:   Name: .blag (
 # COPY: Symbols [
-# COPY:   Name: bar (
-# COPY:   Name: foobar (
-# COPY:   Name: bazfoobar (
+# COPY:   Name: barrr (
+# COPY:   Name: foobarrr (
+# COPY:   Name: bazfoobarrr (
 
 ## Case 2: Rename section.
 # RUN: llvm-objcopy %t.o %t.rename-section --rename-section .blah.blag=.blob.blab
 # RUN: llvm-readobj --section-headers %t.rename-section \
 # RUN:   | FileCheck %s --check-prefix=RENAME-SECTION
 
 # RENAME-SECTION: Sections [
 # RENAME-SECTION:   Name: .blam.blah.blag (
 # RENAME-SECTION:   Name: .blob.blab (
 # RENAME-SECTION:   Name: .blag (
 
 ## Case 3: Rename symbol.
-# RUN: llvm-objcopy %t.o %t.rename-symbol --redefine-sym foobar=raboof
+# RUN: llvm-objcopy %t.o %t.rename-symbol --redefine-sym foobarrr=raboof
 # RUN: llvm-readobj --symbols %t.rename-symbol \
 # RUN:   | FileCheck %s --check-prefix=RENAME-SYMBOL
 
 # RENAME-SYMBOL: Symbols [
-# RENAME-SYMBOL:   Name: bar (
+# RENAME-SYMBOL:   Name: barrr (
 # RENAME-SYMBOL:   Name: raboof (
-# RENAME-SYMBOL:   Name: bazfoobar (
+# RENAME-SYMBOL:   Name: bazfoobarrr (
 
 ## Case 4: Remove section.
 # RUN: llvm-objcopy %t.o %t.remove-section -R .blah.blag
 # RUN: llvm-readobj --section-headers %t.remove-section \
 # RUN:   | FileCheck %s --check-prefix=REMOVE-SECTION --implicit-check-not .blah.blag
 
 # REMOVE-SECTION: Sections [
 # REMOVE-SECTION:   Name: .blam.blah.blag (
 # REMOVE-SECTION:   Name: .blag (
 
 ## Case 5: Remove symbol.
-# RUN: llvm-objcopy %t.o %t.remove-symbol -N foobar
+# RUN: llvm-objcopy %t.o %t.remove-symbol -N foobarrr
 # RUN: llvm-readobj --symbols %t.remove-symbol \
-# RUN:   | FileCheck %s --check-prefix=REMOVE-SYMBOL --implicit-check-not foobar
+# RUN:   | FileCheck %s --check-prefix=REMOVE-SYMBOL --implicit-check-not foobarrr
 
 # REMOVE-SYMBOL: Symbols [
-# REMOVE-SYMBOL:   Name: bar (
-# REMOVE-SYMBOL:   Name: bazfoobar (
+# REMOVE-SYMBOL:   Name: barrr (
+# REMOVE-SYMBOL:   Name: bazfoobarrr (
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
   - Name: .blam.blah.blag
     Type: SHT_PROGBITS
   - Name: .blah.blag
     Type: SHT_PROGBITS
   - Name: .blag
     Type: SHT_PROGBITS
 Symbols:
-  - Name: bar
-  - Name: foobar
-  - Name: bazfoobar
+  - Name: barrr
+  - Name: foobarrr
+  - Name: bazfoobarrr