[test] Add lit helper for windows paths

[keith]

Summary

This adds 2 new lit helpers %{fsroot} and %{fssep}, these allow
writing tests that correctly handle slashes on Windows. In the case of
tests like clang/test/CodeGen/debug-prefix-map.c, these are unable to
correctly test behavior on both platforms, unless they fork and add OS
requirements, because the relevant logic hits host specific codepaths
like checking if paths are absolute.

[rmaz]

clang/test/lit.cfg.py

60 ↗

(On Diff #378333)

c:\?

[keith]

Fix tests

[keith]

clang/test/lit.cfg.py

60 ↗

(On Diff #378333)

Do you mean lowercase or a single slash? I see ~2x the number of uppercase vs lowercase in this codebase now, and the second is just because of escaping

[rmaz]

clang/test/lit.cfg.py

60 ↗

(On Diff #378333)

I meant the single slash, yes, wasn't sure if it needed to be escaped or not

[keith]

Allow empty leading on windows

This behavior is currently broken, the directory should never be / or C:\ but the logic is broken and only handles /

[mstorsjo]

Wouldn't this one also be solved pretty much the same, but differently, by changing if (llvm::sys::path::is_absolute(RemappedFile)) { into is_absolute_gnu? Since we're remapping debug paths, it's plausible that the target path can be a different style (when cross compiling, where debug prefix remapping is kinda important), and then it's probably good to use a more lax definition of whether a path is absolute. (Alternatively, we maybe should try to detect the kind of path used and use the appropriate style as argument to is_absolute, but I don't think that necessarily helps here.)

(Overall I think it might be good to have the native separators and root available for tests though, even if this particular test might make sense both as-is and with native separators.)

[keith]

In D111457#3066508, @mstorsjo wrote:

Wouldn't this one also be solved pretty much the same, but differently, by changing if (llvm::sys::path::is_absolute(RemappedFile)) { into is_absolute_gnu?

I think it could but also users could still remap to native window's paths, so likely we'd want this test as well I think either way?

Since we're remapping debug paths, it's plausible that the target path can be a different style (when cross compiling, where debug prefix remapping is kinda important), and then it's probably good to use a more lax definition of whether a path is absolute. (Alternatively, we maybe should try to detect the kind of path used and use the appropriate style as argument to is_absolute, but I don't think that necessarily helps here.)

Good point here, I could definitely see wanting to support the entire matrix of host windows paths vs not, and target windows paths vs not, but I think that would require significantly more changes for other places that call llvm::sys::path::* APIs and also use the default native argument (I'm not sure how difficult this would be, but it would require a bit of auditing)

[mstorsjo]

In D111457#3073726, @keith wrote:

In D111457#3066508, @mstorsjo wrote:

Wouldn't this one also be solved pretty much the same, but differently, by changing if (llvm::sys::path::is_absolute(RemappedFile)) { into is_absolute_gnu?

I think it could but also users could still remap to native window's paths, so likely we'd want this test as well I think either way?

I guess this might be a good addition as a new test, yeah, but I think it would be good to keep this test as is too, and change the code to use is_absolute_gnu (and fix up the test reference here to expect an empty directory in the output).

Since we're remapping debug paths, it's plausible that the target path can be a different style (when cross compiling, where debug prefix remapping is kinda important), and then it's probably good to use a more lax definition of whether a path is absolute. (Alternatively, we maybe should try to detect the kind of path used and use the appropriate style as argument to is_absolute, but I don't think that necessarily helps here.)

Good point here, I could definitely see wanting to support the entire matrix of host windows paths vs not, and target windows paths vs not, but I think that would require significantly more changes for other places that call llvm::sys::path::* APIs and also use the default native argument (I'm not sure how difficult this would be, but it would require a bit of auditing)

I would expect that to mostly work so far, except for corner cases like these. Auditing probably doesn't hurt if one wants to spend the effort, otherwise I'd just expect it to work and try it out and see if one runs into any issues somewhere, if someone has such a usecase.

[keith]

In D111457#3082398, @mstorsjo wrote:

In D111457#3073726, @keith wrote:

In D111457#3066508, @mstorsjo wrote:

Wouldn't this one also be solved pretty much the same, but differently, by changing if (llvm::sys::path::is_absolute(RemappedFile)) { into is_absolute_gnu?

I think it could but also users could still remap to native window's paths, so likely we'd want this test as well I think either way?

I guess this might be a good addition as a new test, yeah, but I think it would be good to keep this test as is too, and change the code to use is_absolute_gnu (and fix up the test reference here to expect an empty directory in the output).

Since we're remapping debug paths, it's plausible that the target path can be a different style (when cross compiling, where debug prefix remapping is kinda important), and then it's probably good to use a more lax definition of whether a path is absolute. (Alternatively, we maybe should try to detect the kind of path used and use the appropriate style as argument to is_absolute, but I don't think that necessarily helps here.)

Good point here, I could definitely see wanting to support the entire matrix of host windows paths vs not, and target windows paths vs not, but I think that would require significantly more changes for other places that call llvm::sys::path::* APIs and also use the default native argument (I'm not sure how difficult this would be, but it would require a bit of auditing)

I would expect that to mostly work so far, except for corner cases like these. Auditing probably doesn't hurt if one wants to spend the effort, otherwise I'd just expect it to work and try it out and see if one runs into any issues somewhere, if someone has such a usecase.

Yep this makes sense. I would rather not scope that into this if that's ok. Since this test was already invalid and broken (actual fix in https://reviews.llvm.org/D111579) I would rather land these and then take that on to unblock my original use case (https://reviews.llvm.org/D111587) if I can find some more time

[mstorsjo]

In D111457#3085273, @keith wrote:

Yep this makes sense. I would rather not scope that into this if that's ok. Since this test was already invalid and broken (actual fix in https://reviews.llvm.org/D111579) I would rather land these and then take that on to unblock my original use case (https://reviews.llvm.org/D111587) if I can find some more time

Ok, fair enough - I guess that sounds reasonable, so that'd be a +1 from me to this patch. Ideally it'd be good to find someone more authoritative around clang and these areas than me, but I guess reviewer attention is scarce and hard to come by. @rnk - does this seem sensible to you?

Let's give it a little more time for someone else to comment on it, otherwise I guess I could approve it...

Then for the future cleanup, one might be able to stop using the native substitutions, by keeping one test entirely using possibly-foreign posix style forward slashes, and one using the windows style paths (with that test possibly limited to only executed when on windows, if necessary - but being able to map things to a windows path while cross compiling is good too).

[rnk]

These seem like generic substitutions that would be useful to anyone writing lit tests. We already have %{pathsep}:
https://llvm.org/docs/CommandGuide/lit.html#substitutions

These should live there as well, and be documented similarly, I think.

[compnerd]

clang/test/lit.cfg.py

60 ↗

(On Diff #378725)

This isn't really a separator, this is the root itself. I think that it makes sense to name it accordingly. Please do not hard code this to C:\. There is no guarantee that the root is C:\.

[keith]

clang/test/lit.cfg.py

60 ↗

(On Diff #378725)

Yea I didn't like the original naming here either, any suggestions? I avoid path in these because of the existing pathsep, maybe fsroot and fssep?

What are you suggesting in place of hardcoding C:\? I don't think we could detect what folks are using in the tests, so it does seem like it would be up for individual tests to do this however it makes sense. It appears that there are a few hundred uses of this in tests today, does that break in general if you're using another drive?

[keith]

Move config to llvm lit config, add docs

[rnk]

llvm/docs/TestingGuide.rst

577

Stray backticks in "Expands"?

llvm/utils/lit/lit/TestRunner.py

1127

It is pretty common to run the LLVM test suite on secondary drives, especially on buildbot VMs, to separate build disk usage from the root partition. Can we have two separators, ${srcroot} and ${tmproot} that expand to the drive letter of the source and temp path base? That seems like it would be general and implementable.

[probinson]

llvm/utils/lit/lit/TestRunner.py

1127

+1. I don't even do builds on C: drive, on one of my machines.

[keith]

Use the actual drive on windows, split out into fssrcroot and fstmproot

[keith]

llvm/utils/lit/lit/TestRunner.py

1127

Updated, I kept the fs prefixing thinking that srcroot and tmproot might be too confusable as an alternative to %t and friends, let me know what you think!

[keith]

For reference here's an example of a test that uses this https://reviews.llvm.org/D111579

[rnk]

lgtm

[keith]

@compnerd can you re-review here? I think I covered your feedback, let me know!

[compnerd]

Seems reasonable, though I'm not a fan of the variable names - they seem a bit difficult to read due to no separation (e.g., %fs-src-root or %fs_src_root vs %fssrcroot)

[keith]

Update substitutions to use dashes

llvm/docs/CommandGuide/lit.rst

 
  ======================= ==============
   Macro                   Substitution
  ======================= ==============
  %s                      source path (path to the file currently being run)
  %S                      source dir (directory of the file currently being run)
  %p                      same as %S
  %{pathsep}              path separator
+ %{fs-src-root}          root component of file system paths pointing to the LLVM checkout
+ %{fs-tmp-root}          root component of file system paths pointing to the test's temporary directory
+ %{fs-sep}               file system path separator
  %t                      temporary file name unique to the test
  %basename_t             The last path component of %t but without the ``.tmp`` extension
  %T                      parent directory of %t (not unique, deprecated, do not use)
  %%                      %
  %/s                     %s but ``\`` is replaced by ``/``
  %/S                     %S but ``\`` is replaced by ``/``
  %/p                     %p but ``\`` is replaced by ``/``
  %/t                     %t but ``\`` is replaced by ``/``

llvm/docs/TestingGuide.rst

``%T``
    Use ``rm -rf %t && mkdir %t`` instead if a temporary directory is necessary.
 
    Example: ``/home/user/llvm.build/test/MC/ELF/Output``
 
 ``%{pathsep}``
 
    Expands to the path separator, i.e. ``:`` (or ``;`` on Windows).
 
+``${fs-src-root}``
+   Expands to the root component of file system paths for the source directory,
+   i.e. ``/`` on Unix systems or ``C:\`` (or another drive) on Windows.
+
+``${fs-tmp-root}``
+   Expands to the root component of file system paths for the test's temporary
+   directory, i.e. ``/`` on Unix systems or ``C:\`` (or another drive) on
+   Windows.
+
+``${fs-sep}``
+   Expands to the file system separator, i.e. ``/`` or ``\`` on Windows.
+
 ``%/s, %/S, %/t, %/T:``
 
   Act like the corresponding substitution above but replace any ``\``
   character with a ``/``. This is useful to normalize path separators.
 
    Example: ``%s:  C:\Desktop Files/foo_test.s.tmp``
 
    Example: ``%/s: C:/Desktop Files/foo_test.s.tmp``

llvm/utils/lit/lit/TestRunner.py

 from __future__ import absolute_import
 import errno
 import io
 import itertools
 import getopt
 import os, signal, subprocess, sys
 import re
 import stat
+import pathlib
 import platform
 import shutil
 import tempfile
 import threading
 
 import io
 try:
     from StringIO import StringIO
def getDefaultSubstitutions(test, tmpDir, tmpBase, normalize_slashes=False):
     substitutions.extend([('%s', sourcepath),
                           ('%S', sourcedir),
                           ('%p', sourcedir),
                           ('%{pathsep}', os.pathsep),
                           ('%t', tmpName),
                           ('%basename_t', baseName),
                           ('%T', tmpDir)])
 
+    substitutions.extend([
+        ('%{fs-src-root}', pathlib.Path(sourcedir).anchor),
+        ('%{fs-tmp-root}', pathlib.Path(tmpBase).anchor),
+        ('%{fs-sep}', os.path.sep),
+    ])
+
     # "%/[STpst]" should be normalized.
     substitutions.extend([
             ('%/s', sourcepath.replace('\\', '/')),
             ('%/S', sourcedir.replace('\\', '/')),
             ('%/p', sourcedir.replace('\\', '/')),
             ('%/t', tmpBase.replace('\\', '/') + '.tmp'),
             ('%/T', tmpDir.replace('\\', '/')),
             ])