- User Since
- Jan 26 2015, 9:26 AM (323 w, 5 d)
Thu, Apr 1
Tue, Mar 30
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.
Mon, Mar 29
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.
Thu, Mar 25
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.
Wed, Mar 24
This looks even better.
Tue, Mar 23
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.
Wed, Mar 17
Minidumps should have the registers in the processor context. It seems LLDB knows about TCR_ELn for n > 0. Maybe TCR_EL0 is special?
Tue, Mar 16
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.
Mar 5 2021
Mar 4 2021
From a Windows point of view, this LGTM.
Feb 26 2021
Looks good enough to me. (But I'm also not a libc++ owner.)
Feb 25 2021
Feb 24 2021
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.
Feb 19 2021
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.
Feb 18 2021
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.
Feb 16 2021
I think we'd just want it to sync as much say destruction a file stream or destruction a mapped_file_region.
Feb 12 2021
Jan 29 2021
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.
Jan 22 2021
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.
Jan 21 2021
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.
Jan 13 2021
Thanks for the changes!
Jan 11 2021
Jan 8 2021
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.
Jan 5 2021
Dec 17 2020
This looks correct and straightforward, so LGTM.
Dec 11 2020
Your call on using the CRT. I was under the mistaken impression that libcxx would implement both the standard C++ library as well as the CRT.
Dec 10 2020
LGTM iff this function never has to deal with paths prefixed with \\?\, as in \\?\C:\Windows or \\?\UNC\server\foo.
Dec 8 2020
Dec 7 2020
Oh, are there tests that need to be enabled on Windows?
I'm glad @gkistanova asked for this simplification. The more cautious approach added a lot of probably unnecessary noise.
Dec 4 2020
This looks like it'll work just fine, but I've added some inline questions for readability and robustness.
Dec 3 2020
Dec 2 2020
I'm out of my depth on 128-bit arithmetic and ABIs.
Dec 1 2020
These test look pretty thorough. I think these will be useful in finding portability problems.
I think this patch is a good idea, but I'm having second thoughts about whether this will solve the problem.
LGTM. (I must have slipped on the Action drop-down.)
I look forward to just switching everything to RemoveDirectory.
Nov 30 2020
I prefer the relatively localized disabling of the warning with the #pragmas as you've done as opposed to building the sanitizer builds with different settings.
Nov 20 2020
LGTM. I'll leave it to your discretion on the new_time range check.
Nov 19 2020
This looks good. I'm just confused by one point and have a small suggested edit.
I added two suggested edits for your consideration.
Nov 18 2020
LGTM as long as the run-time calculation of the default value for set_errno is legit.
I'm not a libcxx owner, so I'm not sure what other hoops you should jump through for them, but this looks good to me. Thanks for the improvements!
Nov 17 2020
Excellent. Somehow I missed that AreFileApisANSI was a Win32 function. I must have mistyped it in my searches the other day. This looks good.
Nov 16 2020
The Windows details look correct, though I have a couple questions on the code page conversions.
Nov 12 2020
I have a couple questions to consider inline. If those have already been thought through, then LTGM.
This is pretty close.
Oct 23 2020
This looks pretty good, both the patch and Pavel's insights. I don't see much to comment on that Pavel didn't already catch.
Oct 19 2020
It seems weird that we're implicitly adding defaultlib options without checking if the user specified nodefaultlib. But given that's already the case, I don't see a big problem with also adding oldnames.lib.
Oct 15 2020
I have no reason to suspect those patches in particular. I did a huge git
bisect, which turned up nothing. Older versions that used to work no
longer worked by the time the bisect worked that far back. That makes me
think it might be incomplete dependency checking: Something goes bad, and,
instead of rebuilding it, the bad result is re-used in each subsequent
Oct 14 2020
Oct 13 2020
If I had to guess, my money would be on a deadlock. To unblock, I'd
propose reverting this patch until we can figure it out.
Oct 12 2020
Oct 9 2020
LGTM. Thanks for extending this functionality to Windows!
Oct 8 2020
This looks pretty good.
Oct 7 2020
If I recall correctly, the non-debug builds still had the problem, they just didn't have the assertion that made it obvious.
It's interesting that I haven't encountered some of these errors. There are five _other_ lldb tests that do fail for me. I have a fix in the works for some of those.
Oct 6 2020
Some of this is nitpicky/opinionated, but the race condition is real. We need a reliable way to signal the watcher thread when it's time to exit. Options I see are:
Yes, I also (independently) discovered this problem. I probably have notes somewhere with sources for details about the incompatibility. I believe I also brought it up on the lldb-dev list.
Oct 5 2020
I don't know whether Python and our config tools use the terms consistent, but Win32 is the name of the Windows API used on both 32- and 64-bit processors, Win16 was the API used on 16-bit machines, and Win64 is a made-up term that confuses the issues.
Oct 2 2020
I'm sorry. I haven't had to time to review the entire change yet, but I thought I'd share some early feedback now and take more of a look on Monday.
Sep 29 2020
Sep 25 2020
A big portability problem with the virtual file system (VFS) is that we now have paths that can legitimately be in a hybrid Windows/Posix style. Be aware that some of the "guess-the-style" instances were modified to account for this.
Sep 21 2020
Sep 15 2020
I never realized rc.exe would accept having too few values for a FILEVERSION or PRODUCTVERSION. The documentation implies that you must provide four components--it is the _fixed_ portion of the VERSIONINFO after all. Not that it matters; this looks harmless.