This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Fix filesystem_test_helper.h to compile for windows
ClosedPublic

Authored by mstorsjo on Oct 16 2020, 3:31 AM.

Details

Summary

Use .string() instead of .native() in places where we want to combine paths with std::string.

Convert some methods to take a fs::path as parameter instead of std::string, for cases where they are called with paths as parameters (which can't be implicitly converted to std::string if the path's string_type is wstring).

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 16 2020, 3:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2020, 3:31 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mstorsjo requested review of this revision.Oct 16 2020, 3:31 AM
ldionne requested changes to this revision.Oct 16 2020, 5:57 AM
ldionne added inline comments.
libcxx/test/support/filesystem_test_helper.h
27

I'm tempted to move all of those to

#if __has_include(<foo>)
#  include <foo>
#endif

With appropriate comments about what we're getting from those headers, it seems like this is easier to follow than complicated platform-specific #ifs.

32

Instead, I would create a layer of indirection for the utilities we're using here like this:

namespace utils {

#if defined(_WIN32)
  inline std::string getcwd() { /* windows impl */ }
  inline int mkdir(char const*, int) { /* windows impl */ }
  // etc
#else
  inline std::string getcwd() { /* posix impl */ }
  inline int mkdir(char const*, int) { /* posix impl */ }
  // etc
#endif

  inline bool exists(std::string const& path) { /* common impl */ }
} // end namespace utils

It's more verbose, but easier to follow and the code below can make unconditional calls to these functions without being cluttered by #ifs.

279

What are we missing to handle those correctly? I would rather not #ifdef them out if it's just "until we've fixed it". Or, at least, please put up another patch that removes them and mark it as a child of this one.

This revision now requires changes to proceed.Oct 16 2020, 5:57 AM
mstorsjo added inline comments.Oct 16 2020, 6:07 AM
libcxx/test/support/filesystem_test_helper.h
279

As mentioned in the post on libcxx-dev (which I admittedly didn't refer to in this review), symlinks on windows are pretty icky.

You either need to run your process with "elevated privileges", or be running your machine on windows 10 with developer mode enabled and do the syscall for creating the symlink with the flag SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE set. Not sure if that developer mode flag is set on e.g. buildbots and similar. But I guess we could and should try to use that - MS STL uses that at least.

And symbolic links in general on windows has a bit different semantics than on posix, iirc.

So far I've postponed looking into the whole symlinks bit (and ifdeffed out anything touching symlinks in the rest of the tests), but I guess I could at least try to create them here, to get this patch into a more acceptable form.

mstorsjo added inline comments.Oct 16 2020, 2:39 PM
libcxx/test/support/filesystem_test_helper.h
27

There are matching sets of ifdefs further down in this file, around the create_socket function, and within the tests in fs.op.funcs/fs.op.status/status.pass.cpp and fs.op.funcs/fs.op.symlink_status/symlink_status.pass.cpp as well. But maybe with __has_include() here, and a #define HAS_SOCKET which is used elsewhere?

Also, I'm running the tests against MS STL with clang-cl, but is the test suite supposed to work with MSVC itself as well? I presume that doesn't support __has_include().

32

Hmm, I can agree with the clarity, but for these functions, there's no #ifs anywhere in the calling code. Especially the duplication for getcwd feels a bit harsh, when the difference is just between ::getcwd and ::_getcwd.

I thought we could get rid of a few of these (that just add the underscore prefix) by defining _CRT_NONSTDC_NO_WARNINGS though, leaving just mkdir and ftruncate. But that ends up requiring manually linking against oldnames.lib (which normally is linked implicitly, but not in the setup things are done in the libc++ test suite)...

mstorsjo updated this revision to Diff 298765.Oct 16 2020, 2:40 PM
mstorsjo edited the summary of this revision. (Show Details)

Applied some amount of the suggested changes.

mstorsjo updated this revision to Diff 298965.Oct 19 2020, 2:39 AM

Minor further touch-up (taking fs::path instead of std::string in more functions, using the is_dir parameter to create_symlink in one place), split out the socket specific bits to D89673.

ldionne added inline comments.Oct 19 2020, 5:49 AM
libcxx/test/support/filesystem_test_helper.h
27

Any support for MSVC is accidental at this point: I believe there was some amount of work done to make it work a few years back, however that is not tested on a regular basis and nobody is actively maintaining it.

32

I think we should link against it manually if that's what's done automatically normally. That makes us closer to running the compiler the way it's run by default.

59

You could even use using ::mkdir here!

mstorsjo added inline comments.Oct 19 2020, 5:59 AM
libcxx/test/support/filesystem_test_helper.h
32

Ok, will have a poke into doing that. The thing is that normally, when using the compiler normally, we'd be calling clang-cl, which automatically adds both the C runtime and oldnames.lib, but when invoking plain clang++.exe, it sidesteps all of that, but the clang driver only adds the C runtime. I guess the proper fix there would be to make the clang driver add oldnames.lib correspondingly there.

mstorsjo updated this revision to Diff 299029.Oct 19 2020, 6:02 AM

Updated to using ::mkdir; for posix functions that are used as-is without any modification. Using fileno and chdir as is from windows functions is pending autolinking of oldnames.lib in the clang driver (or in the test suite), but updating this patch to current status quo here now.

ldionne accepted this revision.Oct 19 2020, 7:59 AM
This revision is now accepted and ready to land.Oct 19 2020, 7:59 AM
mstorsjo updated this revision to Diff 299062.Oct 19 2020, 8:15 AM

Redirect fewer functions, by enabling -D_CRT_NONSTDC_NO_WARNINGS and linking to oldnames.lib with D89702.

ldionne accepted this revision.Oct 19 2020, 8:33 AM

Even better, thanks.