[PATCH] D34732: Clean temp directories before running lit

Wed Jun 28 15:08:10 PDT 2017

As discussed on IRC, etc.

The split DWARF llvm-symbolizer tests are a bit finicky & had hardcoded in
the data files, the relative directory between the working directory where
llvm-symbolizer would be run from, and the location of the .dwo files that
it needed to load. This was "Output" originally, but needs to change for
the test directory layout changes you're proposing.

In theory, based on your results, it sounds like this should be "." now
(the .dwo file placed in the %T directory, and the %T should be the current
working directory of llvm-symbolizer). But that's not what I'm observing
locally - maybe there's some platform difference? Perhaps you could provide
the patch to llvm-symbolizer you made to test the working directory?

In any case, I made a review with all the split-dwarf fixes (and a bunch of
missing source files to help produce some of the binaries) here:
https://reviews.llvm.org/D34778

You can verify that the relative directory these tests are using is
'DebugInfo' by running llvm-dwarfdump -debug-dump=info on the .o or
executable files & see the DW_AT_compilation_dir attribute's value.

Also try running llvm-symbolizer from the various different directories
yourself to see when it successfully finds the .dwo files and when it
doesn't (general rule of thumb is that if it prints a simple function name
('f2') then it hasn't found the .dwo file, if it prints the mangled name
('_Z2f2v') then it has).

TBH there's still a few parts of that test that confuse me (well, the fact
that they pass with your change confuses me... but at least there's
something to start/discuss)

I'm worried there may be some non-portability if you were observing the
working directory as test/Output/DebugInfo and I seem to be observing it as
test/Output. If these tests work for you, that sort of points to the
working directory (at some level, in some way - maybe there's some specific
handling in llvm-symbolizer that's resolving the path not just relative to
the current working directory, but something else? Maybe we can
manipulate/fix that so it is test/Output/DebugInfo)  being test/Output.
Give it a go & see?

eg (showing that it passes when run from the test/Output directory, not the
test/Output/DebugInfo directory):

$
/usr/local/google/home/blaikie/dev/llvm/build/default/./bin/llvm-symbolizer
--functions=linkage --inlining --demangle=false     --default-arch=i386 <
/usr/local/google/home/blaikie/dev/llvm/build/default/test/Output/DebugInfo/llvm-symbolizer.test.tmp.input
| /usr/local/google/home/blaikie/dev/llvm/build/default/./bin/FileCheck
--check-prefix=SPLIT --check-prefix=DWO --check-prefix=EXTRA
/usr/local/google/home/blaikie/dev/llvm/src/test/DebugInfo/llvm-symbolizer.test
/usr/local/google/home/blaikie/dev/llvm/src/test/DebugInfo/llvm-symbolizer.test:129:13:
error: SPLIT-NEXT: is not on the line after the previous match
SPLIT-NEXT: {{.*}}split-dwarf-test.cc
            ^
<stdin>:111:1: note: 'next' match was here
Output/split-dwarf-test.cc:5:3
^
<stdin>:105:7: note: previous match ended here
_Z2f2v
      ^
<stdin>:106:1: note: non-matching line after previous match is here
Output/split-dwarf-dwp.cpp:3:3
^
blaikie at blaikie-linux2:~/dev/llvm/build/default$ cd test
blaikie at blaikie-linux2:~/dev/llvm/build/default/test$
/usr/local/google/home/blaikie/dev/llvm/build/default/./bin/llvm-symbolizer
--functions=linkage --inlining --demangle=false     --default-arch=i386 <
/usr/local/google/home/blaikie/dev/llvm/build/default/test/Output/DebugInfo/llvm-symbolizer.test.tmp.input
| /usr/local/google/home/blaikie/dev/llvm/build/default/./bin/FileCheck
--check-prefix=SPLIT --check-prefix=DWO --check-prefix=EXTRA
/usr/local/google/home/blaikie/dev/llvm/src/test/DebugInfo/llvm-symbolizer.test
/usr/local/google/home/blaikie/dev/llvm/src/test/DebugInfo/llvm-symbolizer.test:129:13:
error: SPLIT-NEXT: is not on the line after the previous match
SPLIT-NEXT: {{.*}}split-dwarf-test.cc
            ^
<stdin>:111:1: note: 'next' match was here
Output/split-dwarf-test.cc:5:3
^
<stdin>:105:7: note: previous match ended here
_Z2f2v
      ^
<stdin>:106:1: note: non-matching line after previous match is here
Output/split-dwarf-dwp.cpp:3:3
^
blaikie at blaikie-linux2:~/dev/llvm/build/default/test$ cd Output/
blaikie at blaikie-linux2:~/dev/llvm/build/default/test/Output$
/usr/local/google/home/blaikie/dev/llvm/build/default/./bin/llvm-symbolizer
--functions=linkage --inlining --demangle=false     --default-arch=i386 <
/usr/local/google/home/blaikie/dev/llvm/build/default/test/Output/DebugInfo/llvm-symbolizer.test.tmp.input
| /usr/local/google/home/blaikie/dev/llvm/build/default/./bin/FileCheck
--check-prefix=SPLIT --check-prefix=DWO --check-prefix=EXTRA
/usr/local/google/home/blaikie/dev/llvm/src/test/DebugInfo/llvm-symbolizer.test
blaikie at blaikie-linux2:~/dev/llvm/build/default/test/Output$ cd DebugInfo/
blaikie at blaikie-linux2:~/dev/llvm/build/default/test/Output/DebugInfo$
/usr/local/google/home/blaikie/dev/llvm/build/default/./bin/llvm-symbolizer
--functions=linkage --inlining --demangle=false     --default-arch=i386 <
/usr/local/google/home/blaikie/dev/llvm/build/default/test/Output/DebugInfo/llvm-symbolizer.test.tmp.input
| /usr/local/google/home/blaikie/dev/llvm/build/default/./bin/FileCheck
--check-prefix=SPLIT --check-prefix=DWO --check-prefix=EXTRA
/usr/local/google/home/blaikie/dev/llvm/src/test/DebugInfo/llvm-symbolizer.test
/usr/local/google/home/blaikie/dev/llvm/src/test/DebugInfo/llvm-symbolizer.test:129:13:
error: SPLIT-NEXT: is not on the line after the previous match
SPLIT-NEXT: {{.*}}split-dwarf-test.cc
            ^
<stdin>:111:1: note: 'next' match was here
Output/split-dwarf-test.cc:5:3
^
<stdin>:105:7: note: previous match ended here
_Z2f2v
      ^
<stdin>:106:1: note: non-matching line after previous match is here
Output/split-dwarf-dwp.cpp:3:3
^

On Wed, Jun 28, 2017 at 10:16 AM Reid Kleckner via Phabricator <
reviews at reviews.llvm.org> wrote:

> rnk added inline comments.
>
>
> ================
> Comment at: llvm/utils/lit/lit/run.py:254
> +        # runs' output.
> +        import time
> +        start = time.time()
> ----------------
> The time module is already available at global scope
>
>
> ================
> Comment at: llvm/utils/lit/lit/run.py:260
> +        clean_paths = list(clean_paths)
> +        clean_paths.sort(key=lambda x: len(x.split(os.sep)))
> +        for base in clean_paths:
> ----------------
> Can you add a comment, something like "Sort the list by number of path
> components so that parent directories come before their children."
>
>
> ================
> Comment at: llvm/utils/lit/lit/run.py:264
> +                if not os.path.islink(base) and os.path.isdir(base):
> +                    from shutil import rmtree
> +                    rmtree(base, True)
> ----------------
> Let's just import shutil at global scope and do shutil.rmtree.
>
>
> ================
> Comment at: llvm/utils/lit/lit/run.py:271
> +        end = time.time()
> +        print("Cleanup took {} seconds".format(end - start))
> +
> ----------------
> Should we print this at all? If we decide we want it, should we print with
> litConfig.note()?
>
>
> https://reviews.llvm.org/D34732
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170628/6feea3da/attachment.html>