This is an archive of the discontinued LLVM Phabricator instance.

[MSVC] Disable <fstream> usage of <filesystem>
AbandonedPublic

Authored by rnk on Jan 15 2021, 1:58 PM.

Details

Reviewers
mstorsjo
ldionne
Group Reviewers
Restricted Project
Summary

ninja cxx now succeeds for me locally.

This fixes link errors when producing a DLL for libc++ with clang-cl.
When building a DLL, most of <filesystem> is annotated as dllexport.
When a class is marked dllexport, all of its inline functions are
emitted into the object file. Those inline functions reference
out-of-line functions that are not available because the filesystem cpp
files are not built on Windows because they haven't been ported yet.

Diff Detail

Event Timeline

rnk requested review of this revision.Jan 15 2021, 1:58 PM
rnk created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 1:58 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mstorsjo added inline comments.Jan 15 2021, 2:08 PM
libcxx/include/fstream
190

I'm not entirely fond of hardcoding the condition like this - would it be better to emit something to __config (via __config_site) about whether filesystem is disabled?

I guess hardcoding msvc could be argued for, because the dllexport behaviour is kinda msvc specific (this problem doesn't manifest in mingw builds, that build successfully in dll mode).

Another reason for not hardcoding it for msvc, is that while filesystem is disabled for now, it hopefully won't be for long, and it'd be nice not to have an extra condition to keep track of to update in sync.

rnk added inline comments.Jan 15 2021, 3:05 PM
libcxx/include/fstream
190

I think if the project wants to support the LIBCXX_ENABLE_FILESYSTEM option over the long run, then it probably makes sense to feed that into __config_site and use that to control this include and the availability of these overloads.

However, if we view this as a temporary portability work around, then I think it makes sense to limit the scope of the workaround as much as possible.

mstorsjo added inline comments.Jan 15 2021, 11:03 PM
libcxx/include/fstream
190

Fair enough. Wrt enabling filesystem in msvc configurations, the int128 builtin issue (see D91139) remains. (For mingw configurations, the patchset in review is everything that is needed.)

@rnk Can you please take a look at https://reviews.llvm.org/D94921 and tell me if that solves your issues? That would be my preferred way forward.

rnk added a comment.Jan 19 2021, 9:56 AM

@rnk Can you please take a look at https://reviews.llvm.org/D94921 and tell me if that solves your issues? That would be my preferred way forward.

Yep, that should work for us. I haven't patched in and tested yet, but I'll send follow up patches if it doesn't work on the first try.

rnk abandoned this revision.Jan 19 2021, 9:57 AM