[PATCH] D144596: Add extra parameter to thinlto-prefix-replace

[Ivan Tadeu Ferreira Antunes Filho via Phabricator via llvm-commits]

itf added inline comments.


================
Comment at: lld/test/MachO/thinlto-index-file-object-prefix-replace.ll:3
+; RUN: rm -rf %t ; split-file %s %t && cd %t
+; RUN: mkdir -p oldpath oldpath/prefix_replace
+
----------------
oontvoo wrote:
> MaskRay wrote:
> > int3 wrote:
> > > MaskRay wrote:
> > > > int3 wrote:
> > > > > itf wrote:
> > > > > > oontvoo wrote:
> > > > > > > This needs to go under `%t` (otherwise the tests may fail in our integrates because only stuff under `%t` is writeable)
> > > > > > is `cd %t` in the previous line not enough to keep the following commands under `%t` ?
> > > > > It is. However, none of the other tests do this, so I would prefer we avoid it unless it's required by the nature of this test
> > > > I support the use of `cd %t` as in `rm -rf %t && split-file %s %t && cd %t`, especially if there are many temporary/output files or complex file hierarchies. It's difficult to ensure a test doesn't write to other directories without `%t`.
> > > > 
> > > > (As mentioned elsewhere, Google has a simplified lit runner where CWD is the source directory instead of some directive relative to `%t`. And writing to the source directory will cause an error due to Bazel's sandbox.)
> > > aside from consistency, I like explicit paths in order that I can copy and paste individual lines from the verbose test output for debugging purposes. (I prefer to avoid environment variables in test for the same reasons.)
> > > 
> > > using a sandbox does seem like a better way of ensuring a test doesn't write outside of the test dir
> > > 
> > > but ok, I won't insist :)
> > OK:) I use my own simplified lit runner: https://github.com/MaskRay/Config/blob/master/home/bin/ff
> > 
> > ```
> > % cd lld/test/ELF/lto 
> > % ff thinlto-index-file.ll
> > 0  rm -rf a && mkdir a && cd a
> > 1  opt -module-summary ../thinlto-index-file.ll -o 1.o
> > 2  opt -module-summary $HOME/llvm/lld/test/ELF/lto/Inputs/thinlto.ll -o 2.o
> > 3  opt -module-summary $HOME/llvm/lld/test/ELF/lto/Inputs/thinlto_empty.ll -o 3.o
> > 4  ld.lld --plugin-opt=thinlto-index-only=1.txt -shared 1.o 2.o 3.o -o /dev/null
> > 5  FileCheck ../thinlto-index-file.ll < 1.txt
> > 6  ld.lld --thinlto-index-only=2.txt -shared 1.o 2.o 3.o -o /dev/null
> > 7  diff 1.txt 2.txt
> > ```
> > 
> > When I want to inspect something, I just `cd a`. So `cd %t` in the test file doesn't bother me that much :)
> > 
> > Another advantage is that some features depend on whether a stale output file exists. `rm -rf %t` makes the test immune to such bugs. With a lot of `%t1 %t2` it's tedious to do `rm -f %t1 %t2`
> +1 - also prefer the explicit paths taht I can copy without havign to look up where the current directory is
one of the problems of using full paths with %t is that then we cannot use 
`FileCheck --match-full-lines` 
it seems. Because 
`; CHECK: %t/foo.o `
Does not seem to expand `%t` before checking the content of the file.

We can only use FileCheck without --match-full-lines, which is a weaker test


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

https://reviews.llvm.org/D144596