[PATCH] D52638: [XRay] Fix fdr-thread-order.cc when current directory contains fdr-thread-order.cc

[Dean Michael Berris via Phabricator via llvm-commits]

dberris accepted this revision.
dberris added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: test/xray/TestCases/Posix/fdr-thread-order.cc:2
+// RUN: rm -rf %t && mkdir %t
+// RUN: %clangxx_xray -g -std=c++11 %s -o %t.exe
+// RUN: XRAY_OPTIONS="patch_premain=false \
----------------
MaskRay wrote:
> dberris wrote:
> > I'm not a fan of the `.exe` extension here -- consider `-bin` instead?
> I am not a fan of Windows but chose `.exe` because I've seen `%t.exe` used in llvm-readobj gold lld tests. They are also used in `compiler-rt/test/asan/TestCases/{Posix,Linux,Darwin}`. In ELF, such executables are called `ET_EXEC` before PIE era. The `.exe` suffix should not be too exotic.
I see, that's a convincing reason to pick `.exe` too then. :)


================
Comment at: test/xray/TestCases/Posix/fdr-thread-order.cc:12
-// RUN:    FileCheck %s --check-prefix TRACE
-// RUN: rm fdr-thread-order.*
 // FIXME: Make llvm-xray work on non-x86_64 as well.
----------------
MaskRay wrote:
> dberris wrote:
> > Unfortunately, doing this makes it really easy to clog up the filesystem with log files upon failure (which I learned the hard way while working on these). The suggestion has been to re-create the sequence by hand if we want to debug, rather than to keep them lying around when the tests fail.
> > 
> > Thoughts?
> If the tests use unique directory names to isolate, it shouldn't clog up the filesystem.
> 
> Are sizes of log files an issue? If not, I think not cleaning the temporary files should be favored, but right now tests put `xray-log.*` in the current directory, which easily lead to conflicts.
> 
> I've added back `rm -rf %t` here. We may discuss this later. If we reach a consensus, I can create a refactor revision for other tests as well, if needed.
It's both the number and the potential size(s) of the logs generated. Because each new invocation will attempt to create a unique filename, it will be a problem if files accumulate upon failures. I see that there's a cleanup of the directory at the beginning of the test which somewhat mitigates this, which I somehow thought was removed.

So I think leaving the files behind is fine given that we'll delete the directory anyway at the beginning.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D52638