- User Since
- Jan 26 2015, 9:26 AM (289 w, 2 d)
If you make the second loop consistent, I'll approve, despite my concerns about the related problems.
Mon, Aug 10
I'm OK with this.
Fri, Aug 7
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.
Tue, Aug 4
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.
Mon, Aug 3
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!
Thu, Jul 30
Thanks. Working on a test.
Wed, Jul 29
Tue, Jul 28
Fixed typos in comments.
Mon, Jul 27
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.
Thu, Jul 23
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.
Wed, Jul 22
Tue, Jul 21
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
Fri, Jul 17
LGTM. That was a nice catch on the other review.
Thu, Jul 16
Thanks @mstorsjo for clarifying for me.
Wed, Jul 15
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.
Tue, Jul 14
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.
Mon, Jul 13
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.
May 26 2020
Thanks for extending the tests. Those will certainly make your fix more robust against regressions.
May 20 2020
May 19 2020
I read back through some of the history, but I still don't feel like I have all the context. That said, this change looks simple and straightforward to me.
I'm happy to see this through.
I think the fix looks good, but I'd like to see a bit more in the test.
May 15 2020
As long as the tests are working, I'm OK with this. But please hold off until others chime in. I know @thakis and others have been dealing with making cross-platform builds work better with relative paths, so they may know about more constraints on file names in debug info.
I'll re-iterate my opinion that the casts currently required to bypass the warnings are useful (e.g., if you ever want to port the code to 64-bit). There are lots of places where the Windows API essentially required uncomfortable conversions, and we can't paper over all of them. The way to get it right in all those other places is to be very aware of the underlying types. Hiding an uncomfortable detail here and there seems like a disservice.
May 13 2020
Addressed feedback, specifically:
- Distinction is now on CodeView generation rather than -fms-compatibility.
- Tests two --std= levels plus the default one.
Made the requested changes after an in-person conversation to clear up my earlier confusion.
May 12 2020
Looks good. Thanks for the improvements.
Windows doesn't properly support pass plugins (as a shared library can't have undefined references, which pass plugins assume, being loaded into a host process that contains provides them)
May 7 2020
May 6 2020
Updated an existing test. Thanks for the pointer; I had gotten myself completely confused about the organization of the test directories.
Updated test to validate the change.
I'm less worried about the details of style (like camel casing), since I have the impression things are changing. In general, stick to prevalent style of the file when it's discernable. My issue with PrefixStyle was that the name could be confusing.
May 5 2020
I think this is pretty close now.
LGTM, but I think we should hear again from rnk before landing this.
May 4 2020
May 1 2020
OK as is, but please consider re-using llvm::sys::path to distinguish separators.
Apr 30 2020
Thanks for including me in this discussion.
Apologies for the delay. I thought I had already responded to this one.
Thanks for the timeout fix on Windows. I'm happy now, but I'm not qualified to approve other parts of this patch.
Apologies for the drive-by questions.
The llvm::sys::Path functions are designed to deal with paths that are either Posix-style or (a subset of) Windows styles. We also have the virtual file system (VFS), specifically the redirecting file system, which can truly be hybrids, mixing Posix with Windows styles in a single path.
Apr 29 2020
If the goal is to synchronize writes to a highly contended log file, would it be better (and feasible) to have the individual threads/processes write timestamped output to separate streams that can be merged after-the-fact?
This seems to make sense.
Apr 27 2020
Apr 20 2020
Thanks. I appreciate the separation of concerns with regard to finding and loading of the PDB.
Apr 16 2020