LGTM

I'm trying to understand why I've never seen the problem that this patch is trying to address.

LGTM.

Thanks for the fixes. LGTM.

Minor suggestions, but otherwise LGTM. I assume you've coordinated with @aganea to ensure you aren't duplicating effort.

I'm mildly surprised the make_absolute call doesn't remove unnecessary dotted paths. I guess that's related to the weird schism between sys::fs and sys::path.

This is a larger change than I anticipated. I was surprised by the need to have the pointer be thread local (and then ensure that the thread local pointers for new threads are set up, too). If I'm understanding correctly, the idea is that a single program could have n threads doing n links at the same time. Is that right? I'm not arguing against that. I'm just trying to make sure I understand why I was surprised.

LGTM.

I also thank you for working to improve this documentation. I mostly agree the other reviewer. I've only added a couple inline comments to raise questions. I wouldn't hold this back for either of those.

Thanks for making the change more local.

In D107557#2929544, @mstorsjo wrote:

Hmm, that's a good question. Most of the options that are listed aren't even hooked up in llvm-cvtres (and the tool it impersonates, MS cvtres.exe, also just lists the options by name without any help text in its help printing).

I guess we could add a help text for the options that we have implemented at least, and then omit the other ones. Or do @thakis or @amccarth have a strong opinion on it?

I was a little thrown by llvm-windres versus llvm-rc. I hadn't heard of llvm-windres before, and my web search came up with zero hits (literally!). But now I understand.

I see. I didn't realize llvm-rc used an actual separate preprocessor step. (RC has a crude one built-in that doesn't quite match any of the standards.)

Cool. I'd never seen anyone use strings that don't look like a C or C++ identifier as a resource identifier, but it should work given that FindResource just uses the string as an opaque key.

LGTM

Two small requests, then LGTM.

In D104987#2856224, @mstorsjo wrote:

In D104987#2855606, @amccarth wrote:

tl;dr: I understand the desire to avoid even tiny synchronization costs when trying to read a high-precision timer, but I'm uncomfortable with the data race. I suggest initializing the static function pointer under the locking provided by the compiler.

The simplest option would be to initialize it where it's defined. I would move the code that finds the address to a separate function, say GetGetSysTimeProcAddress, and then simplify this code to:

static GetSysTime_t getSysTime_p = GetGetSysTimeProcAddress();
getSysTime_p(&ft);

Oh, that's great, I had no idea that this actually ends up synchronized. That's very neat.

tl;dr: I understand the desire to avoid even tiny synchronization costs when trying to read a high-precision timer, but I'm uncomfortable with the data race. I suggest initializing the static function pointer under the locking provided by the compiler.

LGTM. Thanks.

The idea looks fine. Just a couple inline questions.

In D88666#2825557, @stella.stamenova wrote:

One thing we've run into in the past is that running some of these tests on a drive other than the OS drive can cause weird failure. I am not sure if that may be the case here as well.

This patch was reverted a while back because a couple DirectoryWatcher tests routinely timed out on build bots even though they work when run in isolation.

Thanks for the changes. This LGTM now.

Nice catch Reid.

LGTM

In D87732#2298861, @rnk wrote:

In D87732#2296149, @amccarth wrote:

In D87732#2296076, @rnk wrote:

Separately, I would like LLVM to move in the direction of standardizing on forward slashes in internal representations and data structures.

If we move that direction, we'll have to re-think VFS again. I'll also lobby hard to make sure that we show the native representation in compiler output, like diagnostic messages.

Yes, we might have to adjust VFS usage, but most of the important users are in-tree, so this is doable, not something unfixable,

I'm fine focusing on this part to get the bug fixed. We can consider looking at accepting upper and lower case letters in the future.

LGTM.

Yeah, this is good. My remaining comments are all speculations about how to improve this further in the future, but they aren't directly applicable to the goal here.

This sounds like an appropriate place to apply Postel's Law: Be conservative in what you do, be liberal in what you accept from others. I'm be happy to always generate uppercase hex digits when formatting a GUID to registry format, but maybe we should also consider accepting upper or lowercase when reading a formatted GUID.

In D102684#2775573, @mstorsjo wrote:

While exiting from a non-main thread is tricky, things also do hang when exiting from the main thread, when bypassing constructors. The fatal issue there is that we have a number of threads still running, that can keep various locks. When exiting, all other threads are stopped where they are, and we run the destructors for DLLs.

In D102684#2773342, @andrewng wrote:

I looked into this in some detail back in the day and am mainly responsible for how this thread pool currently functions. I primarily focused on static linking with the MSVC static runtime, as that's what we use downstream and were seeing issues with, but also included MinGW, GNU and libc++. To be honest, I can't remember if I tried LLVM_LINK_LLVM_DYLIB, there are just too many possible configurations! The various compilers/runtimes have changed since I last looked at this code, so it's not surprising that there may be issues depending on your configuration.

From past experience, I think it has always been "dangerous" on Windows to exit without waiting/terminating any running threads. If this can't be done, then a "hard" exit, e.g. via ExitProcess(), is usually the only reliable way to finish....

Cool! This is getting much simpler. My remaining comments are mostly musings and you can tell me to buzz off.

Sorry, forgot to Request Changes.

At some point, the duplicate handle must be closed. I don't see that happening. I've added an inline comment where I think it should be done.

I'll have to dig into my archived email tomorrow.

When this gets further along, maybe we can get LLDB to use it instead of EditLine.

LGTM. Dead code should go. If somebody wants to own a Windows port of EditLine, they can re-instate it and put some tests on it.

It looks like this EditLine port was added before I joined this project. @zturner may have worked on it, but I don't know/remember.

In D101479#2748475, @mstorsjo wrote:

In D101479#2748189, @amccarth wrote:

In D101479#2733354, @mstorsjo wrote:

Not sure if we want the desicion between static and shared libc++ be coupled with /MT and /MD, as one can quite plausibly want to use e.g. a static libc++ with /MD.

I don't understand this. When would someone want to use /MD and not get the DLL version of the run-time libraries?

Whether one wants to link against the CRT statically or dynamically, and libc++ statically or dynamically, are two entirely separate things. I would e.g. expect that Chrome is built with a statically linked libc++ but linked against the dynamic CRT.

In D101479#2733354, @mstorsjo wrote:

Not sure if we want the desicion between static and shared libc++ be coupled with /MT and /MD, as one can quite plausibly want to use e.g. a static libc++ with /MD.

LGTM. Thanks!

A drive-by look.

There's a lot going on here, but I don't see anything wrong. Thanks for the completeness of the tests and the comments, as that helps a lot in understanding what's going on here.

Wow. Excellent. Thank you!

I'll merge it for you.

Ah, I get it. Thanks for the clarification.

I'm not quite clear on the motivation. It's hard for me to imagine the resource compiler being useful if you cannot use the C preprocessor. I guess for simple tests you don't need it, is that worth adding an option?

Perhaps somebody confused <utility> std::move with <algorithm> std::move.

Personally, I'm not a fan of boolean function parameters because of the inline comments necessary to make the call site understandable. But it appears to be consistent with LLVM Coding Standards and other APIs, so this looks right to me.

Sorry, I think I've lost track of some context while I was on vacation. I don't understand why several of the tests are now unsupported on Windows. Some of those seem like important tests.

In D99580#2660040, @kamleshbhalui wrote:

In D99580#2659858, @amccarth wrote:

It looks like the code change is for everyone, but the new test is specific to mingw.

For Linux like platform it does not create problem because input file path is already in POSIX style, so having a test case will not test the change because even without the change it will pass.

The previous discussions (that I participated in) were centered around the redirecting virtual filesystem, which creates paths in hybrid style and there is no "correct" way to make those native without taking breaking changes and making it less useful for writing platform-agnostic tests. But it's not clear that's relevant here.

I like the composability enabled by making OF_Text and OF_CRLF independent. I wonder, though, if there's a chokepoint where we could/should assert if the caller uses OF_CRLF without OF_Text, which would be suspicious.

LGTM.

Adding everything in LIB to PATH is not a normal thing to do in Windows, so I would support this patch to remove that hack. If we knew what the original problem was, we could probably find a better solution.

Just some drive-by comments.

When changing an IO stream's mode from binary to text breaks only Windows, it's likely due to the fact that Windows uses CR+LF as the newline indicator. Not only are the CRs sometimes unexpected, they can also throw off code that tries to seek to a computed file position.

This looks even better.

LGTM.

LGTM.

I like the solution. Has anyone actually tried it on Windows 7? I ask because some of the observations noted in second hand descriptions of the problem didn't fully add up. I think we understand the root problem, but I'm not 100% certain.

Minidumps should have the registers in the processor context. It seems LLDB knows about TCR_ELn for n > 0. Maybe TCR_EL0 is special?

This workaround seems consistent with other overrides of FixCodeAddress, my only concern is about the assumption of the number of bits that need to be preserved/stripped.

In D97538#2597729, @mstorsjo wrote:

Tagging @curdeius and @amccarth for input and ideas
If a process running as root can iterate over a directory even after chmod 000, I don't think changing the owner would help. Dunno about windows - on the API level we operate so far, permissions only consist of a boolean readonly flag, not much more. On some other level, windows filesystems have full ACLs for controlling access though.

From a Windows point of view, this LGTM.

Looks good enough to me. (But I'm also not a libc++ owner.)

LGTM.

The option is used in the wild, but not as widely as I first believed. We've already fixed a couple projects that were using it. I think the Release Note is the right solution. Thanks for doing that. I withdraw my suggestion to allow and ignore.

LGTM.

I'm in no position to evaluate the functionality of lldb on Windows. I've spent the last few months trying to understand why 900+ tests spontaneously started failing on Windows.

Fair enough. I'm continuing the whack-a-mole game for the Chromium third-parties.

The removal of -Wreturn-std-move-in-c++11 breaks a few projects. These projects are specifying the option, which then triggers an unknown-option warning and thus -Wx halts the build.

I think we'd just want it to sync as much say destruction a file stream or destruction a mapped_file_region.

In D95494#2534572, @rnk wrote:

@amccarth, can you confirm that these are the right APIs to use here? Do we need both FlushViewOfFile and FlushFileBuffers?

LGTM

The changes to provide realpath on Windows looks right. Since it's relying on a system call, it would be difficult to create tests for the \\?\ and UNC cases. I'm assuming the more mainstream cases will be tested by existing tests. So LGTM.

BTW, I hope I didn't come across as overly negative in my previous
response. I'd love to see the situation improved. I just don't know what
that would look like.

I didn't design VFS. I just fixed a bunch of portability bugs that
prevented us from running most of the VFS tests on Windows. If I were
designing it, I (hope) I wouldn't have done it this way. But the horse is
already out of the barn.

Thanks for the changes!

This looks good to me, in that I believe it will correctly parse typical native Windows path names. Only tests will tell, but I understand that there are many patches necessary to enable the tests.

LGTM, but please take the suggested edit to update a documentation URL.

LGTM.

LGTM.

In D91852#2460100, @mstorsjo wrote:

Ping @amccarth, do you want to follow up on the discussion before I land this?

This looks correct and straightforward, so LGTM.

LGTM.