- User Since
- Jan 26 2015, 9:26 AM (299 w, 7 h)
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.
Thu, Oct 15
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
Wed, Oct 14
Tue, Oct 13
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.
Mon, Oct 12
Fri, Oct 9
LGTM. Thanks for extending this functionality to Windows!
Thu, Oct 8
This looks pretty good.
Wed, Oct 7
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.
Tue, Oct 6
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.
Mon, Oct 5
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.
Fri, Oct 2
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.
Tue, Sep 29
Fri, Sep 25
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.
Mon, Sep 21
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.
Sep 2 2020
Aug 31 2020
I like it.
Aug 28 2020
Is there a more specific term than "traditional linker"?
Aug 27 2020
The change looks fine.
Aug 26 2020
Oops, I forgot to "Accept Revision."
Not my area of expertise, but this looks good to me. Before submitting, please make sure you update the commit description since you didn't end up adding any fields to SectionChunk.
I know it's not part of this patch, but Phabricator is showing a clang-tidy lint error on the inclusion of <io.h>. While Windows does have one of these, it's only a compatibility hack, and it doesn't seem to be needed. Can you yank that #include <io.h> line before submitting?
Aug 14 2020
Aug 13 2020
Landed on Tuesday. I must have messed up the Differential Revision: line.
Aug 12 2020
Aug 11 2020
If you make the second loop consistent, I'll approve, despite my concerns about the related problems.
Aug 10 2020
I'm OK with this.
Aug 7 2020
OK, I see. I was looking at the wrong field.
Huh, I thought aggregate initialization was effectively zero-initialization for the remaining fields, so I'm surprised this changes anything. I would have expected the original line, which is idiomatic in Win32 code, to have zero-initialized all of the fields.
Added test to check locating the PDB either in the original build directory or adjacent to the executable.
Aug 4 2020
OK, makes sense. I was hoping the processing could happen as you build up the concatenated string rather than at the end. But now I see the design doesn't really allow for that.
I'll defer to @thakis, but I have a couple suggestions inline.
Aug 3 2020
Well then, since Reid is out, this is good to go! :-)
I have a couple responses, but nothing worth blocking this path for. LGTM, and thanks for doing this!
Jul 30 2020
Thanks. Working on a test.
Jul 29 2020
Jul 28 2020
Fixed typos in comments.
Jul 27 2020
This is probably fine, be I have an inline comment with some questions.
As I noted on April 30, I'm OK with the Windows portions of this. I didn't explicitly "Accept" because I didn't want to pre-empt the concerns of the other reviewers.
Jul 23 2020
This looks fine to me. Is there a lit expert/owner who should also review?
I'm okay with the idea. I haven't used GnuWin32 in long while since I already have Git's usr/bin in my PATH. See detailed comments inline.
Jul 22 2020
Jul 21 2020
The naming improvements really help. And it's nice to see the symbol tag set correctly (rather than generically only to be overridden). Nice work.
It seems @sepavloff has already reverted, so I'll assume they'll fix it.
Just a few days ago, I landed another single file /bigobj to get some LLDB tests running: https://reviews.llvm.org/rG14dde438d69c81ab4651157a94d32e5555e804ff
Jul 17 2020
LGTM. That was a nice catch on the other review.
Jul 16 2020
Thanks @mstorsjo for clarifying for me.
Jul 15 2020
Yes, getting rid of this hack looks like a good idea. If it was actually necessary, there should have been a test on it, and the comments should have been clearer.
Thanks! I know it was more work, but I think unifying the command line generation to work the same in both cases will avoid future surprises.
Jul 14 2020
Sorry, I thought I had LGTM'ed yesterday. I guess the Submit didn't take.
I'm jumping in since rnk is on leave and (I believe) zturner is less focused on these issues than he used to be.
Jul 13 2020
I don't have enough background here, so I'll defer to the decision of other reviewers whether this makes sense. @ruiu ?
Jul 1 2020
Jun 30 2020
Jun 25 2020
rnk is on leave, so I'll jump in.
Jun 24 2020
Very close now. It looks like the change I requested led to a bug. Once that's corrected, this'll look great.
Jun 22 2020
Jun 16 2020
This looks right, but I'd like to see a test.
Jun 15 2020
That commit (which was reviewed here https://reviews.llvm.org/D48051) suggests this wasn't about the occasional temp file not getting deleted. It was to work around surprising behavior on Windows when a temp file is mapped into memory, which, as I recall, was the underlying cause in what appeared to be cache corruption for LTO.
Thanks. This seems well-contained and the test changes look good.
Jun 12 2020
Sorry about my review latency.
Jun 3 2020
I'm just responding to the questions @labath raised without really looking at the details of the code.
Please make sure all of the clang VFS tests still pass before submitted this. VFS paths are inherently problematic with llvm::sys::path because they can legitimately be in a hybrid of Windows and Posix styles. Guessing the style from the first separator you see can be misleading, but I don't know whether that's ever wrong in this path.
Jun 2 2020
Does the full path to the tool end up only in the object file, or does this make it all the way to the final executable or library? Does embedding full paths affect distributed builds or build reproducibility?
Jun 1 2020
Yowza. Mingw still links against MSVCRT?!
May 27 2020
No problem. I'll land it this afternoon (Pacific time). Thanks!
Ah, I guess the diff was based off the old head rather than the rebased one. Phabricator highlighted all that code as part of this patch.