[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.

As long as it's a local drive, that _shouldn't_ be a problem.  I always run 
tests on a different drive than the OS system drive.

If it's a network drive, then, yeah, that would likely be a problem.  If I 
recall correctly, ReadDirectoryChangesW has substantial limitations when 
pointed at a remote drive.  The implementation should probably check that and 
signal an "unsupported" error.

Also note that Stella's sample log looks slightly different than the failures 
we were reproducing.  It's almost as if the initial scan never finished.  I 
haven't looked at that code, but I wonder if the file iteration is stuck in 
some kind of loop due to links or mount points or something.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88666/new/

https://reviews.llvm.org/D88666

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.

I believe the problem is that, on a machine busy doing a build, the startup of 
the watcher thread is delayed (either because the scheduler algorithm or the 
thread pool policy).  Thus the "initial scan" on the test thread can complete 
_before_ the watcher thread has called ReadDirectoryChangesW.  This leaves a 
window of time where the test can change a file that the watcher thread will 
miss.

To test this hypothesis, I took this patch and created one more event called 
`WatcherThreadReady`.  I have the watcher thread set this event after 
successfully calling ReadDirectoryChangesW.  In the constructor, I changed:

  if (WaitForInitialSync)
InitialScan();

to

  if (WaitForInitialSync) {
::WaitForSingleObject(WatcherThreadReady, 1);
InitialScan();
  }

This is crude, but it seems to be effective.  The tests pass reliably for me 
when my machine is fully loaded.  I didn't use an INFINITE timeout because it 
seems possibly missing a file change is less bad than hanging forever.  I 
didn't even bother to check the result from the wait because there's nothing 
sane to do besides "best effort" if something goes wrong.  I used a Windows 
event because those are most familiar to me and it's Windows-only code.   But 
it certainly could be done with other type of synchronization object.

There may be more elegant ways to solve this, but something like this directly 
addresses the root cause with fairly minimal changes.

I wonder if the Linux and Mac implementations might suffer from a similar 
window but the bug is rare because of differences in thread scheduler.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88666/new/

https://reviews.llvm.org/D88666

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102736: Fix tmp files being left on Windows builds.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

Nice catch Reid.




Comment at: clang/lib/Frontend/CompilerInstance.cpp:857-858
+  OS.reset(new llvm::raw_fd_ostream(Temp->FD, /*shouldClose=*/false,
+Binary ? llvm::sys::fs::OF_None
+   : llvm::sys::fs::OF_Text));
+  OSFile = std::string(TempPath.str());

rnk wrote:
> I think the bug is here: the third parameter is `bool unbuffered`, not file 
> flags, so we are opening the file for unbuffered writing, and that is really 
> slow.
Yowza!  In addition to a fix, we need some memes.

* Why do we even have that lever?
* This constructor has too many overloads.  Please remove three.
* [Facepalm] `bool` parameters.  [Double facepalm]  Negative `bool` parameter 
names that default to `false`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102736/new/

https://reviews.llvm.org/D102736

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103806: [SystemZ][z/OS] Pass OpenFlags when creating tmp files

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103806/new/

https://reviews.llvm.org/D103806

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102736: Fix tmp files being left on Windows builds.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102736/new/

https://reviews.llvm.org/D102736

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102736: Fix tmp files being left on Windows builds.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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

But I'd like to see @aganea's questions addressed.  It does seem redundant to 
have to keep the file name when we already have an object that represents the 
file.




Comment at: llvm/lib/Support/Path.cpp:1237
   RenameEC = copy_file(TmpName, Name);
   setDeleteDisposition(H, true);
 }

I'm curious if this path has ever been exercised.

I see that rename_handle is implemented with MoveFileExW on Windows.  
MoveFileExW docs say it will fail if you're trying to move a _directory_ to 
another device, but that it can do copy+delete to move a _file_ to another 
device.  (That might require adding MOVEFILE_COPY_ALLOWED to the flags passed 
in the MoveFileExW in lib\Support\Windows\Path.inc near line 485.)

Anyway, it just seems like we're re-implementing functionality the OS calls can 
already do for us.

https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefileexw



Comment at: llvm/lib/Support/Path.cpp:1243
   if (RenameEC)
 setDeleteDisposition(H, true);
 #else

If I understand correctly, we're using the delete disposition flag to get the 
OS to clean up the file in case we crash.  But does that prevent us from just 
deleting it outright once we know it's safe to do so?

In other words if we could just delete the file here, then the divergence 
between Win32 and others could possibly be reduced.



Comment at: llvm/lib/Support/Path.cpp:1253
   }
   sys::DontRemoveFileOnSignal(TmpName);
 #endif

Possibly stupid idea:  What if RemoveFileOnSignal and DontRemoveFileOnSignal 
did the Win32-specific delete disposition flag twiddling?  With that 
encapsulated, and assuming we can still explicitly delete a Windows file that's 
already marked for deletion, it seems like all of the special handling here 
could go away.

Then again, the ...OnSignal things are in sys rather than sys::fs, so maybe 
it's not such a good idea.

I'm not trying to make more work for Amy.  The immediate goal probably isn't 
well served by this level of overthinking.  But it would be nice to see all 
this get simpler in the long term.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102736/new/

https://reviews.llvm.org/D102736

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102736: Fix tmp files being left on Windows builds.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth requested changes to this revision.
amccarth added a comment.
This revision now requires changes to proceed.

Sorry, forgot to Request Changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102736/new/

https://reviews.llvm.org/D102736

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102736: Fix tmp files being left on Windows builds.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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 find it weird that duplicating the handle seems necessary.)

At a high level, it seems a shame that `llvm::support::fs` doesn't have 
create-temporary-file and keep-temporary-file operations to hide all this 
detail from the frontend.




Comment at: clang/lib/Frontend/CompilerInstance.cpp:721
+llvm::sys::fs::UnmarkFileForDeletion(OF.Handle);
+
 // If '-working-directory' was passed, the output filename should be

IIUC, OF.Handle is the duplicate handle, and we're done with it at this point.  
It should be closed, before doing things like trying to rename/move the file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102736/new/

https://reviews.llvm.org/D102736

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102736: Fix tmp files being left on Windows builds.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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

I seem to recall some thrashing on this topic a few months ago.  If I'm 
remembering correctly, setting the disposition to delete temporary files on 
Windows was causing problems with Rust builds because you can't always set the 
delete disposition (e.g., for a file on a network drive).  I think it got 
pulled out, and then put back in in a limited way.

It certainly would be nice to clean up temporaries left because of a crash, but 
it's even better not to crash. :-)  Lots of programs will leave partially 
written files if they die unexpectedly, and sometimes they can be useful when 
figuring out what happened.  Part of me wonders whether we should reclaim 
orphaned temporaries on the next run instead.  That might not be hard if we had 
a predictable location and naming scheme.

Another crazy idea would be to have the driver sit idly while the compiler and 
linker do their thing, and then clean up behind them if necessary.

Tests, however, always need to be cleaned up one way or another.

I'll take a closer look in the morning.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102736/new/

https://reviews.llvm.org/D102736

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102339: [clang] On Windows, ignore case and separators when discarding duplicate dependency file paths.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added inline comments.



Comment at: clang/lib/Frontend/DependencyFile.cpp:145
+  StringRef SearchPath;
+#ifdef _WIN32
+  // Make the search insensitive to case and separators.

rnk wrote:
> I feel like this should somehow be a property of the input virtual 
> filesystem: we should be able to ask the VFS to do the path canonicalization 
> for us, or ask if the FS is case insensitive and do the lower-casing if so.
Additional complication:

Windows NTFS partitions can have case-sensitivity enabled on a per-directory 
basis, so there is no single answer for the filesystem or even the host OS.  
Granted, this is not yet a commonly used feature.

I wonder about the (also rarely used) case of cross compiling for a Linux 
target on a Windows build machine.



Comment at: clang/lib/Frontend/DependencyFile.cpp:149
+  std::replace(TmpPath.begin(), TmpPath.end(), '/', '\\');
+  std::transform(TmpPath.begin(), TmpPath.end(), TmpPath.begin(), ::tolower);
+  SearchPath = TmpPath.str();

If `::tolower` is the same as `std::tolower` (e.g., if the header declared it 
in both the `std` and global namespaces), and if the locale is the default "C" 
locale, then this downcases just the 26 ASCII capital letters.  Windows 
considers more characters when it's wearing its case-blinding glasses. 
 For example, the pre-composed `U+00C1 LATIN CAPITAL LETTER A WITH ACUTE 
ACCENT` and `U+00E1 LATIN SMALL LETTER A WITH ACUTE ACCENT` characters match in 
case-blind file names.

I'll concede that this is probably an existing problem elsewhere in LLVM, so it 
may not be a high enough priority to worry about right now.  It just 
underscores the need for better centralized handling of paths and file names, 
so that we'll have a handle these sorts of problems if/when we ever accept that 
Windows is not Posix with backward slashes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102339/new/

https://reviews.llvm.org/D102339

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101479: [Driver] Support libc++ in MSVC

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.

Ah, thanks for explaining that!  In the VC++ stack, `/MD` and `/MT` make the 
DLL/static choice for the CRT, the C++ standard library, and the compiler 
runtime.[1]  It never occurred to me that someone might want to select these 
independently.

[1]:  
https://docs.microsoft.com/en-us/cpp/c-runtime-library/crt-library-features?view=msvc-160


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101479/new/

https://reviews.llvm.org/D101479

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101479: [Driver] Support libc++ in MSVC

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101479/new/

https://reviews.llvm.org/D101479

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101378: [llvm, clang] Remove stdlib includes from .h files without `std::`

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

A drive-by look.




Comment at: clang/include/clang/Tooling/Transformer/Parsing.h:24
 #include 
-#include 
 

`` appears unnecessary as well.

And while this doesn't require `` it does require 
`llvm/ADT/StringRef.h`.



Comment at: llvm/include/llvm/DebugInfo/CodeView/GlobalTypeTableBuilder.h:72
   template 
   TypeIndex insertRecordAs(GloballyHashedType Hash, size_t RecordSize,
CreateFunc Create) {

`size_t` may have been coming indirectly from ``.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101378/new/

https://reviews.llvm.org/D101378

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100756: [llvm-rc] [4/4] Add a GNU windres-like frontend to llvm-rc

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

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.




Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:295
+ .Case(".o", Coff)
+ .Default(Unknown);
+  if (F != Unknown)

".obj"?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100756/new/

https://reviews.llvm.org/D100756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68321: Fix clang Visual Studio build instructions

[Adrian McCarthy via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6e77a67171e6: Fix clang Visual Studio build instructions 
(authored by Loghorn, committed by amccarth).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68321/new/

https://reviews.llvm.org/D68321

Files:
  clang/www/get_started.html


Index: clang/www/get_started.html
===
--- clang/www/get_started.html
+++ clang/www/get_started.html
@@ -137,7 +137,7 @@
   
   Run CMake to generate the Visual Studio solution and project files:
   
-cd ..\..  (back to where you started)
+cd llvm-project
 mkdir build (for building without polluting the source 
dir)
 cd build
 


Index: clang/www/get_started.html
===
--- clang/www/get_started.html
+++ clang/www/get_started.html
@@ -137,7 +137,7 @@
   
   Run CMake to generate the Visual Studio solution and project files:
   
-cd ..\..  (back to where you started)
+cd llvm-project
 mkdir build (for building without polluting the source dir)
 cd build
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68321: Fix clang Visual Studio build instructions

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

I'll merge it for you.

Reviewers generally assume you have commit access and will submit it yourself 
unless tell them otherwise.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68321/new/

https://reviews.llvm.org/D68321

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100488: [SystemZ][z/OS] Add IsText Argument to GetFile and GetFileOrSTDIN

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100488/new/

https://reviews.llvm.org/D100488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99580: [CLANG] [DebugInfo] Convert File name to native format

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.

If canonicalizing the file names to the platform's native style creates new 
test failures, then I think we need to wonder whether canonicalization is the 
right solution (and whether it's done at the right point.  If it is, then I 
think we need to find a way to make these tests work cross-platform.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99580/new/

https://reviews.llvm.org/D99580

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99580: [CLANG] [DebugInfo] Convert File name to native format

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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 fix is on the code path for all platforms, so running the test on all 
platforms could help catch future regressions.  There could also be unusual 
cases, such as cross compiling for Linux on Windows, and thus you could have 
Windows-style paths even though the target is Posix.  Or someone might further 
refine the fix to make more cases work correctly for mingw but inadvertently 
cause problems for other platforms.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99580/new/

https://reviews.llvm.org/D99580

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99580: [CLANG] [DebugInfo] Convert File name to native format

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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'm also told that clang _generally_ tries to avoid canonicalization and 
instead treats file paths as mostly opaque strings that can be concatenated.  
Personally, I prefer file paths in the style that the native system prefers, so 
I'd like to see this go through, but I'm not sure how compatible this would be 
with other aspects of clang's file path handling.

Another possible issue is that `llvm::sys::path` and other functions try to map 
Windows filesystem concepts to Posix ones.  There are many cases where there 
isn't a perfect mapping.  For example:  Windows has a current working directory 
_per drive_, and so it can be hard to resolve paths like `D:foo.ext`, which are 
relative to something other than the "current working directory."  Details like 
this can come into play in the immediate vicinity of the code change, so I have 
some trepidation.

It looks like the code change is for everyone, but the new test is specific to 
mingw.  Is that the right place for the new test?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99580/new/

https://reviews.llvm.org/D99580

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.

I'm not concerned by the number of files touched, since it's mostly a 
mechanical refactor.  If anyone's worried that the mechanical changes obscure 
the behavior change, this _could_ be broken into an NFC refactor patch for the 
renaming followed by a patch that implements the behavioral distinction.  But 
I'm not going to insist on that.




Comment at: llvm/include/llvm/Support/FileSystem.h:746
   /// The file should be opened in text mode on platforms that make this
   /// distinction.
   OF_Text = 1,

Don't be shy to give examples in the comments, as they can illuminate the 
motivation for the design.

```
... on platforms, like SystemZ, that distinguish between text and binary files.
```



Comment at: llvm/include/llvm/Support/FileSystem.h:757
+  /// OF_TextWithCRLF = OF_Text | OF_CRLF
+  OF_TextWithCRLF = 3,
+

Nit:  If it were me, I'd put the `OF_Text | OF_CRLF` directly in the code (in 
place of the `3`) instead of in the comment.



Comment at: llvm/lib/Support/Windows/Path.inc:1086
 
-  if (Flags & OF_Text)
+  if (Flags & OF_CRLF)
 CrtOpenFlags |= _O_TEXT;

I assume it would be weird to set `OF_CRLF` without also setting `OF_Text`, so 
this change seems right for correctness.  But maybe this Windows-only code 
would better express the intent if it were written:

```
if (Flags & (OF_CRLF | OF_Text))
```

Another idea would be to assert if `OF_CRLF` is set but `OF_Text` is not.

Or maybe it's fine as it is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99426/new/

https://reviews.llvm.org/D99426

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97785/new/

https://reviews.llvm.org/D97785

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99200: [SystemZ][z/OS] JSON file should be text files

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99200/new/

https://reviews.llvm.org/D99200

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

From a Windows point of view, this LGTM.




Comment at: clang/lib/Frontend/FrontendActions.cpp:841
 }
   }
 

The preceding block that detects the type of line separator seems ripe for 
factoring out into a separate function.  It's a lot of low-level detail that 
visually outweighs the primary intent of this method.

But I'm fine with the change as it doesn't impact Windows and--as far as I 
understand--Posix platforms don't distinguish between text and binary mode.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97785/new/

https://reviews.llvm.org/D97785

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97437: Rewrite MSVC toolchain discovery with VFS

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97437/new/

https://reviews.llvm.org/D97437

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88220: [C++20] P1825R0: More implicit moves

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88220/new/

https://reviews.llvm.org/D88220

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97364: [docs] Add a release note for the removing of -Wreturn-std-move-in-c++11

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97364/new/

https://reviews.llvm.org/D97364

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96971: Allow but ignore `-Wreturn-std-move-in-c++11`

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth abandoned this revision.
amccarth added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96971/new/

https://reviews.llvm.org/D96971

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96971: Allow but ignore `-Wreturn-std-move-in-c++11`

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth created this revision.
amccarth added reviewers: thakis, nullptr.cpp.
amccarth requested review of this revision.

This warning is no longer needed, but at least a few open source projects still 
build with this option (and `-Werror`).  Accepting but ignoring the option 
keeps these projects building.


https://reviews.llvm.org/D96971

Files:
  clang/include/clang/Basic/DiagnosticGroups.td


Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -499,6 +499,7 @@
 def Padded : DiagGroup<"padded">;
 
 def PessimizingMove : DiagGroup<"pessimizing-move">;
+def : DiagGroup<"return-std-move-in-c++11">;
 def ReturnStdMove : DiagGroup<"return-std-move">;
 
 def PointerArith : DiagGroup<"pointer-arith">;


Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -499,6 +499,7 @@
 def Padded : DiagGroup<"padded">;
 
 def PessimizingMove : DiagGroup<"pessimizing-move">;
+def : DiagGroup<"return-std-move-in-c++11">;
 def ReturnStdMove : DiagGroup<"return-std-move">;
 
 def PointerArith : DiagGroup<"pointer-arith">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88220: [C++20] P1825R0: More implicit moves

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.

Would anyone object to allowing the option but silently ignoring it, at least 
for a transition period?  Was there prior notice of this option being 
deprecated?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88220/new/

https://reviews.llvm.org/D88220

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89702: [clang] [msvc] Automatically link against oldnames just as linking against libcmt

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89702/new/

https://reviews.llvm.org/D89702

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

LGTM.  Thanks for extending this functionality to Windows!




Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:15
+#include "llvm/Support/Windows/WindowsSupport.h"
 #include 
 #include 

I don't see a reason to include `` here.



Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:77
+  void WatcherThreadProc(HANDLE DirectoryHandle);
+  void NotifierThreadProc(bool WaitForInitialSync);
 };

I like the name change from HandlerThread to NotifierThread.  Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88666/new/

https://reviews.llvm.org/D88666

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth requested changes to this revision.
amccarth added a comment.
This revision now requires changes to proceed.

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:

1. Use the FindFirstChange function to get a handle to wait on for the 
directory change and create a separate event to signal when it's time to exit.  
The watcher thread would use WaitForMultipleObjects.  If' it's a directory 
change, then it can make a synchronous call the ReadDirectoryChangesW, knowing 
that there's info available.  (It could possibly even do the callback at that 
point, without the need for a separate handler thread.)

2. Continue to use the ReadDirectoryChangesW with overlapped IO, but, instead 
of waiting in GetOverlappedResult, it would first use WaitForMultipleObjects on 
the event in the overlapped IO and a distinct event used to tell the threat to 
exit (as in #1).




Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:36
+  alignas(DWORD)
+  CHAR Buffer[4 * (sizeof(FILE_NOTIFY_INFORMATION) + MAX_PATH * 
sizeof(WCHAR))];
+

compnerd wrote:
> amccarth wrote:
> > If it were me, I'd probably make this a `std::vector`.
> > 
> > * If an off-by-one bug causes an overrun of one WCHAR, you could trash a 
> > crucial member variable.  On the heap, the damage is less likely to be 
> > catastrophic.
> > * You wouldn't need `alignas`.
> > * I don't think these are created in a tight loop, so the overhead doesn't 
> > concern me.
> > 
> > Also, I'd probably go with a slightly more descriptive name, like 
> > `Notifications` rather than `Buffer`.
> The `alignas` is because the documentation states that the buffer should be 
> DWORD aligned.  It is more for pedantic reasons rather than anything else.  I 
> think that making it a catastrophic failure is a good thing though - it would 
> catch the error :)  You are correct about the allocation - it is once per 
> watch.  I'll rename it at least.
But it's still an arbitrarily-sized buffer in the middle of a class definition. 
 If you change your mind about how big to make it, it changes the definition of 
the class.  The buffer is going to be accessed using pointer arithmetic, which 
is generally dangerous.  Moving the buffer out of the class avoids both of 
those problems.

The alignas here is _not_ pedantic, it's essential.  Without it, you could 
easily have an alignment problem.  But if you used a vector, you'd know that it 
would always be suitably aligned.



Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:82
+DirectoryWatcherCallback Receiver)
+: Callback(Receiver) {
+  // Pre-compute the real location as we will be handing over the directory

compnerd wrote:
> amccarth wrote:
> > There's a lot going on in this constructor.  Is this how the other 
> > implementations are arranged?
> > 
> > Would it make sense to just initialize the object, and save most of the 
> > actual work to a `Watch` method?
> Largely the same.  However, the majority of the "work" is actually the thread 
> proc for the two threads.
Let me put it another way.  Constructors cannot return errors and LLVM does use 
exceptions, so things that can fail generally shouldn't be in the constructor.  
This code is accessing the file system, creating and event, and spawning two 
threads.  Any of those things can fail, but you've got no way to let the caller 
know whether something went wrong.

If the lambdas were really short, then it would be easy to see that they're 
thread procs.  But they're not, so they're hard to find and understand.  If 
they were private member functions with descriptive names, the code would be 
easier to understand.



Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:92
+Length = GetFinalPathNameByHandleW(DirectoryHandle, Buffer.data(),
+   Buffer.capacity(), 0);
+Buffer.resize(Length);

Using `Buffer.data()` when you've only reserved space is undefined behavior.  
You should used `resize` instead of `reserve` and then pass the `size` rather 
than the `capacity`.

Be aware that, while unlikely, this could still fail.  The directory could have 
been removed or renamed between calls or the caller could have passed a bad 
handle to begin with.



Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:195
+  // Signal the Watcher to exit.
+  SetEvent(Overlapped.hEvent);
+  HandlerThread.join();

I don't think this is a reliable way to get the watcher thread to exit.

You've overloaded the meaning of the event object and are racing the i/o system 
who plans to use the event for its own purposes.

Suppose the watcher thread is waiting in GetOverlappedResult.  If you set the 
event, then the other 

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.

The high level issues on my mind:

I'm wondering whether this has been overcomplicated with the overlapped IO.  If 
the monitoring thread used `FindFirstChangeNotificationW` to get a waitable 
handle and then used, then you'd be able to call `ReadDirectoryChangesW` 
synchronously.  In order to allow the parent thread signal to quit, they'd just 
need an Event and the monitor thread would use `WaitForMultipleObjects` to wait 
for either handle to become signaled.  Maybe I'm overlooking something, but it 
might be worth a few minutes of consideration.

We'll also have to think about how to test this.

The lower level issues that I've spotted are inlined.




Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:33
 class DirectoryWatcherWindows : public clang::DirectoryWatcher {
+  OVERLAPPED ovlIO;
+

The `ovlIO` name isn't consistent with LLVM style.



Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:36
+  alignas(DWORD)
+  CHAR Buffer[4 * (sizeof(FILE_NOTIFY_INFORMATION) + MAX_PATH * 
sizeof(WCHAR))];
+

If it were me, I'd probably make this a `std::vector`.

* If an off-by-one bug causes an overrun of one WCHAR, you could trash a 
crucial member variable.  On the heap, the damage is less likely to be 
catastrophic.
* You wouldn't need `alignas`.
* I don't think these are created in a tight loop, so the overhead doesn't 
concern me.

Also, I'd probably go with a slightly more descriptive name, like 
`Notifications` rather than `Buffer`.



Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:82
+DirectoryWatcherCallback Receiver)
+: Callback(Receiver) {
+  // Pre-compute the real location as we will be handing over the directory

There's a lot going on in this constructor.  Is this how the other 
implementations are arranged?

Would it make sense to just initialize the object, and save most of the actual 
work to a `Watch` method?



Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:87
+DWORD dwLength = GetFinalPathNameByHandleW(hDirectory, NULL, 0, 0);
+std::unique_ptr buffer{new WCHAR[dwLength + 1]};
+(void)GetFinalPathNameByHandleW(hDirectory, buffer.get(), dwLength + 1, 0);

compnerd wrote:
> aaron.ballman wrote:
> > Is a smart pointer required here or could you use `std::vector` and 
> > reserve the space that way?
> Sure, I can convert this to a `std::vector` instead.
* I guess it's fine to use the array form of `std::unique_ptr` (but then you 
should `#include `).  If it were me, I'd probably just use a 
`std::wstring` or `std::vector`.

* `dwLength` already includes the size of the null terminator.  Your first 
`GetFinalPathNameByHandleW` function "fails" because the buffer is too small.  
The does says that, if it fails because the buffer is too small, then the 
return value is the required size _including_ the null terminator.  (In the 
success case, it's the size w/o the terminator.)

* I know this is the Windows-specific implementation, but it might be best to 
just the Support api ` realPathFromHandle`, which does this and has tests.



Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:88
+std::unique_ptr buffer{new WCHAR[dwLength + 1]};
+(void)GetFinalPathNameByHandleW(hDirectory, buffer.get(), dwLength + 1, 0);
+sys::windows::UTF16ToUTF8(buffer.get(), dwLength + 1, Path);

I don't think you want to ignore the return value, since it'll tell you exactly 
how many characters you actually got back (or whether there was an error).  
Again, I recommend using `realPathFromHandle` from Support.



Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:94
+  ovlIO.hEvent =
+  CreateEvent(NULL, /*bManualReset=*/TRUE, /*bInitialState=*/FALSE, NULL);
+  assert(ovlIO.hEvent);

No real difference here, but, for consistency, please make this `CreateEventW` 
with the explicit -W suffix.



Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:189
+  HandlerThread.join();
+  WatcherThread.join();
+}

I think this leaks the `ovlIO.hEvent`.  After you've joined both threads, make 
sure to call `::CloseHandle()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88666/new/

https://reviews.llvm.org/D88666

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70378: [LLD][COFF] Cover usage of LLD as a library

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70378/new/

https://reviews.llvm.org/D70378

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83772: [Windows] Fix limit on command line size

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

In D83772#2164685 , @max-kudr wrote:

> This commit is now failing LLDB Windows buildbot 
>  builds 
> http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/17702. Please 
> fix or revert.
>
>   
> E:\build_slave\lldb-x64-windows-ninja\llvm-project\lldb\source\Host\windows\ProcessLauncherWindows.cpp(53):
>  error C2679: binary '=': no operator found which takes a right-hand operand 
> of type 'llvm::ErrorOr' (or there is no acceptable conversion)
>


I'll fix that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83772/new/

https://reviews.llvm.org/D83772



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83991: With MSVC, file needs to be compiled with /BIGOBJ

[Adrian McCarthy via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG14dde438d69c: With MSVC, file needs to be compiled with 
/BIGOBJ (authored by amccarth).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83991/new/

https://reviews.llvm.org/D83991

Files:
  clang/lib/ARCMigrate/CMakeLists.txt


Index: clang/lib/ARCMigrate/CMakeLists.txt
===
--- clang/lib/ARCMigrate/CMakeLists.txt
+++ clang/lib/ARCMigrate/CMakeLists.txt
@@ -2,6 +2,12 @@
   Support
   )
 
+# By default MSVC has a 2^16 limit on the number of sections in an object
+# file, and Transforms.cpp needs more than that.
+if (MSVC)
+  set_source_files_properties(Transforms.cpp PROPERTIES COMPILE_FLAGS /bigobj)
+endif()
+
 add_clang_library(clangARCMigrate
   ARCMT.cpp
   ARCMTActions.cpp


Index: clang/lib/ARCMigrate/CMakeLists.txt
===
--- clang/lib/ARCMigrate/CMakeLists.txt
+++ clang/lib/ARCMigrate/CMakeLists.txt
@@ -2,6 +2,12 @@
   Support
   )
 
+# By default MSVC has a 2^16 limit on the number of sections in an object
+# file, and Transforms.cpp needs more than that.
+if (MSVC)
+  set_source_files_properties(Transforms.cpp PROPERTIES COMPILE_FLAGS /bigobj)
+endif()
+
 add_clang_library(clangARCMigrate
   ARCMT.cpp
   ARCMTActions.cpp
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83772: [Windows] Fix limit on command line size

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83772/new/

https://reviews.llvm.org/D83772



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83772: [Windows] Fix limit on command line size

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

I'm jumping in since rnk is on leave and (I believe) zturner is less focused on 
these issues than he used to be.

Thanks for tracking down the cause of this bug.

I have some concerns:

- We're trying to cut it right up to the hard limit.  That seems an unnecessary 
risk, as the off-by-one bug illustrates.  Is there anything wrong with just 
rounding down to 32,000 and leaving ourselves some wiggle room?  Will someone 
be upset that we used a response file for a _very_ long command that 
technically would have been a viable command line?

- We're constructing a temporary copy of the command line to test if it's short 
enough and then constructing the actual command line elsewhere, which leaves us 
open to divergence between when we think the command line will be and what it 
actually ends up being.  I'd rather measure the actual command line, especially 
if we're going to run right up to the limit.

- We're checking to make sure that the number of UTF-8 code units is below the 
limit, but the limit is actually in terms of WCHARs.  Fortunately, I think that 
discrepancy works in our favor, since the number of WCHARs will always be 
smaller than the number of UTF-8 code units.  But it underscores the points I'm 
making above:  the precise limit probably isn't an issue and we're measuring a 
proxy command line rather than the actual command line.

Please consider these suggestions:

- Round down to 32,000 to leave us wiggle room.

- Have `flattenWindowsCommandLine` return a `wstring` rather than a `string`.  
This will reduce the chance of the proxy string we measure differing from the 
actual command string we're issuing, and it's already a Windows-specific 
function.  This will require a change in Execute (in the same source file), 
which is currently doing the conversion from UTF-8 to UTF-16.

- Add a short command to the test for `commandLineFitsWithinSystemLimits`.  
Right now, the test only checks a humongous command line.  (The test is in 
llvm\unittests\Support\CommandLineTest.cpp.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83772/new/

https://reviews.llvm.org/D83772



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81794: [clang] Don't emit warn_cxx_ms_struct when MSBitfields is enabled globally

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

Thanks.  This seems well-contained and the test changes look good.

(rnk is on leave and unlikely to respond.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81794/new/

https://reviews.llvm.org/D81794



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81041: Use existing path sep style in clang::FileManager::FixupRelativePath

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

In D81041#2072396 , @ctetreau wrote:

> After some further investigation, I have come to believe that the root cause 
> of the issue I am seeing is on line 783 of clang/lib/Lex/HeaderSearch.cpp. A 
> path is constructed using string concatenation (dir + '/' + file), which is 
> obviously not robust to the various issues in path construction. A fix had 
> been committed and reverted back in 2015.


When I was fixing portability problems with VFS paths, I started out by trying 
to make paths canonical, and that always led to roadblocks.  A clang developer 
told me that clang philosophically does not try to do any regularization of 
paths.  It turns out that's actually key to making VFS paths viable.  Since 
they can truly consist of a mix of styles, there is no "correct" canonical 
form.  Once I took that approach, most of the VFS portability problems were 
simple to fix without inflicting collateral damage.  So I'm not surprised that 
the 2015 "fix" causes problems.

I'm happy to look at future proposals, and I'll CC myself on that bug report.  
But since you've said you have other priorities now, I'll treat this patch as 
dormant.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81041/new/

https://reviews.llvm.org/D81041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.

My only real concern was about the possible reproducibility impact of 
introducing absolute paths into the binaries.  If @thakis and @hans are 
satisfied this is OK, then LGTM.




Comment at: clang/test/CodeGen/debug-info-codeview-buildinfo.c:8
+// CHECK: 
+// CHECK: 0x[[PWD:.+]] | LF_STRING_ID [size = {{.+}}] ID: , String: 
[[PWDVAL:.+]]
+// CHECK: 0x[[FILEPATH:.+]] | LF_STRING_ID [size = {{.+}}] ID: , 
String: [[FILEPATHVAL:.+[\\/]debug-info-codeview-buildinfo.c]]

aganea wrote:
> amccarth wrote:
> > PWD?  Did you mean CWD (current working directory)?
> I meant the current working directory when the program starts. I was under 
> the impression that the nomenclature for that is PWD = CWD at startup. While 
> CWD is the current directory at any point in time during the execution, and 
> can be different of PWD.
> Would you prefer if I changed the regex capture name to CWD?
Sorry, I always misread "pwd" as "password."




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80833/new/

https://reviews.llvm.org/D80833



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81041: Use existing path sep style in clang::FileManager::FixupRelativePath

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.

Exactly which assertion is firing?  Is it possible it's the assertion that's 
wrong?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81041/new/

https://reviews.llvm.org/D81041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

In D80833#2069874 , @thakis wrote:

> The change description says something about PWD


I believe that was a typo for CWD.

> Please don't add code that stores absolute paths, but at the moment this 
> doesn't do that as far as I can tell :)

Ah, but the summary suggests that it is:

> The LF_BUILDINFO therefore stores a full path to the compiler

From what I see, it's storing Args[0], which is usually the program name as 
specified on the command line.  If a build system is using full paths in the 
commands, I think they will be stored.




Comment at: clang/test/CodeGen/debug-info-codeview-buildinfo.c:8
+// CHECK: 
+// CHECK: 0x[[PWD:.+]] | LF_STRING_ID [size = {{.+}}] ID: , String: 
[[PWDVAL:.+]]
+// CHECK: 0x[[FILEPATH:.+]] | LF_STRING_ID [size = {{.+}}] ID: , 
String: [[FILEPATHVAL:.+[\\/]debug-info-codeview-buildinfo.c]]

PWD?  Did you mean CWD (current working directory)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80833/new/

https://reviews.llvm.org/D80833



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80833/new/

https://reviews.llvm.org/D80833



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.

Yowza.  Mingw still links against MSVCRT?!

Anyway, I think the patch is fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80880/new/

https://reviews.llvm.org/D80880



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80554: [DebugInfo] Use SplitTemplateClosers (foo >) in DWARF too

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth marked an inline comment as done.
amccarth added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:242
+// SplitTemplateClosers yields better interop with GCC and GDB (PR46052).
+PP.SplitTemplateClosers = true;
   }

sammccall wrote:
> amccarth wrote:
> > So, in other words, we'll always set `PP.SplitTemplateClosers` to true, now.
> Yes. I thought it was clearest to write it separately in each branch, as it's 
> basically a coincidence and I can give a separate comment.
> 
> Happy to fold it together if you think it's confusing.
No change necessary.  I agree with how you've done it.  I was just amused that, 
after fixing the regression for CodeView, it ended up being undone for DWARF as 
well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80554/new/

https://reviews.llvm.org/D80554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80554: [DebugInfo] Use SplitTemplateClosers (foo >) in DWARF too

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:242
+// SplitTemplateClosers yields better interop with GCC and GDB (PR46052).
+PP.SplitTemplateClosers = true;
   }

So, in other words, we'll always set `PP.SplitTemplateClosers` to true, now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80554/new/

https://reviews.llvm.org/D80554



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78508: [Clang] Allow long as size_t printf argument on 32-bit Windows platforms.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.




Comment at: clang/lib/AST/FormatString.cpp:407
+if ((isSizeT() || isPtrdiffT()) &&
+C.getTargetInfo().getTriple().isOSMSVCRT() &&
+C.getTargetInfo().getTriple().isArch32Bit())

I'm not convinced `isOSMSVCRT` is the right check.  The troubling typedefs are 
[[ 
https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types#size-t
 | `SIZE_T` and `SSIZE_T` ]], which come from the Windows API and are unrelated 
to Microsoft's C run-time library.  I think `isOSWindows` would be more precise.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

[Adrian McCarthy via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa549c0d00486: Fix template class debug info for Visual 
Studio visualizers (authored by amccarth).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79274/new/

https://reviews.llvm.org/D79274

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp


Index: clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
===
--- clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
+++ clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
@@ -1,11 +1,19 @@
 // RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: 
"\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
 // RUN: %clang_cc1 -fblocks -debug-info-kind=line-tables-only -gcodeview 
-emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep 'DISubprogram' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=QUAL
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++11 | \
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: 
"\([^"]*\)".*/"\1"/' | \
+// RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null | \
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: 
"\([^"]*\)".*/"\1"/' | \
+// RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
 
 void freefunc() { }
 // CHECK-DAG: "freefunc"
@@ -94,5 +102,7 @@
 
 template  struct ClassTemplate { A a; B b; 
C c; };
 ClassTemplate > f;
-// This will only show up in normal debug builds.
+// This will only show up in normal debug builds.  The space in `> >` is
+// important for compatibility with Windows debuggers, so it should always be
+// there when generating CodeView.
 // UNQUAL-DAG: "ClassTemplate >"
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -231,9 +231,12 @@
   // If we're emitting codeview, it's important to try to match MSVC's naming 
so
   // that visualizers written for MSVC will trigger for our class names. In
   // particular, we can't have spaces between arguments of standard templates
-  // like basic_string and vector.
-  if (CGM.getCodeGenOpts().EmitCodeView)
+  // like basic_string and vector, but we must have spaces between consecutive
+  // angle brackets that close nested template argument lists.
+  if (CGM.getCodeGenOpts().EmitCodeView) {
 PP.MSVCFormatting = true;
+PP.SplitTemplateClosers = true;
+  }
 
   // Apply -fdebug-prefix-map.
   PP.Callbacks = 


Index: clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
===
--- clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
+++ clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
@@ -1,11 +1,19 @@
 // RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
 // RUN: %clang_cc1 -fblocks -debug-info-kind=line-tables-only -gcodeview -emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep 'DISubprogram' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=QUAL
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++11 | \
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
+// RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null | \
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: 

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth updated this revision to Diff 263790.
amccarth added a comment.

Addressed feedback, specifically:

- Distinction is now on CodeView generation rather than -fms-compatibility.
- Tests two --std= levels plus the default one.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79274/new/

https://reviews.llvm.org/D79274

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp


Index: clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
===
--- clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
+++ clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
@@ -1,11 +1,19 @@
 // RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: 
"\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
 // RUN: %clang_cc1 -fblocks -debug-info-kind=line-tables-only -gcodeview 
-emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep 'DISubprogram' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=QUAL
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++11 | \
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: 
"\([^"]*\)".*/"\1"/' | \
+// RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null | \
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: 
"\([^"]*\)".*/"\1"/' | \
+// RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
 
 void freefunc() { }
 // CHECK-DAG: "freefunc"
@@ -94,5 +102,7 @@
 
 template  struct ClassTemplate { A a; B b; 
C c; };
 ClassTemplate > f;
-// This will only show up in normal debug builds.
+// This will only show up in normal debug builds.  The space in `> >` is
+// important for compatibility with Windows debuggers, so it should always be
+// there when generating CodeView.
 // UNQUAL-DAG: "ClassTemplate >"
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -231,9 +231,12 @@
   // If we're emitting codeview, it's important to try to match MSVC's naming 
so
   // that visualizers written for MSVC will trigger for our class names. In
   // particular, we can't have spaces between arguments of standard templates
-  // like basic_string and vector.
-  if (CGM.getCodeGenOpts().EmitCodeView)
+  // like basic_string and vector, but we must have spaces between consecutive
+  // angle brackets that close nested template argument lists.
+  if (CGM.getCodeGenOpts().EmitCodeView) {
 PP.MSVCFormatting = true;
+PP.SplitTemplateClosers = true;
+  }
 
   // Apply -fdebug-prefix-map.
   PP.Callbacks = 


Index: clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
===
--- clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
+++ clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
@@ -1,11 +1,19 @@
 // RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
 // RUN: %clang_cc1 -fblocks -debug-info-kind=line-tables-only -gcodeview -emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep 'DISubprogram' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=QUAL
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++11 | \
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
+// RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null | \
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
+// RUN:

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth marked 2 inline comments as done.
amccarth added a comment.

Made the requested changes after an in-person conversation to clear up my 
earlier confusion.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79274/new/

https://reviews.llvm.org/D79274



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth marked 2 inline comments as done.
amccarth added inline comments.



Comment at: clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp:10
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null 
-fms-compatibility | \
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: 
"\([^"]*\)".*/"\1"/' | \

rnk wrote:
> Why choose fms-compatibility? Does it have a side effect of raising the 
> default standard version?
> Why choose fms-compatibility?

I thought that `-fms-compatibility` was the solution we had discussed.

With ms-compat, we retain the space.  Without ms-compat, clang does what clang 
does (which, currently, is to omit the space).

> Does it have a side effect of raising the default standard version?

That was not my intent.  Removing the `-std=c++98` raises the standard.



Comment at: clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp:12
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: 
"\([^"]*\)".*/"\1"/' | \
+// RUN:FileCheck %s --check-prefix=CHECK --check-prefix=MSCOMPAT
 

rnk wrote:
> Any reason not to reuse `--check-prefix=UNQUAL`? This should be the same as 
> the first RUN line, with a different standard.
This partly overlaps with the thread about ms-compat.  But apart from that, I 
was trying to be explicit about what's being tested, which, in this case, is 
the ms-compat.  The reason the old test got the space is somewhat tangential to 
the qualified/unqualified distinction.

If you wanted to not pin to C++98 and not use ms-compat, then you wouldn't 
expect the space.  Maybe that's something that I should also test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79274/new/

https://reviews.llvm.org/D79274



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth updated this revision to Diff 262499.
amccarth added a comment.

Updated an existing test.  Thanks for the pointer; I had gotten myself 
completely confused about the organization of the test directories.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79274/new/

https://reviews.llvm.org/D79274

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp


Index: clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
===
--- clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
+++ clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
@@ -1,11 +1,15 @@
 // RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: 
"\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
 // RUN: %clang_cc1 -fblocks -debug-info-kind=line-tables-only -gcodeview 
-emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep 'DISubprogram' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=QUAL
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null 
-fms-compatibility | \
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: 
"\([^"]*\)".*/"\1"/' | \
+// RUN:FileCheck %s --check-prefix=CHECK --check-prefix=MSCOMPAT
 
 void freefunc() { }
 // CHECK-DAG: "freefunc"
@@ -96,3 +100,7 @@
 ClassTemplate > f;
 // This will only show up in normal debug builds.
 // UNQUAL-DAG: "ClassTemplate >"
+
+// Without -std=c++98, the nested template ends with ">>" instead of "> >".
+// We need to ensure -fms-compatibility keeps the space.
+// MSCOMPAT-DAG: "ClassTemplate >"
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -231,9 +231,12 @@
   // If we're emitting codeview, it's important to try to match MSVC's naming 
so
   // that visualizers written for MSVC will trigger for our class names. In
   // particular, we can't have spaces between arguments of standard templates
-  // like basic_string and vector.
-  if (CGM.getCodeGenOpts().EmitCodeView)
+  // like basic_string and vector, but we must have spaces between consecutive
+  // angle brackets that close nested template argument lists.
+  if (CGM.getCodeGenOpts().EmitCodeView) {
 PP.MSVCFormatting = true;
+PP.SplitTemplateClosers = true;
+  }
 
   // Apply -fdebug-prefix-map.
   PP.Callbacks = 


Index: clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
===
--- clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
+++ clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
@@ -1,11 +1,15 @@
 // RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
 // RUN: %clang_cc1 -fblocks -debug-info-kind=line-tables-only -gcodeview -emit-llvm %s \
-// RUN:   -o - -triple=x86_64-pc-win32 -std=c++98 | \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -std=c++98 | \
 // RUN:grep 'DISubprogram' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
 // RUN:FileCheck %s --check-prefix=CHECK --check-prefix=QUAL
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -fms-compatibility | \
+// RUN:grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
+// RUN:FileCheck %s --check-prefix=CHECK --check-prefix=MSCOMPAT
 
 void freefunc() { }
 // CHECK-DAG: "freefunc"
@@ -96,3 +100,7 @@
 ClassTemplate > f;
 // This will only show up in normal debug builds.
 // UNQUAL-DAG: "ClassTemplate >"
+
+// Without -std=c++98, the nested template ends with ">>" instead of "> >".
+// We need to ensure -fms-compatibility keeps the space.
+// MSCOMPAT-DAG: "ClassTemplate >"
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -231,9 +231,12 @@
   // If we're emitting codeview, it's important to try to match MSVC's naming so
   // that 

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

Updated test to validate the change.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79274/new/

https://reviews.llvm.org/D79274



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth created this revision.
amccarth added reviewers: rnk, hans.
Herald added a subscriber: aprantl.

An earlier change eliminated spaces between the close brackets of nested
template lists.  Unfortunately that prevents the Windows debuggers from
matching some types to their corresponding visualizers (e.g., std::map).

This selects the SeparateTemplateClosers flag when generating COFF debug
info.  Note that we were already making formatting adjustments under
similar circumstances for similar reasons.

This solves the immediate problem, but there aren't many tests for COFF
debug info (that I can see).  I will be looking into integrated tests with
Dexter next.


https://reviews.llvm.org/D79274

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp


Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -231,9 +231,12 @@
   // If we're emitting codeview, it's important to try to match MSVC's naming 
so
   // that visualizers written for MSVC will trigger for our class names. In
   // particular, we can't have spaces between arguments of standard templates
-  // like basic_string and vector.
-  if (CGM.getCodeGenOpts().EmitCodeView)
+  // like basic_string and vector, but we must have spaces between consecutive
+  // angle brackets that close nested template argument lists.
+  if (CGM.getCodeGenOpts().EmitCodeView) {
 PP.MSVCFormatting = true;
+PP.SplitTemplateClosers = true;
+  }
 
   // Apply -fdebug-prefix-map.
   PP.Callbacks = 


Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -231,9 +231,12 @@
   // If we're emitting codeview, it's important to try to match MSVC's naming so
   // that visualizers written for MSVC will trigger for our class names. In
   // particular, we can't have spaces between arguments of standard templates
-  // like basic_string and vector.
-  if (CGM.getCodeGenOpts().EmitCodeView)
+  // like basic_string and vector, but we must have spaces between consecutive
+  // angle brackets that close nested template argument lists.
+  if (CGM.getCodeGenOpts().EmitCodeView) {
 PP.MSVCFormatting = true;
+PP.SplitTemplateClosers = true;
+  }
 
   // Apply -fdebug-prefix-map.
   PP.Callbacks = 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79223: Fix pr31836 on Windows too, and correctly handle repeated separators.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

OK as is, but please consider re-using `llvm::sys::path` to distinguish 
separators.

Please also check that there are no regressions in the clang VFS tests.




Comment at: clang/lib/Lex/PPDirectives.cpp:2118
+return c == '/' || (BackslashIsSeparator && c == '\\');
+  };
+

Can you re-use `llvm::sys::path::is_separator` instead of inventing a new 
thing?  You can explicitly pass `llvm::path::Style::windows` if the Microsoft 
extensions are enabled.  Otherwise let it default to llvm::path::Style::native.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79223/new/

https://reviews.llvm.org/D79223



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78508: [Clang] Allow long as size_t printf argument on 32-bit Windows platforms.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

In D78508#2005311 , @aaron.ballman 
wrote:

> Personally, I don't consider those to be useless casts. They appropriately 
> ensure that the types match between the arguments and the format specifiers, 
> rather than relying on underlying details of what SIZE_T expands to for a 
> given target. I would expect these to at least use the 
> `warn_format_conversion_argument_type_mismatch_confusion` diagnostic rather 
> than be silenced entirely.


+1.  This is useful to anyone trying to write code that compiles for both 32- 
and 64-bit code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71092: [VFS] More consistent support for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth closed this revision.
amccarth added a comment.

This landed in da45bd232165eab5d6ec4f1f4f18db8644289142 


It wasn't automatically closed because I misspelled "Differential Revision."


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71092/new/

https://reviews.llvm.org/D71092



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71092: [VFS] More consistent support for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth updated this revision to Diff 242190.
amccarth marked an inline comment as done.
amccarth added a comment.

Addressed rnk's comments:  fixed path style detection bug, and used 
`StringRef::endswith`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71092/new/

https://reviews.llvm.org/D71092

Files:
  clang/test/VFS/external-names.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -2148,11 +2148,9 @@
   ASSERT_FALSE(Status.getError());
   EXPECT_TRUE(Status->exists());
 
-#if !defined(_WIN32)
   Status = FS->status("../bar/baz/a");
   ASSERT_FALSE(Status.getError());
   EXPECT_TRUE(Status->exists());
-#endif
 }
 
 TEST_F(VFSFromYAMLTest, WorkingDirectoryFallthroughInvalid) {
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -990,6 +990,28 @@
 // RedirectingFileSystem implementation
 //===---===/
 
+namespace {
+
+/// Removes leading "./" as well as path components like ".." and ".".
+static llvm::SmallString<256> canonicalize(llvm::StringRef Path) {
+  // First detect the path style in use by checking the first separator.
+  llvm::sys::path::Style style = llvm::sys::path::Style::native;
+  const size_t n = Path.find_first_of("/\\");
+  if (n != static_cast(-1))
+style = (Path[n] == '/') ? llvm::sys::path::Style::posix
+ : llvm::sys::path::Style::windows;
+
+  // Now remove the dots.  Explicitly specifying the path style prevents the
+  // direction of the slashes from changing.
+  llvm::SmallString<256> result =
+  llvm::sys::path::remove_leading_dotslash(Path, style);
+  llvm::sys::path::remove_dots(result, /*remove_dot_dot=*/true, style);
+  return result;
+}
+
+} // anonymous namespace
+
+
 RedirectingFileSystem::RedirectingFileSystem(IntrusiveRefCntPtr FS)
 : ExternalFS(std::move(FS)) {
   if (ExternalFS)
@@ -1083,7 +1105,23 @@
   if (!WorkingDir)
 return WorkingDir.getError();
 
-  llvm::sys::fs::make_absolute(WorkingDir.get(), Path);
+  // We can't use sys::fs::make_absolute because that assumes the path style
+  // is native and there is no way to override that.  Since we know WorkingDir
+  // is absolute, we can use it to determine which style we actually have and
+  // append Path ourselves.
+  sys::path::Style style = sys::path::Style::windows;
+  if (sys::path::is_absolute(WorkingDir.get(), sys::path::Style::posix)) {
+style = sys::path::Style::posix;
+  }
+
+  std::string Result = WorkingDir.get();
+  StringRef Dir(Result);
+  if (!Dir.endswith(sys::path::get_separator(style))) {
+Result += sys::path::get_separator(style);
+  }
+  Result.append(Path.data(), Path.size());
+  Path.assign(Result.begin(), Result.end());
+
   return {};
 }
 
@@ -1318,8 +1356,8 @@
 bool HasContents = false; // external or otherwise
 std::vector>
 EntryArrayContents;
-std::string ExternalContentsPath;
-std::string Name;
+SmallString<256> ExternalContentsPath;
+SmallString<256> Name;
 yaml::Node *NameValueNode = nullptr;
 auto UseExternalName =
 RedirectingFileSystem::RedirectingFileEntry::NK_NotSet;
@@ -1342,16 +1380,9 @@
   return nullptr;
 
 NameValueNode = I.getValue();
-if (FS->UseCanonicalizedPaths) {
-  SmallString<256> Path(Value);
-  // Guarantee that old YAML files containing paths with ".." and "."
-  // are properly canonicalized before read into the VFS.
-  Path = sys::path::remove_leading_dotslash(Path);
-  sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  Name = std::string(Path.str());
-} else {
-  Name = std::string(Value);
-}
+// Guarantee that old YAML files containing paths with ".." and "."
+// are properly canonicalized before read into the VFS.
+Name = canonicalize(Value).str();
   } else if (Key == "type") {
 if (!parseScalarString(I.getValue(), Value, Buffer))
   return nullptr;
@@ -1404,13 +1435,10 @@
   FullPath = Value;
 }
 
-if (FS->UseCanonicalizedPaths) {
-  // Guarantee that old YAML files containing paths with ".." and "."
-  // are properly canonicalized before read into the VFS.
-  FullPath = sys::path::remove_leading_dotslash(FullPath);
-  sys::path::remove_dots(FullPath, /*remove_dot_dot=*/true);
-}
-ExternalContentsPath = std::string(FullPath.str());
+// Guarantee that old YAML files containing paths with 

[PATCH] D71092: [VFS] More consistent support for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth marked 3 inline comments as done.
amccarth added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:-1113
+  sys::path::Style style = sys::path::Style::posix;
+  if (sys::path::is_absolute(WorkingDir.get(), sys::path::Style::windows)) {
+style = sys::path::Style::posix;

rnk wrote:
> It looks like you assign posix style if the working directory has windows 
> path style. Is that what you meant to do?
Oops!  Nice catch.

Although some of the tests exercise this path, none of them caught the problem 
because the effect here is very minor (direction of a slash).  I've turned the 
logic around, which makes the more common case (posix) dependent upon the 
condition.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1117
+  std::string Result = WorkingDir.get();
+  if (Result.empty() || Result.back() != sys::path::get_separator(style)[0]) {
+Result += sys::path::get_separator(style);

rnk wrote:
> `Result.endswith(sys::path::get_separator(style))` saves a boolean condition
I hadn't realized `ends_with` was added to `std::string`.  But it's a C++20 
enhancement, which I think is too new for us right now.  I'll use 
`StringRef::endswith`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71092/new/

https://reviews.llvm.org/D71092



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71092: [VFS] More consistent support for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

Ping?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71092/new/

https://reviews.llvm.org/D71092



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

In D73457#1842493 , @simon_tatham 
wrote:

> Removed the special case for `MSCompatibilityVersion == 0`. If the default 
> compatibility setting needs to be changed, that's a separate piece of work 
> and should be done by someone who understands more than I do about all those 
> failing tests :-) The new version of this patch just uses the existing 
> default.


+1.  The issue of how the default version gets set is a separate issue.  I 
think it's best to keep this patch decoupled from that.

> With the typical command line I use with the `clang-cl` driver, a specific 
> version has been set anyway by the time cc1 is running. So probably I 
> shouldn't have been worrying in the first place about what happens if there 
> isn't one set.

The default you found for MSVC15 is the last result default.  (Though I would 
have expected that to be MSVC17 by now.)  I believe the driver will actually 
try to figure out the most recent version of MSVC installed on the machine 
(assuming it's Windows), and use that.  Only if it can't find one, will it use 
that default.

As I recall, this is because clang will still (by default) use the MS runtime 
libraries for Windows builds, in which case it's important for the 
compatibility version to match the one for the libraries that are going to be 
used.  I think we can/should reconsider this when clang-cl starts using 
different run-time libraries for Windows builds.

I'm not officially a reviewer on this patch, but LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73457/new/

https://reviews.llvm.org/D73457



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71092: [VFS] More consistent support for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

cc: +thakis, who expressed interest in seeing a fix for the text exempted on 
Windows.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71092/new/

https://reviews.llvm.org/D71092



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71092: [VFS] More consistent support for Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth updated this revision to Diff 240240.
amccarth retitled this revision from "[VFS] Use path canonicalization on all 
paths" to "[VFS] More consistent support for Windows".
amccarth edited the summary of this revision.
Herald added a subscriber: ormris.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71092/new/

https://reviews.llvm.org/D71092

Files:
  clang/test/VFS/external-names.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -2147,11 +2147,9 @@
   ASSERT_FALSE(Status.getError());
   EXPECT_TRUE(Status->exists());
 
-#if !defined(_WIN32)
   Status = FS->status("../bar/baz/a");
   ASSERT_FALSE(Status.getError());
   EXPECT_TRUE(Status->exists());
-#endif
 }
 
 TEST_F(VFSFromYAMLTest, WorkingDirectoryFallthroughInvalid) {
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -989,6 +989,28 @@
 // RedirectingFileSystem implementation
 //===---===/
 
+namespace {
+
+/// Removes leading "./" as well as path components like ".." and ".".
+static llvm::SmallString<256> canonicalize(llvm::StringRef Path) {
+  // First detect the path style in use by checking the first separator.
+  llvm::sys::path::Style style = llvm::sys::path::Style::native;
+  const size_t n = Path.find_first_of("/\\");
+  if (n != static_cast(-1))
+style = (Path[n] == '/') ? llvm::sys::path::Style::posix
+ : llvm::sys::path::Style::windows;
+
+  // Now remove the dots.  Explicitly specifying the path style prevents the
+  // direction of the slashes from changing.
+  llvm::SmallString<256> result =
+  llvm::sys::path::remove_leading_dotslash(Path, style);
+  llvm::sys::path::remove_dots(result, /*remove_dot_dot=*/true, style);
+  return result;
+}
+
+} // anonymous namespace
+
+
 RedirectingFileSystem::RedirectingFileSystem(IntrusiveRefCntPtr FS)
 : ExternalFS(std::move(FS)) {
   if (ExternalFS)
@@ -1082,7 +1104,22 @@
   if (!WorkingDir)
 return WorkingDir.getError();
 
-  llvm::sys::fs::make_absolute(WorkingDir.get(), Path);
+  // We can't use sys::fs::make_absolute because that assumes the path style
+  // is native and there is no way to override that.  Since we know WorkingDir
+  // is absolute, we can use it to determine which style we actually have and
+  // append Path ourselves.
+  sys::path::Style style = sys::path::Style::posix;
+  if (sys::path::is_absolute(WorkingDir.get(), sys::path::Style::windows)) {
+style = sys::path::Style::posix;
+  }
+
+  std::string Result = WorkingDir.get();
+  if (Result.empty() || Result.back() != sys::path::get_separator(style)[0]) {
+Result += sys::path::get_separator(style);
+  }
+  Result.append(Path.data(), Path.size());
+  Path.assign(Result.begin(), Result.end());
+
   return {};
 }
 
@@ -1341,16 +1378,9 @@
   return nullptr;
 
 NameValueNode = I.getValue();
-if (FS->UseCanonicalizedPaths) {
-  SmallString<256> Path(Value);
-  // Guarantee that old YAML files containing paths with ".." and "."
-  // are properly canonicalized before read into the VFS.
-  Path = sys::path::remove_leading_dotslash(Path);
-  sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  Name = Path.str();
-} else {
-  Name = Value;
-}
+// Guarantee that old YAML files containing paths with ".." and "."
+// are properly canonicalized before read into the VFS.
+Name = canonicalize(Value).str();
   } else if (Key == "type") {
 if (!parseScalarString(I.getValue(), Value, Buffer))
   return nullptr;
@@ -1403,12 +1433,9 @@
   FullPath = Value;
 }
 
-if (FS->UseCanonicalizedPaths) {
-  // Guarantee that old YAML files containing paths with ".." and "."
-  // are properly canonicalized before read into the VFS.
-  FullPath = sys::path::remove_leading_dotslash(FullPath);
-  sys::path::remove_dots(FullPath, /*remove_dot_dot=*/true);
-}
+// Guarantee that old YAML files containing paths with ".." and "."
+// are properly canonicalized before read into the VFS.
+FullPath = canonicalize(FullPath);
 ExternalContentsPath = FullPath.str();
   } else if (Key == "use-external-name") {
 bool Val;
@@ -1653,14 +1680,10 @@
   if (std::error_code EC = makeAbsolute(Path))
 return EC;
 
-  // Canonicalize path by removing ".", "..", "./", etc components. This is
-  // a VFS request, do bot 

[PATCH] D71991: Fix external-names.c test when separator is \\

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

It would surprise me if the different test config is cause here.  The path is 
assembled entirely in the code, so it should be consistent.  If you had a 
systemic problem introduced by test config, I'd expect more of the VFS tests to 
fail.

Anyway, if you're back in business, that's great, but I can't promise future 
VFS changes won't cause similar problems for you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71991/new/

https://reviews.llvm.org/D71991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71991: Fix external-names.c test when separator is \\

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

I'm still not seeing how the backslashes got in there for you.  They don't 
happen for me.  It looks like these paths come (almost) directly from the temp 
.yaml files created by the `sed` commands at the beginning of the test, so--for 
these particular tests--I'd expect forward slashes regardless of platform.

I made a set of interelated VFS changes for Windows compatibility.  The last of 
those was on December 18.  Could your repo be older than that?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71991/new/

https://reviews.llvm.org/D71991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71991: Fix external-names.c test when separator is \\

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.

I've made a similar change in several other tests, but this test works for me 
without this change.  I'm trying to understand why, but I don't see any harm in 
landing this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71991/new/

https://reviews.llvm.org/D71991



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70701: Fix more VFS tests on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
amccarth marked an inline comment as done.
Closed by commit rG738b5c9639b4: Fix more VFS tests on Windows (authored by 
amccarth).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D70701?vs=230984=234592#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70701/new/

https://reviews.llvm.org/D70701

Files:
  clang/test/VFS/vfsroot-include.c
  clang/test/VFS/vfsroot-module.m
  clang/test/VFS/vfsroot-with-overlay.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1073,6 +1073,19 @@
   return ExternalFS->isLocal(Path, Result);
 }
 
+std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl ) const {
+  if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) ||
+  llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::windows))
+return {};
+
+  auto WorkingDir = getCurrentWorkingDirectory();
+  if (!WorkingDir)
+return WorkingDir.getError();
+
+  llvm::sys::fs::make_absolute(WorkingDir.get(), Path);
+  return {};
+}
+
 directory_iterator RedirectingFileSystem::dir_begin(const Twine ,
 std::error_code ) {
   ErrorOr E = lookupPath(Dir);
@@ -1428,21 +1441,31 @@
   return nullptr;
 }
 
-if (IsRootEntry && !sys::path::is_absolute(Name)) {
-  assert(NameValueNode && "Name presence should be checked earlier");
-  error(NameValueNode,
-"entry with relative path at the root level is not discoverable");
-  return nullptr;
+sys::path::Style path_style = sys::path::Style::native;
+if (IsRootEntry) {
+  // VFS root entries may be in either Posix or Windows style.  Figure out
+  // which style we have, and use it consistently.
+  if (sys::path::is_absolute(Name, sys::path::Style::posix)) {
+path_style = sys::path::Style::posix;
+  } else if (sys::path::is_absolute(Name, sys::path::Style::windows)) {
+path_style = sys::path::Style::windows;
+  } else {
+assert(NameValueNode && "Name presence should be checked earlier");
+error(NameValueNode,
+  "entry with relative path at the root level is not discoverable");
+return nullptr;
+  }
 }
 
 // Remove trailing slash(es), being careful not to remove the root path
 StringRef Trimmed(Name);
-size_t RootPathLen = sys::path::root_path(Trimmed).size();
+size_t RootPathLen = sys::path::root_path(Trimmed, path_style).size();
 while (Trimmed.size() > RootPathLen &&
-   sys::path::is_separator(Trimmed.back()))
+   sys::path::is_separator(Trimmed.back(), path_style))
   Trimmed = Trimmed.slice(0, Trimmed.size() - 1);
+
 // Get the last component
-StringRef LastComponent = sys::path::filename(Trimmed);
+StringRef LastComponent = sys::path::filename(Trimmed, path_style);
 
 std::unique_ptr Result;
 switch (Kind) {
@@ -1460,12 +1483,12 @@
   break;
 }
 
-StringRef Parent = sys::path::parent_path(Trimmed);
+StringRef Parent = sys::path::parent_path(Trimmed, path_style);
 if (Parent.empty())
   return Result;
 
 // if 'name' contains multiple components, create implicit directory entries
-for (sys::path::reverse_iterator I = sys::path::rbegin(Parent),
+for (sys::path::reverse_iterator I = sys::path::rbegin(Parent, path_style),
  E = sys::path::rend(Parent);
  I != E; ++I) {
   std::vector> Entries;
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -293,7 +293,7 @@
   /// \param Path A path that is modified to be an absolute path.
   /// \returns success if \a path has been made absolute, otherwise a
   ///  platform-specific error_code.
-  std::error_code makeAbsolute(SmallVectorImpl ) const;
+  virtual std::error_code makeAbsolute(SmallVectorImpl ) const;
 };
 
 /// Gets an \p vfs::FileSystem for the 'real' file system, as seen by
@@ -749,6 +749,8 @@
 
   std::error_code isLocal(const Twine , bool ) override;
 
+  std::error_code makeAbsolute(SmallVectorImpl ) const override;
+
   directory_iterator dir_begin(const Twine , std::error_code ) override;
 
   void setExternalContentsPrefixDir(StringRef PrefixDir);
Index: clang/test/VFS/vfsroot-with-overlay.c
===
--- clang/test/VFS/vfsroot-with-overlay.c
+++ clang/test/VFS/vfsroot-with-overlay.c
@@ -1,6 +1,3 @@
-// FIXME: PR43272
-// XFAIL: system-windows
-
 

[PATCH] D70701: Fix more VFS tests on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth marked 2 inline comments as done.
amccarth added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1431
 
-if (IsRootEntry && !sys::path::is_absolute(Name)) {
-  assert(NameValueNode && "Name presence should be checked earlier");

rnk wrote:
> amccarth wrote:
> > rnk wrote:
> > > Is there a way to unit test this? I see some existing tests in 
> > > llvm/unittests/Support/VirtualFileSystemTest.cpp.
> > > 
> > > I looked at the yaml files in the VFS tests this fixes, and I see entries 
> > > like this:
> > > { 'name': '/tests', 'type': 'directory', ... },
> > > { 'name': '/indirect-vfs-root-files', 'type': 'directory', ... },
> > > { 'name': 
> > > 'C:/src/llvm-project/clang/test/VFS/Inputs/Broken.framework', 'type': 
> > > 'directory', ... },
> > > { 'name': 
> > > 'C:/src/llvm-project/build/tools/clang/test/VFS/Output/vfsroot-with-overlay.c.tmp/cache',
> > >  'type': 'directory', ... },
> > > 
> > > So it makes sense to me that we need to handle both kinds of absolute 
> > > path.
> > > Is there a way to unit test this?
> > 
> > What do you mean by "this"?  The `/tests` and `/indirect-vfs-root-files` 
> > entries in that yaml are the ones causing several tests to fail without 
> > this fix, so I take it that this is already being tested.  But perhaps you 
> > meant testing something more specific?
> > What do you mean by "this"? 
> I guess what I meant was, can you unit test the whole change in case there 
> are behavior differences here not covered by the clang tests?
This change causes no regressions in those llvm unit tests 
(`llvm/unittests/Support/VirtualFileSystemTest.cpp`).  They all still pass.

But thanks for pointing those out, because my subsequent changes do seem to 
make a difference.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70701/new/

https://reviews.llvm.org/D70701



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71092: [VFS] Use path canonicalization on all paths

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth planned changes to this revision.
amccarth added a comment.

Hold off on this one.  It needs an explicit test for canonicalization.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71092/new/

https://reviews.llvm.org/D71092



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71092: [VFS] Use path canonicalization on all paths

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth created this revision.
amccarth added reviewers: rnk, vsapsai, arphaman.
Herald added subscribers: llvm-commits, dexonsmith, hiraditya.
Herald added a project: LLVM.

The virtual file system had an option to eliminate dots from paths (e.g., 
`/foo/./bar` -> `/foo/bar`).  Because of differences in Windows-style paths, 
this was disabled by default on Windows.

Once the earlier VFS portability fixes land we were just one away from enable 
canonicalization on Windows.  (See https://reviews.llvm.org/D70701)

Note that the old UsePathCanonicalization member could never be changed from 
the default, so now that we canonicalize on all systems, there was no need to 
keep the option nor the alternate code paths, so this patch deletes all of that.

Tested with ninja check-clang.


https://reviews.llvm.org/D71092

Files:
  clang/test/VFS/external-names.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1341,16 +1341,12 @@
   return nullptr;
 
 NameValueNode = I.getValue();
-if (FS->UseCanonicalizedPaths) {
-  SmallString<256> Path(Value);
-  // Guarantee that old YAML files containing paths with ".." and "."
-  // are properly canonicalized before read into the VFS.
-  Path = sys::path::remove_leading_dotslash(Path);
-  sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  Name = Path.str();
-} else {
-  Name = Value;
-}
+// Guarantee that old YAML files containing paths with ".." and "."
+// are properly canonicalized before read into the VFS.
+SmallString<256> Path(Value);
+Path = sys::path::remove_leading_dotslash(Path);
+sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
+Name = Path.str();
   } else if (Key == "type") {
 if (!parseScalarString(I.getValue(), Value, Buffer))
   return nullptr;
@@ -1403,12 +1399,11 @@
   FullPath = Value;
 }
 
-if (FS->UseCanonicalizedPaths) {
-  // Guarantee that old YAML files containing paths with ".." and "."
-  // are properly canonicalized before read into the VFS.
-  FullPath = sys::path::remove_leading_dotslash(FullPath);
-  sys::path::remove_dots(FullPath, /*remove_dot_dot=*/true);
-}
+// Guarantee that old YAML files containing paths with ".." and "."
+// are properly canonicalized before read into the VFS.
+FullPath = sys::path::remove_leading_dotslash(FullPath);
+sys::path::remove_dots(FullPath, /*remove_dot_dot=*/true);
+
 ExternalContentsPath = FullPath.str();
   } else if (Key == "use-external-name") {
 bool Val;
@@ -1653,13 +1648,11 @@
   if (std::error_code EC = makeAbsolute(Path))
 return EC;
 
-  // Canonicalize path by removing ".", "..", "./", etc components. This is
-  // a VFS request, do bot bother about symlinks in the path components
+  // Canonicalize path by removing ".", "..", "./", components. This is
+  // a VFS request, do not bother about symlinks in the path components
   // but canonicalize in order to perform the correct entry search.
-  if (UseCanonicalizedPaths) {
-Path = sys::path::remove_leading_dotslash(Path);
-sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  }
+  Path = sys::path::remove_leading_dotslash(Path);
+  sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
 
   if (Path.empty())
 return make_error_code(llvm::errc::invalid_argument);
@@ -1679,16 +1672,9 @@
 RedirectingFileSystem::lookupPath(sys::path::const_iterator Start,
   sys::path::const_iterator End,
   RedirectingFileSystem::Entry *From) const {
-#ifndef _WIN32
   assert(!isTraversalComponent(*Start) &&
  !isTraversalComponent(From->getName()) &&
  "Paths should not contain traversal components");
-#else
-  // FIXME: this is here to support windows, remove it once canonicalized
-  // paths become globally default.
-  if (Start->equals("."))
-++Start;
-#endif
 
   StringRef FromName = From->getName();
 
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -705,16 +705,6 @@
   bool IsFallthrough = true;
   /// @}
 
-  /// Virtual file paths and external files could be canonicalized without "..",
-  /// "." and "./" in their paths. FIXME: some unittests currently fail on
-  /// win32 when using remove_dots and remove_leading_dotslash on paths.
-  bool UseCanonicalizedPaths =
-#ifdef _WIN32
-  false;
-#else
-  true;
-#endif
-
   

[PATCH] D70701: Fix more VFS tests on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth marked 2 inline comments as done.
amccarth added a comment.

A (hopefully) more cogent response than my last round.  I'm still hoping to 
hear from VFS owners.




Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1077-1078
+std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl 
) const {
+  if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) ||
+  llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::windows))
+return {};

amccarth wrote:
> rnk wrote:
> > I think Posix-style paths are considered absolute, even on Windows. The 
> > opposite isn't true, a path with a drive letter is considered to be 
> > relative if the default path style is Posix. But, I don't think that 
> > matters. We only end up in this mixed path style situation on Windows.
> > 
> > Does this change end up being necessary? I would expect the existing 
> > implementation of makeAbsolute to do the right thing on Windows as is. I 
> > think the other change seems to be what matters.
> Yes, it's necessary.  The Posix-style path `\tests` is not considered 
> absolute on Windows.  Thus the original makeAboslute would merge it with the 
> current working directory, which gives it a drive letter, like `D:\tests\`.  
> The drive letter component then causes comparisons to fail.
To make sure I wasn't misremembering or hallucinating, I double-checked the 
behavior here:  Posix-style paths like `\tests` are not considered absolute 
paths in Windows-style.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1448-1449
+  // which style we have, and use it consistently.
+  if (sys::path::is_absolute(Name, sys::path::Style::posix)) {
+path_style = sys::path::Style::posix;
+  } else if (sys::path::is_absolute(Name, sys::path::Style::windows)) {

amccarth wrote:
> rnk wrote:
> > I wonder if there's a simpler fix here. If the working directory is an 
> > absolute Windows-style path, we could prepend the drive letter of the 
> > working directory onto any absolute Posix style paths read from YAML files. 
> > That's somewhat consistent with what native Windows tools do. In cmd, you 
> > can run `cd \Windows`, and that will mean `C:\Windows` if the active driver 
> > letter is C. I think on Windows every drive has an active directory, but 
> > that's not part of the file system model.
> I'm not seeing how this would be simpler.
> 
> I don't understand the analogy of your proposal to what the native Windows 
> tools do.  When you say, `cd \Windows`, the `\Windows` is _not_ an absolute 
> path.  It's relative to the current drive.
> 
> I could be wrong, but I don't think prepending the drive onto absolution 
> Posix style paths read from YAML would work.  That would give us something 
> like `D:/tests` (which is what was happening in makeAbsolute) and that won't 
> match paths generated from non-YAML sources, which will still come out as 
> `/tests/foo.h`.
> 
> > I think on Windows every drive has an active directory 
> 
> Yep.  I think of it as "every drive has a _current_ directory" and each 
> process has a current drive.  When you want the current working directory, 
> you get the current directory of the current drive.
> ... I don't think prepending the drive onto absolution Posix style paths read 
> from YAML would work

I misspoke.  The idea of prepending the drive onto absolute Posix-style paths 
read from the YAML probably could be made to work for the common cases like the 
ones in these tests.

But I still don't think that approach would simplify anything.

Furthermore, I think it could open a potential Pandora's box of weird corner 
cases.  For example, in a system with multiple drives, the current drive may 
not always be the "correct" one to use.  Slapping a drive letter onto a 
Posix-style path creates a Frankenstein hybrid that's neither Windows-style nor 
Posix-style.  Doing so because we know the subsequent code would then recognize 
it as an absolute path seems like a way to create an unnecessary coupling 
between the VFS YAML parser and the LLVM path support.

In my mind, the model here is that these roots can be in either style.  I 
prefer to let the code explicitly reflect that fact rather than trying to 
massage some of the paths into a form that happens to cause the right outcome.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70701/new/

https://reviews.llvm.org/D70701



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70701: Fix more VFS tests on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth marked 3 inline comments as done.
amccarth added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1077-1078
+std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl 
) const {
+  if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) ||
+  llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::windows))
+return {};

rnk wrote:
> I think Posix-style paths are considered absolute, even on Windows. The 
> opposite isn't true, a path with a drive letter is considered to be relative 
> if the default path style is Posix. But, I don't think that matters. We only 
> end up in this mixed path style situation on Windows.
> 
> Does this change end up being necessary? I would expect the existing 
> implementation of makeAbsolute to do the right thing on Windows as is. I 
> think the other change seems to be what matters.
Yes, it's necessary.  The Posix-style path `\tests` is not considered absolute 
on Windows.  Thus the original makeAboslute would merge it with the current 
working directory, which gives it a drive letter, like `D:\tests\`.  The drive 
letter component then causes comparisons to fail.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1431
 
-if (IsRootEntry && !sys::path::is_absolute(Name)) {
-  assert(NameValueNode && "Name presence should be checked earlier");

rnk wrote:
> Is there a way to unit test this? I see some existing tests in 
> llvm/unittests/Support/VirtualFileSystemTest.cpp.
> 
> I looked at the yaml files in the VFS tests this fixes, and I see entries 
> like this:
> { 'name': '/tests', 'type': 'directory', ... },
> { 'name': '/indirect-vfs-root-files', 'type': 'directory', ... },
> { 'name': 'C:/src/llvm-project/clang/test/VFS/Inputs/Broken.framework', 
> 'type': 'directory', ... },
> { 'name': 
> 'C:/src/llvm-project/build/tools/clang/test/VFS/Output/vfsroot-with-overlay.c.tmp/cache',
>  'type': 'directory', ... },
> 
> So it makes sense to me that we need to handle both kinds of absolute path.
> Is there a way to unit test this?

What do you mean by "this"?  The `/tests` and `/indirect-vfs-root-files` 
entries in that yaml are the ones causing several tests to fail without this 
fix, so I take it that this is already being tested.  But perhaps you meant 
testing something more specific?



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1448-1449
+  // which style we have, and use it consistently.
+  if (sys::path::is_absolute(Name, sys::path::Style::posix)) {
+path_style = sys::path::Style::posix;
+  } else if (sys::path::is_absolute(Name, sys::path::Style::windows)) {

rnk wrote:
> I wonder if there's a simpler fix here. If the working directory is an 
> absolute Windows-style path, we could prepend the drive letter of the working 
> directory onto any absolute Posix style paths read from YAML files. That's 
> somewhat consistent with what native Windows tools do. In cmd, you can run 
> `cd \Windows`, and that will mean `C:\Windows` if the active driver letter is 
> C. I think on Windows every drive has an active directory, but that's not 
> part of the file system model.
I'm not seeing how this would be simpler.

I don't understand the analogy of your proposal to what the native Windows 
tools do.  When you say, `cd \Windows`, the `\Windows` is _not_ an absolute 
path.  It's relative to the current drive.

I could be wrong, but I don't think prepending the drive onto absolution Posix 
style paths read from YAML would work.  That would give us something like 
`D:/tests` (which is what was happening in makeAbsolute) and that won't match 
paths generated from non-YAML sources, which will still come out as 
`/tests/foo.h`.

> I think on Windows every drive has an active directory 

Yep.  I think of it as "every drive has a _current_ directory" and each process 
has a current drive.  When you want the current working directory, you get the 
current directory of the current drive.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70701/new/

https://reviews.llvm.org/D70701



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70701: Fix more VFS tests on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth created this revision.
amccarth added reviewers: rnk, vsapsai, arphaman, Bigcheese.
Herald added subscribers: dexonsmith, hiraditya.
Herald added a project: LLVM.

Since VFS paths can be in either Posix or Windows style, we have to use a more 
flexible definition of "absolute" path.

The key here is that FileSystem::makeAbsolute is now virtual, and the 
RedirectingFileSystem override checks for either concept of absolute before 
trying to make the path absolute by combining it with the current directory.


https://reviews.llvm.org/D70701

Files:
  clang/test/VFS/vfsroot-include.c
  clang/test/VFS/vfsroot-module.m
  clang/test/VFS/vfsroot-with-overlay.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1073,6 +1073,19 @@
   return ExternalFS->isLocal(Path, Result);
 }
 
+std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl ) const {
+  if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) ||
+  llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::windows))
+return {};
+
+  auto WorkingDir = getCurrentWorkingDirectory();
+  if (!WorkingDir)
+return WorkingDir.getError();
+
+  llvm::sys::fs::make_absolute(WorkingDir.get(), Path);
+  return {};
+}
+
 directory_iterator RedirectingFileSystem::dir_begin(const Twine ,
 std::error_code ) {
   ErrorOr E = lookupPath(Dir);
@@ -1428,21 +1441,31 @@
   return nullptr;
 }
 
-if (IsRootEntry && !sys::path::is_absolute(Name)) {
-  assert(NameValueNode && "Name presence should be checked earlier");
-  error(NameValueNode,
-"entry with relative path at the root level is not discoverable");
-  return nullptr;
+sys::path::Style path_style = sys::path::Style::native;
+if (IsRootEntry) {
+  // VFS root entries may be in either Posix or Windows style.  Figure out
+  // which style we have, and use it consistently.
+  if (sys::path::is_absolute(Name, sys::path::Style::posix)) {
+path_style = sys::path::Style::posix;
+  } else if (sys::path::is_absolute(Name, sys::path::Style::windows)) {
+path_style = sys::path::Style::windows;
+  } else {
+assert(NameValueNode && "Name presence should be checked earlier");
+error(NameValueNode,
+  "entry with relative path at the root level is not discoverable");
+return nullptr;
+  }
 }
 
 // Remove trailing slash(es), being careful not to remove the root path
 StringRef Trimmed(Name);
-size_t RootPathLen = sys::path::root_path(Trimmed).size();
+size_t RootPathLen = sys::path::root_path(Trimmed, path_style).size();
 while (Trimmed.size() > RootPathLen &&
-   sys::path::is_separator(Trimmed.back()))
+   sys::path::is_separator(Trimmed.back(), path_style))
   Trimmed = Trimmed.slice(0, Trimmed.size() - 1);
+
 // Get the last component
-StringRef LastComponent = sys::path::filename(Trimmed);
+StringRef LastComponent = sys::path::filename(Trimmed, path_style);
 
 std::unique_ptr Result;
 switch (Kind) {
@@ -1460,12 +1483,12 @@
   break;
 }
 
-StringRef Parent = sys::path::parent_path(Trimmed);
+StringRef Parent = sys::path::parent_path(Trimmed, path_style);
 if (Parent.empty())
   return Result;
 
 // if 'name' contains multiple components, create implicit directory entries
-for (sys::path::reverse_iterator I = sys::path::rbegin(Parent),
+for (sys::path::reverse_iterator I = sys::path::rbegin(Parent, path_style),
  E = sys::path::rend(Parent);
  I != E; ++I) {
   std::vector> Entries;
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -293,7 +293,7 @@
   /// \param Path A path that is modified to be an absolute path.
   /// \returns success if \a path has been made absolute, otherwise a
   ///  platform-specific error_code.
-  std::error_code makeAbsolute(SmallVectorImpl ) const;
+  virtual std::error_code makeAbsolute(SmallVectorImpl ) const;
 };
 
 /// Gets an \p vfs::FileSystem for the 'real' file system, as seen by
@@ -749,6 +749,8 @@
 
   std::error_code isLocal(const Twine , bool ) override;
 
+  std::error_code makeAbsolute(SmallVectorImpl ) const override;
+
   directory_iterator dir_begin(const Twine , std::error_code ) override;
 
   void setExternalContentsPrefixDir(StringRef PrefixDir);
Index: clang/test/VFS/vfsroot-with-overlay.c
===
--- clang/test/VFS/vfsroot-with-overlay.c
+++ 

[PATCH] D69958: Improve VFS compatibility on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1275ab1620b6: Improve VFS compatibility on Windows (authored 
by amccarth).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69958/new/

https://reviews.llvm.org/D69958

Files:
  clang/test/Index/index-module-with-vfs.m
  clang/test/Modules/double-quotes.m
  clang/test/Modules/framework-public-includes-private.m
  clang/test/VFS/external-names.c
  clang/test/VFS/framework-import.m
  clang/test/VFS/implicit-include.c
  clang/test/VFS/include-mixed-real-and-virtual.c
  clang/test/VFS/include-real-from-virtual.c
  clang/test/VFS/include-virtual-from-real.c
  clang/test/VFS/include.c
  clang/test/VFS/incomplete-umbrella.m
  clang/test/VFS/module-import.m
  clang/test/VFS/real-path-found-first.m
  clang/test/VFS/relative-path.c
  clang/test/VFS/umbrella-framework-import-skipnonexist.m
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1671,9 +1671,7 @@
 
   // Forward the search to the next component in case this is an empty one.
   if (!FromName.empty()) {
-if (CaseSensitive ? !Start->equals(FromName)
-  : !Start->equals_lower(FromName))
-  // failure to match
+if (!pathComponentMatches(*Start, FromName))
   return make_error_code(llvm::errc::no_such_file_or_directory);
 
 ++Start;
@@ -1695,6 +1693,7 @@
 if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
   return Result;
   }
+
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -532,7 +532,7 @@
 /// \endverbatim
 ///
 /// All configuration options are optional.
-///   'case-sensitive': 
+///   'case-sensitive': 
 ///   'use-external-names': 
 ///   'overlay-relative': 
 ///   'fallthrough': 
@@ -651,6 +651,17 @@
 return ExternalFSValidWD && IsFallthrough;
   }
 
+  // In a RedirectingFileSystem, keys can be specified in Posix or Windows
+  // style (or even a mixture of both), so this comparison helper allows
+  // slashes (representing a root) to match backslashes (and vice versa).  Note
+  // that, other than the root, patch components should not contain slashes or
+  // backslashes.
+  bool pathComponentMatches(llvm::StringRef lhs, llvm::StringRef rhs) const {
+if ((CaseSensitive ? lhs.equals(rhs) : lhs.equals_lower(rhs)))
+  return true;
+return (lhs == "/" && rhs == "\\") || (lhs == "\\" && rhs == "/");
+  }
+
   /// The root(s) of the virtual file system.
   std::vector> Roots;
 
@@ -674,7 +685,12 @@
   /// Whether to perform case-sensitive comparisons.
   ///
   /// Currently, case-insensitive matching only works correctly with ASCII.
-  bool CaseSensitive = true;
+  bool CaseSensitive =
+#ifdef _WIN32
+  false;
+#else
+  true;
+#endif
 
   /// IsRelativeOverlay marks whether a ExternalContentsPrefixDir path must
   /// be prefixed in every 'external-contents' when reading from YAML files.
Index: clang/test/VFS/umbrella-framework-import-skipnonexist.m
===
--- clang/test/VFS/umbrella-framework-import-skipnonexist.m
+++ clang/test/VFS/umbrella-framework-import-skipnonexist.m
@@ -1,8 +1,5 @@
 // REQUIRES: crash-recovery
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t
 // RUN: mkdir -p %t/vdir %t/outdir %t/cache
 // RUN: cp -R %S/Inputs/Bar.framework %t/outdir/
Index: clang/test/VFS/relative-path.c
===
--- clang/test/VFS/relative-path.c
+++ clang/test/VFS/relative-path.c
@@ -3,9 +3,6 @@
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -I . -ivfsoverlay %t.yaml -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 #include "not_real.h"
 
 void foo() {
Index: clang/test/VFS/real-path-found-first.m
===
--- clang/test/VFS/real-path-found-first.m
+++ clang/test/VFS/real-path-found-first.m
@@ -4,9 +4,6 @@
 // intentionally rebuild modules, since the precompiled module file refers to
 // the dependency files by real path.
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t %t-cache %t.pch
 // RUN: mkdir -p %t/SomeFramework.framework/Modules
 // RUN: cat %S/Inputs/some_frame_module.map > %t/SomeFramework.framework/Modules/module.modulemap
Index: clang/test/VFS/module-import.m

[PATCH] D69958: Improve VFS compatibility on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth updated this revision to Diff 229197.
amccarth marked an inline comment as done.
amccarth added a comment.

Corrected comment about default for case sensitivity.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69958/new/

https://reviews.llvm.org/D69958

Files:
  clang/test/Index/index-module-with-vfs.m
  clang/test/Modules/double-quotes.m
  clang/test/Modules/framework-public-includes-private.m
  clang/test/VFS/external-names.c
  clang/test/VFS/framework-import.m
  clang/test/VFS/implicit-include.c
  clang/test/VFS/include-mixed-real-and-virtual.c
  clang/test/VFS/include-real-from-virtual.c
  clang/test/VFS/include-virtual-from-real.c
  clang/test/VFS/include.c
  clang/test/VFS/incomplete-umbrella.m
  clang/test/VFS/module-import.m
  clang/test/VFS/real-path-found-first.m
  clang/test/VFS/relative-path.c
  clang/test/VFS/umbrella-framework-import-skipnonexist.m
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1671,9 +1671,7 @@
 
   // Forward the search to the next component in case this is an empty one.
   if (!FromName.empty()) {
-if (CaseSensitive ? !Start->equals(FromName)
-  : !Start->equals_lower(FromName))
-  // failure to match
+if (!pathComponentMatches(*Start, FromName))
   return make_error_code(llvm::errc::no_such_file_or_directory);
 
 ++Start;
@@ -1695,6 +1693,7 @@
 if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
   return Result;
   }
+
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -532,7 +532,7 @@
 /// \endverbatim
 ///
 /// All configuration options are optional.
-///   'case-sensitive': 
+///   'case-sensitive': 
 ///   'use-external-names': 
 ///   'overlay-relative': 
 ///   'fallthrough': 
@@ -651,6 +651,17 @@
 return ExternalFSValidWD && IsFallthrough;
   }
 
+  // In a RedirectingFileSystem, keys can be specified in Posix or Windows
+  // style (or even a mixture of both), so this comparison helper allows
+  // slashes (representing a root) to match backslashes (and vice versa).  Note
+  // that, other than the root, patch components should not contain slashes or
+  // backslashes.
+  bool pathComponentMatches(llvm::StringRef lhs, llvm::StringRef rhs) const {
+if ((CaseSensitive ? lhs.equals(rhs) : lhs.equals_lower(rhs)))
+  return true;
+return (lhs == "/" && rhs == "\\") || (lhs == "\\" && rhs == "/");
+  }
+
   /// The root(s) of the virtual file system.
   std::vector> Roots;
 
@@ -674,7 +685,12 @@
   /// Whether to perform case-sensitive comparisons.
   ///
   /// Currently, case-insensitive matching only works correctly with ASCII.
-  bool CaseSensitive = true;
+  bool CaseSensitive =
+#ifdef _WIN32
+  false;
+#else
+  true;
+#endif
 
   /// IsRelativeOverlay marks whether a ExternalContentsPrefixDir path must
   /// be prefixed in every 'external-contents' when reading from YAML files.
Index: clang/test/VFS/umbrella-framework-import-skipnonexist.m
===
--- clang/test/VFS/umbrella-framework-import-skipnonexist.m
+++ clang/test/VFS/umbrella-framework-import-skipnonexist.m
@@ -1,8 +1,5 @@
 // REQUIRES: crash-recovery
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t
 // RUN: mkdir -p %t/vdir %t/outdir %t/cache
 // RUN: cp -R %S/Inputs/Bar.framework %t/outdir/
Index: clang/test/VFS/relative-path.c
===
--- clang/test/VFS/relative-path.c
+++ clang/test/VFS/relative-path.c
@@ -3,9 +3,6 @@
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -I . -ivfsoverlay %t.yaml -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 #include "not_real.h"
 
 void foo() {
Index: clang/test/VFS/real-path-found-first.m
===
--- clang/test/VFS/real-path-found-first.m
+++ clang/test/VFS/real-path-found-first.m
@@ -4,9 +4,6 @@
 // intentionally rebuild modules, since the precompiled module file refers to
 // the dependency files by real path.
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t %t-cache %t.pch
 // RUN: mkdir -p %t/SomeFramework.framework/Modules
 // RUN: cat %S/Inputs/some_frame_module.map > %t/SomeFramework.framework/Modules/module.modulemap
Index: clang/test/VFS/module-import.m
===
--- clang/test/VFS/module-import.m

[PATCH] D69958: Improve VFS compatibility on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth marked an inline comment as done.
amccarth added a comment.

Thanks for the review.

In D69958#1744861 , @vsapsai wrote:

> There is one issue with a comment, apart of that everything looks good to me. 
> Looked into replacing slash and backslash literals with calls to 
> `llvm::sys::path::get_separator` or `llvm::sys::path::is_separator` but in my 
> attempts failed to improve existing patch.


Yes, I originally started down the path of trying to "normalize" path styles 
(like slash direction), but that always led to collateral damage because we 
literally have situations where a path is a hybrid of both Windows and Posix.  
Rather than writing Windows and Posix versions of the tests, this patch takes 
the approach that choosing a path style is out-of-scope, which I'm told is 
Clang's general strategy.




Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:535
 /// All configuration options are optional.
 ///   'case-sensitive': 
 ///   'use-external-names': 

vsapsai wrote:
> I believe changing default `CaseSensitive` value makes this comment 
> incorrect. Have no strong opinion on making case-insensitive default on 
> Windows because a better requirement is for VFS generators to specify 
> 'case-sensitive' explicitly instead of relying on default values.
Make CaseSensitive system-dependent aligns with UseCanonicalizePaths, so this 
feels pretty consistent.  (Perhaps in a future path, we can fix both of those.) 
 In the mean time, I'll update the comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69958/new/

https://reviews.llvm.org/D69958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69958: Improve VFS compatibility on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth updated this revision to Diff 229144.
amccarth marked an inline comment as done.
amccarth added a comment.

Modified comment per feedback.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69958/new/

https://reviews.llvm.org/D69958

Files:
  clang/test/Index/index-module-with-vfs.m
  clang/test/Modules/double-quotes.m
  clang/test/Modules/framework-public-includes-private.m
  clang/test/VFS/external-names.c
  clang/test/VFS/framework-import.m
  clang/test/VFS/implicit-include.c
  clang/test/VFS/include-mixed-real-and-virtual.c
  clang/test/VFS/include-real-from-virtual.c
  clang/test/VFS/include-virtual-from-real.c
  clang/test/VFS/include.c
  clang/test/VFS/incomplete-umbrella.m
  clang/test/VFS/module-import.m
  clang/test/VFS/real-path-found-first.m
  clang/test/VFS/relative-path.c
  clang/test/VFS/umbrella-framework-import-skipnonexist.m
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1671,9 +1671,7 @@
 
   // Forward the search to the next component in case this is an empty one.
   if (!FromName.empty()) {
-if (CaseSensitive ? !Start->equals(FromName)
-  : !Start->equals_lower(FromName))
-  // failure to match
+if (!pathComponentMatches(*Start, FromName))
   return make_error_code(llvm::errc::no_such_file_or_directory);
 
 ++Start;
@@ -1695,6 +1693,7 @@
 if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
   return Result;
   }
+
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -651,6 +651,17 @@
 return ExternalFSValidWD && IsFallthrough;
   }
 
+  // In a RedirectingFileSystem, keys can be specified in Posix or Windows
+  // style (or even a mixture of both), so this comparison helper allows
+  // slashes (representing a root) to match backslashes (and vice versa).  Note
+  // that, other than the root, patch components should not contain slashes or
+  // backslashes.
+  bool pathComponentMatches(llvm::StringRef lhs, llvm::StringRef rhs) const {
+if ((CaseSensitive ? lhs.equals(rhs) : lhs.equals_lower(rhs)))
+  return true;
+return (lhs == "/" && rhs == "\\") || (lhs == "\\" && rhs == "/");
+  }
+
   /// The root(s) of the virtual file system.
   std::vector> Roots;
 
@@ -674,7 +685,12 @@
   /// Whether to perform case-sensitive comparisons.
   ///
   /// Currently, case-insensitive matching only works correctly with ASCII.
-  bool CaseSensitive = true;
+  bool CaseSensitive =
+#ifdef _WIN32
+  false;
+#else
+  true;
+#endif
 
   /// IsRelativeOverlay marks whether a ExternalContentsPrefixDir path must
   /// be prefixed in every 'external-contents' when reading from YAML files.
Index: clang/test/VFS/umbrella-framework-import-skipnonexist.m
===
--- clang/test/VFS/umbrella-framework-import-skipnonexist.m
+++ clang/test/VFS/umbrella-framework-import-skipnonexist.m
@@ -1,8 +1,5 @@
 // REQUIRES: crash-recovery
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t
 // RUN: mkdir -p %t/vdir %t/outdir %t/cache
 // RUN: cp -R %S/Inputs/Bar.framework %t/outdir/
Index: clang/test/VFS/relative-path.c
===
--- clang/test/VFS/relative-path.c
+++ clang/test/VFS/relative-path.c
@@ -3,9 +3,6 @@
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -I . -ivfsoverlay %t.yaml -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 #include "not_real.h"
 
 void foo() {
Index: clang/test/VFS/real-path-found-first.m
===
--- clang/test/VFS/real-path-found-first.m
+++ clang/test/VFS/real-path-found-first.m
@@ -4,9 +4,6 @@
 // intentionally rebuild modules, since the precompiled module file refers to
 // the dependency files by real path.
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t %t-cache %t.pch
 // RUN: mkdir -p %t/SomeFramework.framework/Modules
 // RUN: cat %S/Inputs/some_frame_module.map > %t/SomeFramework.framework/Modules/module.modulemap
Index: clang/test/VFS/module-import.m
===
--- clang/test/VFS/module-import.m
+++ clang/test/VFS/module-import.m
@@ -2,9 +2,6 @@
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t 

[PATCH] D69958: Improve VFS compatibility on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth marked 3 inline comments as done.
amccarth added a comment.

Friendly ping for any VFS experts to comment.




Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:657
+  // slashes to match backslashes (and vice versa).
+  bool pathComponentMatches(llvm::StringRef lhs, llvm::StringRef rhs) const {
+if ((CaseSensitive ? lhs.equals(rhs) : lhs.equals_lower(rhs)))

rnk wrote:
> IIUC, this method receives path components, which must not contain path 
> separators, or the root path component, `\` or `/`. Is there some way to 
> express that invariant? Maybe just comment like "A path component must not 
> contain path separators, unless it is the root component, which is 
> represented as a single path separator," or an assert with the same effect.
I felt the assumption is implicit in "path component," but I've made the 
comment more explicit.  Note that the slash/backslash checks are checking the 
entire string.  I hope that reinforces the idea that these would be path roots.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1666
   // paths become globally default.
   if (Start->equals("."))
 ++Start;

rnk wrote:
> Unrelated, but I wonder if this needs to be `while` instead of `if` to handle 
> repeated `foo/./././bar`.
I'm fairly certain I'll be looking into this further as I try to resolve the 
remaining VFS tests that don't run on Windows.  I'd love to eliminate another 
#ifdef, but I want to keep this patch focused.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69958/new/

https://reviews.llvm.org/D69958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69969: [SEH] Defer checking filter expression types until instantiaton

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69969/new/

https://reviews.llvm.org/D69969



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69958: Improve VFS compatibility on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth created this revision.
amccarth added reviewers: rnk, vsapsai, arphaman, Bigcheese.
Herald added subscribers: dexonsmith, hiraditya.
Herald added a project: LLVM.

Keys in a virtual file system can be in Posix or Windows form or even a 
combination of the two.  Many VFS tests (and a few Clang tests) were XFAILed on 
Windows because of false negatives when comparing paths.

First, we default CaseSenstive to false on Windows.  This allows drive letters 
like "D:" to match "d:".  Windows filesystems are, by default, case 
insensitive, so this makes sense even beyond the drive letter.

Second, we allow slashes to match backslashes when they're used as the root 
component of a path.

Both of these changes are limited to RedirectingFileSystems, so there's little 
chance of affecting other path handling.

These changes allow eleven of the VFS tests to pass on Windows as well as three 
other Clang tests, so they have re-enabled (partially addressing PR43272).


https://reviews.llvm.org/D69958

Files:
  clang/test/Index/index-module-with-vfs.m
  clang/test/Modules/double-quotes.m
  clang/test/Modules/framework-public-includes-private.m
  clang/test/VFS/external-names.c
  clang/test/VFS/framework-import.m
  clang/test/VFS/implicit-include.c
  clang/test/VFS/include-mixed-real-and-virtual.c
  clang/test/VFS/include-real-from-virtual.c
  clang/test/VFS/include-virtual-from-real.c
  clang/test/VFS/include.c
  clang/test/VFS/incomplete-umbrella.m
  clang/test/VFS/module-import.m
  clang/test/VFS/real-path-found-first.m
  clang/test/VFS/relative-path.c
  clang/test/VFS/umbrella-framework-import-skipnonexist.m
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1671,9 +1671,7 @@
 
   // Forward the search to the next component in case this is an empty one.
   if (!FromName.empty()) {
-if (CaseSensitive ? !Start->equals(FromName)
-  : !Start->equals_lower(FromName))
-  // failure to match
+if (!pathComponentMatches(*Start, FromName))
   return make_error_code(llvm::errc::no_such_file_or_directory);
 
 ++Start;
@@ -1695,6 +1693,7 @@
 if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
   return Result;
   }
+
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -651,6 +651,15 @@
 return ExternalFSValidWD && IsFallthrough;
   }
 
+  // In a RedirectingFileSystem, keys can be specified in Posix or Windows
+  // style (or even a mixture of both), so this comparison helper allows
+  // slashes to match backslashes (and vice versa).
+  bool pathComponentMatches(llvm::StringRef lhs, llvm::StringRef rhs) const {
+if ((CaseSensitive ? lhs.equals(rhs) : lhs.equals_lower(rhs)))
+  return true;
+return (lhs == "/" && rhs == "\\") || (lhs == "\\" && rhs == "/");
+  }
+
   /// The root(s) of the virtual file system.
   std::vector> Roots;
 
@@ -674,7 +683,12 @@
   /// Whether to perform case-sensitive comparisons.
   ///
   /// Currently, case-insensitive matching only works correctly with ASCII.
-  bool CaseSensitive = true;
+  bool CaseSensitive =
+#ifdef _WIN32
+  false;
+#else
+  true;
+#endif
 
   /// IsRelativeOverlay marks whether a ExternalContentsPrefixDir path must
   /// be prefixed in every 'external-contents' when reading from YAML files.
Index: clang/test/VFS/umbrella-framework-import-skipnonexist.m
===
--- clang/test/VFS/umbrella-framework-import-skipnonexist.m
+++ clang/test/VFS/umbrella-framework-import-skipnonexist.m
@@ -1,8 +1,5 @@
 // REQUIRES: crash-recovery
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t
 // RUN: mkdir -p %t/vdir %t/outdir %t/cache
 // RUN: cp -R %S/Inputs/Bar.framework %t/outdir/
Index: clang/test/VFS/relative-path.c
===
--- clang/test/VFS/relative-path.c
+++ clang/test/VFS/relative-path.c
@@ -3,9 +3,6 @@
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -I . -ivfsoverlay %t.yaml -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 #include "not_real.h"
 
 void foo() {
Index: clang/test/VFS/real-path-found-first.m
===
--- clang/test/VFS/real-path-found-first.m
+++ clang/test/VFS/real-path-found-first.m
@@ -4,9 +4,6 @@
 // intentionally rebuild modules, since the precompiled module file refers to
 // the dependency files by 

[PATCH] D68953: Enable most VFS tests on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth abandoned this revision.
amccarth added a comment.

I create a new review thread for my improved patch shortly.  These changes were 
too wide-ranging.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68953/new/

https://reviews.llvm.org/D68953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68953: Enable most VFS tests on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth marked 2 inline comments as done.
amccarth added a comment.

Somehow Phabricator failed to notify me that you'd already left comments.  I 
even searched my inbox to see if I'd just missed the notification.  Nope.  
Nothing.  I came here today to ping you for the review and discovered you'd 
already done your part.

Anyway, I'll update the patch and ping again when that's done.

Thanks.




Comment at: clang/lib/Lex/HeaderSearch.cpp:782-783
   // Concatenate the requested file onto the directory.
-  // FIXME: Portability.  Filename concatenation should be in sys::Path.
-  TmpDir = IncluderAndDir.second->getName();
-  TmpDir.push_back('/');
-  TmpDir.append(Filename.begin(), Filename.end());
+  TmpPath = IncluderAndDir.second->getName();
+  llvm::sys::path::append(TmpPath, Filename);
 

rnk wrote:
> I'm surprised this just works. :) You ran the whole clang test suite, and 
> nothing broke, right? I would've expected something to accidentally depend on 
> this behavior.
Yeah, I recall it all working before, but, double-checking today, I'm seeing 
some collateral damage.  Investigating.



Comment at: clang/test/VFS/external-names.c:4-5
 
 // FIXME: PR43272
 // XFAIL: system-windows
 

rnk wrote:
> You made changes to this file, but it's still XFAILed. Is that intentional?
The change removes one obstacle, but the test still fails on Windows because 
there's still an outstanding bug.  If you like, I can hoist this change out 
until I have a patch to fix the outstanding issue.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1839
   SmallVector Components;
-  Components.push_back("/");
+  Components.push_back(sys::path::get_separator());
   getVFSEntries(*RootE, Components, CollectedEntries);

rnk wrote:
> I think this will ultimately produce paths that look like: `\C:\foo\bar\baz`. 
> Ultimately, we loop over the components above and repeatedly call 
> sys::path::append on them.
> 
> Clang only uses this function for collecting module dependency info, it 
> seems. Maybe that's what the remaining failures are about?
I didn't encounter anything like `\C:\foo`.  Perhaps the (enabled) VFS tests 
all prime the roots so that never happens here.  I'll see if I can add a test 
to uncover that problem.

And, yes, use of this function seems limited to reading VFS paths from the 
YAML, so its scope is pretty limited.  At least some of the remaining problems 
have to do with the fact that, while the target paths can always be expressed 
in the host style, the key paths can be in either Posix or Windows format, 
which leads to confusion when you try to get answers from functions like 
`sys::path::is_absolute` where you have to pick the right style to get the 
right answer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68953/new/

https://reviews.llvm.org/D68953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68953: Enable most VFS tests on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth created this revision.
amccarth added a reviewer: rnk.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

Inconsistencies in when/if paths are cannoicalized prevented VFS tests from 
working on Windows.  This fixes the primary cause and enables most of the tests 
(15/20).

Remaining VFS tests are still XFAILed on Windows because there are additional 
causes in some code paths.  I plan to diagnose and correct those in a future 
patch.


https://reviews.llvm.org/D68953

Files:
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/VFS/external-names.c
  clang/test/VFS/framework-import.m
  clang/test/VFS/implicit-include.c
  clang/test/VFS/include-mixed-real-and-virtual.c
  clang/test/VFS/include-real-from-virtual.c
  clang/test/VFS/include-virtual-from-real.c
  clang/test/VFS/include.c
  clang/test/VFS/incomplete-umbrella.m
  clang/test/VFS/module-import.m
  clang/test/VFS/real-path-found-first.m
  clang/test/VFS/relative-path.c
  clang/test/VFS/umbrella-framework-import-skipnonexist.m
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1303,16 +1303,15 @@
   return nullptr;
 
 NameValueNode = I.getValue();
+SmallString<256> Path(Value);
 if (FS->UseCanonicalizedPaths) {
-  SmallString<256> Path(Value);
   // Guarantee that old YAML files containing paths with ".." and "."
   // are properly canonicalized before read into the VFS.
   Path = sys::path::remove_leading_dotslash(Path);
   sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  Name = Path.str();
-} else {
-  Name = Value;
 }
+sys::path::native(Path);
+Name = Path.str();
   } else if (Key == "type") {
 if (!parseScalarString(I.getValue(), Value, Buffer))
   return nullptr;
@@ -1371,6 +1370,7 @@
   FullPath = sys::path::remove_leading_dotslash(FullPath);
   sys::path::remove_dots(FullPath, /*remove_dot_dot=*/true);
 }
+sys::path::native(FullPath);
 ExternalContentsPath = FullPath.str();
   } else if (Key == "use-external-name") {
 bool Val;
@@ -1831,11 +1831,12 @@
   RedirectingFileSystem *VFS = RedirectingFileSystem::create(
   std::move(Buffer), DiagHandler, YAMLFilePath, DiagContext,
   std::move(ExternalFS));
-  ErrorOr RootE = VFS->lookupPath("/");
+  ErrorOr RootE =
+  VFS->lookupPath(sys::path::get_separator());
   if (!RootE)
 return;
   SmallVector Components;
-  Components.push_back("/");
+  Components.push_back(sys::path::get_separator());
   getVFSEntries(*RootE, Components, CollectedEntries);
 }
 
Index: clang/test/VFS/umbrella-framework-import-skipnonexist.m
===
--- clang/test/VFS/umbrella-framework-import-skipnonexist.m
+++ clang/test/VFS/umbrella-framework-import-skipnonexist.m
@@ -1,8 +1,5 @@
 // REQUIRES: crash-recovery
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t
 // RUN: mkdir -p %t/vdir %t/outdir %t/cache
 // RUN: cp -R %S/Inputs/Bar.framework %t/outdir/
Index: clang/test/VFS/relative-path.c
===
--- clang/test/VFS/relative-path.c
+++ clang/test/VFS/relative-path.c
@@ -3,9 +3,6 @@
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -I . -ivfsoverlay %t.yaml -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 #include "not_real.h"
 
 void foo() {
Index: clang/test/VFS/real-path-found-first.m
===
--- clang/test/VFS/real-path-found-first.m
+++ clang/test/VFS/real-path-found-first.m
@@ -4,9 +4,6 @@
 // intentionally rebuild modules, since the precompiled module file refers to
 // the dependency files by real path.
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t %t-cache %t.pch
 // RUN: mkdir -p %t/SomeFramework.framework/Modules
 // RUN: cat %S/Inputs/some_frame_module.map > %t/SomeFramework.framework/Modules/module.modulemap
Index: clang/test/VFS/module-import.m
===
--- clang/test/VFS/module-import.m
+++ clang/test/VFS/module-import.m
@@ -2,9 +2,6 @@
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 @import not_real;
 
 void foo() {
Index: clang/test/VFS/incomplete-umbrella.m
===
--- clang/test/VFS/incomplete-umbrella.m
+++ 

[PATCH] D68736: [MSVC] Automatically add atlmfc include and lib directories as system paths.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

> This matches the behavior of cl.

Are you sure?  In a bare environment, cl.exe doesn't include any system paths, 
not even to the standard library.  It actually uses the INCLUDE environment 
variable for those paths.  Granted, VCVARSALL sets that (and other environment 
variables), but it's not innate behavior of cl.exe.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68736/new/

https://reviews.llvm.org/D68736



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67454: Start porting ivfsoverlay tests to Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67454/new/

https://reviews.llvm.org/D67454



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60094: [MSVC] If unable to find link.exe relative to MSVC, look for link.exe in the path

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added inline comments.



Comment at: lib/Driver/ToolChains/MSVC.cpp:493
   C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+  linkPath = TC.GetProgramPath("link.exe");
+}

The comment above explains one reason why we shouldn't use link.exe on the path.

If it is an appropriate fallback, modify the comment or add another one here 
explaining why this is better than failing.  I think you hit on it in the patch 
summary, but it should be captured in the code.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60094/new/

https://reviews.llvm.org/D60094



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47578: Do not enforce absolute path argv0 in windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

In https://reviews.llvm.org/D47578#1117874, @rnk wrote:

> The LLDB test suite isn't in very good shape on Windows. It is complicated to 
> set up and build, I don't want to block this fix on @takuto.ikuta setting up 
> that build environment. This is a Windows-only change, and I believe it makes 
> it more consistent with Linux, so as long as check-llvm, check-clang, and 
> check-lld pass, this should be relatively safe.


OK, I'll take rnk's word for that.


https://reviews.llvm.org/D47578



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47578: Do not enforce absolute path argv0 in windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

I was under the impression that some tools rely on the fact that arg[0] is 
always expanded to an absolute path.  Does this work with lldb and its test 
suite?


https://reviews.llvm.org/D47578



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

In https://reviews.llvm.org/D43621#1017027, @ruiu wrote:

> > That's weird, because lots of lldb tests compile and link test binaries on 
> > Windows with `-fuse-ld=lld` (without the `.exe`).  What makes you say the 
> > `.exe` is necessary?
>
> Maybe he is using clang (not clang-cl) and want to use ld.lld.exe instead of 
> lld-link?


The LLDB tests use clang, not clang-cl.  Here's a typical invocation:

  D:\src\llvm\build\ninja_release\bin\clang.exe  main.o -g -O0 -fno-builtin 
-m32  
-ID:\src\llvm\mono\llvm-project\lldb\packages\Python\lldbsuite\test\lang\c\conflicting-symbol/../../../make/../../../../../include
 
-ID:\src\llvm\mono\llvm-project\lldb\packages\Python\lldbsuite\test\lang\c\conflicting-symbol/
 -include 
D:\src\llvm\mono\llvm-project\lldb\packages\Python\lldbsuite\test\lang\c\conflicting-symbol/../../../make/test_common.h
 
-ID:\src\llvm\mono\llvm-project\lldb\packages\Python\lldbsuite\test\lang\c\conflicting-symbol/../../../make/
  -fno-limit-debug-info   -L. -LOne -lOne -LTwo -lTwo  -fuse-ld=lld 
--driver-mode=g++  -o "a.out"

Note that it's running `clang.exe` and passing `-fuse-ld=lld`.


Repository:
  rC Clang

https://reviews.llvm.org/D43621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

> Right now, we have to add an .exe suffix when using this switch, like 
> -fuse-ld=lld.exe.

That's weird, because lots of lldb tests compile and link test binaries on 
Windows with `-fuse-ld=lld` (without the `.exe`).  What makes you say the 
`.exe` is necessary?


Repository:
  rC Clang

https://reviews.llvm.org/D43621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

This must be new-ish code, since I've routinely used `-fuse-ld=lld` (without 
`.exe`) on Windows.


Repository:
  rC Clang

https://reviews.llvm.org/D43621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added inline comments.



Comment at: lib/Rewrite/Rewriter.cpp:455
   I->second.write(File.getStream());
+  prewrite(I->first);
 }

I'm not familiar with the structure of this code, but it seems odd to me that 
the callback is called `prewrite` when it happens _after_ the write.


https://reviews.llvm.org/D30385



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29198: clang-cl: Warn about /U flags that look like filenames (PR31662)

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added inline comments.



Comment at: include/clang/Basic/DiagnosticDriverKinds.td:252
+  InGroup>;
+def note_use_dashdash : Note<"Use '--' to treat subsequent arguments as 
filenames">;
+

For Windows, wouldn't it make more sense to suggest using valid paths (with 
backslashes instead of slashes)?

What about all the other characters paths can begin with?  Shouldn't we also 
warn that /code/myproject will also be interpreted as /c rather than 
\code\myproject?


https://reviews.llvm.org/D29198



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D20136: Get default -fms-compatibility-version from cl.exe's version

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

> and folks have to manually add mincore.lib to their link.

I could load the library dynamically on demand and use GetProcAddress, but I 
suspect that would further degrade the performance.  I could probably write my 
own code to find the version in the binary, but I doubt that crosses the 
reward/work threshold.


Repository:
  rL LLVM

https://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28365: [Driver] Updated for Visual Studio 2017

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

(I could be missing something because the diff doesn't have much context.)

If I'm reading this right, it looks like it will no longer search the PATH in 
order to find cl.exe.  If that's the case, then I'm mildly worried about this 
change in behavior.  I know I have multiple versions of VS installed, and 
commonly select one just by moving its bin directory to the head of PATH.  The 
old behavior would find that one (which is used for things like defaulting 
-fms-compatibility-version) while the new behavior will find the newest one 
installed on the machine.

That's not necessarily a deal breaker, but it's an important change for Windows 
developers to be aware of.


https://reviews.llvm.org/D28365



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits