[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-12-04 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3ee277b86b34: [Support] add vfs support for 
ExpandResponseFiles (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -29,6 +29,7 @@
 #include "llvm/Config/config.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
@@ -37,6 +38,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1043,14 +1045,18 @@
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
-static bool ExpandResponseFile(StringRef FName, StringSaver ,
+static llvm::Error ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
-  ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
+  llvm::ErrorOr CurrDirOrErr = FS.getCurrentWorkingDirectory();
+  if (!CurrDirOrErr)
+return llvm::errorCodeToError(CurrDirOrErr.getError());
+  llvm::ErrorOr> MemBufOrErr =
+  FS.getBufferForFile(FName);
   if (!MemBufOrErr)
-return false;
+return llvm::errorCodeToError(MemBufOrErr.getError());
   MemoryBuffer  = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
@@ -1059,7 +1065,8 @@
   std::string UTF8Buf;
   if (hasUTF16ByteOrderMark(BufRef)) {
 if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
-  return false;
+  return llvm::createStringError(std::errc::illegal_byte_sequence,
+ "Could not convert UTF16 to UTF8");
 Str = StringRef(UTF8Buf);
   }
   // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
@@ -1084,9 +1091,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDirOrErr.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1095,14 +1100,14 @@
 }
   }
 
-  return true;
+  return Error::success();
 }
 
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1144,20 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  llvm::ErrorOr LHS = FS.status(FName);
+  if (!LHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(LHS.getError()));
+return false;
+  }
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!RHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(RHS.getError()));
+return false;
+  }
+  return LHS->equivalent(*RHS);
 };
 
 // Check for recursive response files.
@@ -1155,10 +1172,13 @@
 // Replace this response file argument with the tokenization of its
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
-if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+if (llvm::Error Err =
+ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
+   RelativeNames, FS)) {
   // We couldn't read this file, so we

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-29 Thread James Henderson via Phabricator via cfe-commits 
jhenderson added a comment.

In D70769#1763703 , @kadircet wrote:

> @jhenderson I am planning to commit this if the discussion around `std::errc` 
> vs `llvm::errc` is resolved, I don't have any preference towards one or the 
> other both seems to get the work done.


I'm not going to block this being committed. I do think llvm::errc exists for a 
reason, and those reasons are documented in the associated file, giving me the 
impression we should use it and not std::errc. However, I don't know how 
applicable those reasons really are nowadays - I'm certainly no authority on 
them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-29 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet added a comment.

@jhenderson I am planning to commit this if the discussion around `std::errc` 
vs `llvm::errc` is resolved, I don't have any preference towards one or the 
other both seems to get the work done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-29 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231510.
lh123 marked an inline comment as done.
lh123 added a comment.

fix typo:
"Could not convert UTF16 To UTF8" -> "Could not convert UTF16 to UTF8"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -29,6 +29,7 @@
 #include "llvm/Config/config.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
@@ -37,6 +38,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1043,14 +1045,18 @@
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
-static bool ExpandResponseFile(StringRef FName, StringSaver ,
+static llvm::Error ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
-  ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
+  llvm::ErrorOr CurrDirOrErr = FS.getCurrentWorkingDirectory();
+  if (!CurrDirOrErr)
+return llvm::errorCodeToError(CurrDirOrErr.getError());
+  llvm::ErrorOr> MemBufOrErr =
+  FS.getBufferForFile(FName);
   if (!MemBufOrErr)
-return false;
+return llvm::errorCodeToError(MemBufOrErr.getError());
   MemoryBuffer  = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
@@ -1059,7 +1065,8 @@
   std::string UTF8Buf;
   if (hasUTF16ByteOrderMark(BufRef)) {
 if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
-  return false;
+  return llvm::createStringError(std::errc::illegal_byte_sequence,
+ "Could not convert UTF16 to UTF8");
 Str = StringRef(UTF8Buf);
   }
   // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
@@ -1084,9 +1091,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDirOrErr.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1095,14 +1100,14 @@
 }
   }
 
-  return true;
+  return Error::success();
 }
 
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1144,20 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  llvm::ErrorOr LHS = FS.status(FName);
+  if (!LHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(LHS.getError()));
+return false;
+  }
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!RHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(RHS.getError()));
+return false;
+  }
+  return LHS->equivalent(*RHS);
 };
 
 // Check for recursive response files.
@@ -1155,10 +1172,13 @@
 // Replace this response file argument with the tokenization of its
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
-if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+if (llvm::Error Err =
+ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
+   RelativeNames, FS)) {
   // We couldn't read this file,

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-29 Thread liu hui via Phabricator via cfe-commits 
lh123 marked 3 inline comments as done.
lh123 added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1069
+  return llvm::createStringError(
+  std::make_error_code(std::errc::illegal_byte_sequence),
+  "Could not convert UTF16 To UTF8");

jhenderson wrote:
> lh123 wrote:
> > jhenderson wrote:
> > > jhenderson wrote:
> > > > `std::make_error_code(std::errc::illegal_byte_sequence)` -> 
> > > > `errc::illegal_byte_sequence`
> > > LLVM has its own errc error_code set. Please use that, i.e. delete the 
> > > `std::` (note that I didn't add `std::` in my previous comment).
> > I think we should use `std::errc::illegal_byte_sequence`, because it's 
> > sigature is:
> >  ```
> > template 
> > inline Error createStringError(std::errc EC, char const *Fmt, const Ts &... 
> > Vals)
> > ```
> I'm not sure if that's a mistake in the function interface or not, and it 
> looks like our usage is very inconsistent, but the comments in Errc.h imply 
> that we really should use llvm::errc values and not std::errc, or there may 
> be problems.
I think `llvm::errc` and `std::errc` are both correct, `llvm::errc` eventually 
implicitly converts to `std::error_code`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread James Henderson via Phabricator via cfe-commits 
jhenderson added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1069
+  return llvm::createStringError(
+  std::make_error_code(std::errc::illegal_byte_sequence),
+  "Could not convert UTF16 To UTF8");

lh123 wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > `std::make_error_code(std::errc::illegal_byte_sequence)` -> 
> > > `errc::illegal_byte_sequence`
> > LLVM has its own errc error_code set. Please use that, i.e. delete the 
> > `std::` (note that I didn't add `std::` in my previous comment).
> I think we should use `std::errc::illegal_byte_sequence`, because it's 
> sigature is:
>  ```
> template 
> inline Error createStringError(std::errc EC, char const *Fmt, const Ts &... 
> Vals)
> ```
I'm not sure if that's a mistake in the function interface or not, and it looks 
like our usage is very inconsistent, but the comments in Errc.h imply that we 
really should use llvm::errc values and not std::errc, or there may be problems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread liu hui via Phabricator via cfe-commits 
lh123 marked an inline comment as done.
lh123 added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1069
+  return llvm::createStringError(
+  std::make_error_code(std::errc::illegal_byte_sequence),
+  "Could not convert UTF16 To UTF8");

jhenderson wrote:
> jhenderson wrote:
> > `std::make_error_code(std::errc::illegal_byte_sequence)` -> 
> > `errc::illegal_byte_sequence`
> LLVM has its own errc error_code set. Please use that, i.e. delete the 
> `std::` (note that I didn't add `std::` in my previous comment).
I think we should use `std::errc::illegal_byte_sequence`, because it's sigature 
is:
 ```
template 
inline Error createStringError(std::errc EC, char const *Fmt, const Ts &... 
Vals)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread James Henderson via Phabricator via cfe-commits 
jhenderson added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1069
+  return llvm::createStringError(
+  std::make_error_code(std::errc::illegal_byte_sequence),
+  "Could not convert UTF16 To UTF8");

jhenderson wrote:
> `std::make_error_code(std::errc::illegal_byte_sequence)` -> 
> `errc::illegal_byte_sequence`
LLVM has its own errc error_code set. Please use that, i.e. delete the `std::` 
(note that I didn't add `std::` in my previous comment).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231395.
lh123 marked an inline comment as done.
lh123 added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -29,6 +29,7 @@
 #include "llvm/Config/config.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
@@ -37,6 +38,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1043,14 +1045,18 @@
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
-static bool ExpandResponseFile(StringRef FName, StringSaver ,
+static llvm::Error ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
-  ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
+  llvm::ErrorOr CurrDirOrErr = FS.getCurrentWorkingDirectory();
+  if (!CurrDirOrErr)
+return llvm::errorCodeToError(CurrDirOrErr.getError());
+  llvm::ErrorOr> MemBufOrErr =
+  FS.getBufferForFile(FName);
   if (!MemBufOrErr)
-return false;
+return llvm::errorCodeToError(MemBufOrErr.getError());
   MemoryBuffer  = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
@@ -1059,7 +1065,8 @@
   std::string UTF8Buf;
   if (hasUTF16ByteOrderMark(BufRef)) {
 if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
-  return false;
+  return llvm::createStringError(std::errc::illegal_byte_sequence,
+ "Could not convert UTF16 To UTF8");
 Str = StringRef(UTF8Buf);
   }
   // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
@@ -1084,9 +1091,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDirOrErr.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1095,14 +1100,14 @@
 }
   }
 
-  return true;
+  return Error::success();
 }
 
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1144,20 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  llvm::ErrorOr LHS = FS.status(FName);
+  if (!LHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(LHS.getError()));
+return false;
+  }
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!RHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(RHS.getError()));
+return false;
+  }
+  return LHS->equivalent(*RHS);
 };
 
 // Check for recursive response files.
@@ -1155,10 +1172,13 @@
 // Replace this response file argument with the tokenization of its
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
-if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+if (llvm::Error Err =
+ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
+   RelativeNames, FS)) {
   // We couldn't read this file, so we leave it in the argument stream and
   // move on.
+

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread James Henderson via Phabricator via cfe-commits 
jhenderson added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1069
+  return llvm::createStringError(
+  std::make_error_code(std::errc::illegal_byte_sequence),
+  "Could not convert UTF16 To UTF8");

`std::make_error_code(std::errc::illegal_byte_sequence)` -> 
`errc::illegal_byte_sequence`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231390.
lh123 marked 6 inline comments as done.
lh123 added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -29,6 +29,7 @@
 #include "llvm/Config/config.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
@@ -37,6 +38,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1043,14 +1045,18 @@
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
-static bool ExpandResponseFile(StringRef FName, StringSaver ,
+static llvm::Error ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
-  ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
+  llvm::ErrorOr CurrDirOrErr = FS.getCurrentWorkingDirectory();
+  if (!CurrDirOrErr)
+return llvm::errorCodeToError(CurrDirOrErr.getError());
+  llvm::ErrorOr> MemBufOrErr =
+  FS.getBufferForFile(FName);
   if (!MemBufOrErr)
-return false;
+return llvm::errorCodeToError(MemBufOrErr.getError());
   MemoryBuffer  = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
@@ -1059,7 +1065,9 @@
   std::string UTF8Buf;
   if (hasUTF16ByteOrderMark(BufRef)) {
 if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
-  return false;
+  return llvm::createStringError(
+  std::make_error_code(std::errc::illegal_byte_sequence),
+  "Could not convert UTF16 To UTF8");
 Str = StringRef(UTF8Buf);
   }
   // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
@@ -1084,9 +1092,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDirOrErr.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1095,14 +1101,14 @@
 }
   }
 
-  return true;
+  return Error::success();
 }
 
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1145,20 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  llvm::ErrorOr LHS = FS.status(FName);
+  if (!LHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(LHS.getError()));
+return false;
+  }
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!RHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(RHS.getError()));
+return false;
+  }
+  return LHS->equivalent(*RHS);
 };
 
 // Check for recursive response files.
@@ -1155,10 +1173,13 @@
 // Replace this response file argument with the tokenization of its
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
-if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+if (llvm::Error Err =
+ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
+   RelativeNames, FS)) {
   // We couldn't read this file, so we leave it in the argument stream and
   //

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread James Henderson via Phabricator via cfe-commits 
jhenderson added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1071
+  return llvm::make_error(
+  "Could not convert UTF16 To UTF8", llvm::inconvertibleErrorCode());
 Str = StringRef(UTF8Buf);

Not sure, but you might want to use `illegal_byte_sequence` here instead of 
`llvm::inconvertibleErrorCode()`.



Comment at: llvm/lib/Support/CommandLine.cpp:1178
+
+if (llvm::Error Err = ExpandResponseFile(FName, Saver, Tokenizer, 
ExpandedArgv, MarkEOLs,
+RelativeNames, FS)) {

clang-format please


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

LGTM, please make sure you've clang-formatted the changes though




Comment at: llvm/lib/Support/CommandLine.cpp:1054
+  llvm::ErrorOr CurrDirOrErr = FS.getCurrentWorkingDirectory();
+  if (!CurrDirOrErr) {
+return llvm::errorCodeToError(CurrDirOrErr.getError());

nit: no need for braces



Comment at: llvm/lib/Support/CommandLine.cpp:1059
+  FS.getBufferForFile(FName);
+  if (!MemBufOrErr) {
+return llvm::errorCodeToError(MemBufOrErr.getError());

nit: no need for braces



Comment at: llvm/lib/Support/CommandLine.cpp:1178
+
+if (llvm::Error Err = ExpandResponseFile(FName, Saver, Tokenizer, 
ExpandedArgv, MarkEOLs,
+RelativeNames, FS)) {

clang-format ?



Comment at: llvm/lib/Support/CommandLine.cpp:1070
 if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
-  return false;
+  return llvm::make_error(
+  "Could not convert UTF16 To UTF8", llvm::inconvertibleErrorCode());

nit: you can make use of `llvm::createStringError` instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231384.
lh123 added a comment.

fixes no matching constructor for initialization of `llvm::StringError`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -29,6 +29,7 @@
 #include "llvm/Config/config.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
@@ -37,6 +38,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1043,14 +1045,20 @@
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
-static bool ExpandResponseFile(StringRef FName, StringSaver ,
+static llvm::Error ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
-  ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
-  if (!MemBufOrErr)
-return false;
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
+  llvm::ErrorOr CurrDirOrErr = FS.getCurrentWorkingDirectory();
+  if (!CurrDirOrErr) {
+return llvm::errorCodeToError(CurrDirOrErr.getError());
+  }
+  llvm::ErrorOr> MemBufOrErr =
+  FS.getBufferForFile(FName);
+  if (!MemBufOrErr) {
+return llvm::errorCodeToError(MemBufOrErr.getError());
+  }
   MemoryBuffer  = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
@@ -1059,7 +1067,8 @@
   std::string UTF8Buf;
   if (hasUTF16ByteOrderMark(BufRef)) {
 if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
-  return false;
+  return llvm::make_error(
+  "Could not convert UTF16 To UTF8", llvm::inconvertibleErrorCode());
 Str = StringRef(UTF8Buf);
   }
   // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
@@ -1084,9 +1093,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDirOrErr.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1095,14 +1102,14 @@
 }
   }
 
-  return true;
+  return Error::success();
 }
 
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1146,20 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  llvm::ErrorOr LHS = FS.status(FName);
+  if (!LHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(LHS.getError()));
+return false;
+  }
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!RHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(RHS.getError()));
+return false;
+  }
+  return LHS->equivalent(*RHS);
 };
 
 // Check for recursive response files.
@@ -1155,10 +1174,13 @@
 // Replace this response file argument with the tokenization of its
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
-if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+
+if (llvm::Error Err = ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
+RelativeNames, FS)) {
   // We couldn't read this file, so we leave it in the argument stream and
   // move on.

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: FAILURE - 
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231381.
lh123 added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -29,6 +29,7 @@
 #include "llvm/Config/config.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
@@ -37,6 +38,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1043,14 +1045,20 @@
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
-static bool ExpandResponseFile(StringRef FName, StringSaver ,
+static llvm::Error ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
-  ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
-  if (!MemBufOrErr)
-return false;
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
+  llvm::ErrorOr CurrDirOrErr = FS.getCurrentWorkingDirectory();
+  if (!CurrDirOrErr) {
+return llvm::errorCodeToError(CurrDirOrErr.getError());
+  }
+  llvm::ErrorOr> MemBufOrErr =
+  FS.getBufferForFile(FName);
+  if (!MemBufOrErr) {
+return llvm::errorCodeToError(MemBufOrErr.getError());
+  }
   MemoryBuffer  = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
@@ -1059,7 +1067,8 @@
   std::string UTF8Buf;
   if (hasUTF16ByteOrderMark(BufRef)) {
 if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
-  return false;
+  return llvm::make_error(
+  "Could not convert UTF16 To UTF8");
 Str = StringRef(UTF8Buf);
   }
   // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
@@ -1084,9 +1093,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDirOrErr.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1095,14 +1102,14 @@
 }
   }
 
-  return true;
+  return Error::success();
 }
 
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1146,20 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  llvm::ErrorOr LHS = FS.status(FName);
+  if (!LHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(LHS.getError()));
+return false;
+  }
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!RHS) {
+// TODO: The error should be propagated up the stack.
+llvm::consumeError(llvm::errorCodeToError(RHS.getError()));
+return false;
+  }
+  return LHS->equivalent(*RHS);
 };
 
 // Check for recursive response files.
@@ -1155,10 +1174,13 @@
 // Replace this response file argument with the tokenization of its
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
-if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+
+if (llvm::Error Err = ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
+RelativeNames, FS)) {
   // We couldn't read this file, so we leave it in the argument stream and
   // move on.
+  // TODO: The error should be propagated up the stack.
+

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: FAILURE - 
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231377.
lh123 added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -29,6 +29,7 @@
 #include "llvm/Config/config.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
@@ -37,6 +38,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1043,14 +1045,20 @@
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
-static bool ExpandResponseFile(StringRef FName, StringSaver ,
+static llvm::Error ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
-  ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
-  if (!MemBufOrErr)
-return false;
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
+  llvm::ErrorOr CurrDirOrErr = FS.getCurrentWorkingDirectory();
+  if (!CurrDirOrErr) {
+return llvm::errorCodeToError(CurrDirOrErr.getError());
+  }
+  llvm::ErrorOr> MemBufOrErr =
+  FS.getBufferForFile(FName);
+  if (!MemBufOrErr) {
+return llvm::errorCodeToError(MemBufOrErr.getError());
+  }
   MemoryBuffer  = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
@@ -1059,7 +1067,8 @@
   std::string UTF8Buf;
   if (hasUTF16ByteOrderMark(BufRef)) {
 if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
-  return false;
+  return llvm::make_error(
+  "Could not convert UTF16 To UTF8");
 Str = StringRef(UTF8Buf);
   }
   // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
@@ -1084,9 +1093,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDirOrErr.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1095,14 +1102,14 @@
 }
   }
 
-  return true;
+  return Error::success();
 }
 
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1146,18 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  llvm::ErrorOr LHS = FS.status(FName);
+  if (!LHS) {
+llvm::consumeError(llvm::errorCodeToError(LHS.getError()));
+return false;
+  }
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!RHS) {
+llvm::consumeError(llvm::errorCodeToError(RHS.getError()));
+return false;
+  }
+  return LHS->equivalent(*RHS);
 };
 
 // Check for recursive response files.
@@ -1155,10 +1172,13 @@
 // Replace this response file argument with the tokenization of its
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
-if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+
+// TODO: The error should be propagated up the stack.
+if (llvm::Error Err = ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
+RelativeNames, FS)) {
   // We couldn't read this file, so we leave it in the argument stream and
   // move on.
+  llvm::consumeError(std::move(Err));
   AllExpanded = false;
   ++I;
   continue;
@@ -1186,9 +1206,13 @@
 
 bool

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread James Henderson via Phabricator via cfe-commits 
jhenderson added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1148
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!LHS || !RHS) {
+return false;

kadircet wrote:
> again you need to `consumeError`s before returning. I would first get `LHS`, 
> consume and bail out if it was an error, then do the same for `RHS` and only 
> after that return `equivalent`.
Could you add a TODO comment here and at the other `consumeError()` call to say 
that the error should be propagated up the stack, please?



Comment at: llvm/lib/Support/CommandLine.cpp:1054
+  if (!CurrDirOrErr) {
+llvm::consumeError(llvm::errorCodeToError(CurrDirOrErr.getError()));
 return false;

I'd ultimately like to see the public interface return 
llvm::Expected/llvm::Error so that a potentially useful error with information 
isn't just thrown away. However, I agree that that's probably a separate 
change. On the other hand, it should be simple to return it here, since this is 
only used locally, for consumption in `ExpandResponseFiles` below. Could you 
make that change, please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231361.
lh123 added a comment.

Sorry, upload the wrong patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -37,6 +37,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1046,11 +1047,19 @@
 static bool ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
-  ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
-  if (!MemBufOrErr)
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
+  llvm::ErrorOr CurrDirOrErr = FS.getCurrentWorkingDirectory();
+  if (!CurrDirOrErr) {
+llvm::consumeError(llvm::errorCodeToError(CurrDirOrErr.getError()));
 return false;
+  }
+  llvm::ErrorOr> MemBufOrErr =
+  FS.getBufferForFile(FName);
+  if (!MemBufOrErr) {
+llvm::consumeError(llvm::errorCodeToError(MemBufOrErr.getError()));
+return false;
+  }
   MemoryBuffer  = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
@@ -1084,9 +1093,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDirOrErr.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1101,8 +1108,8 @@
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1146,18 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  llvm::ErrorOr LHS = FS.status(FName);
+  if (!LHS) {
+llvm::consumeError(llvm::errorCodeToError(LHS.getError()));
+return false;
+  }
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!RHS) {
+llvm::consumeError(llvm::errorCodeToError(RHS.getError()));
+return false;
+  }
+  return LHS->equivalent(*RHS);
 };
 
 // Check for recursive response files.
@@ -1156,7 +1173,7 @@
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
 if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+RelativeNames, FS)) {
   // We couldn't read this file, so we leave it in the argument stream and
   // move on.
   AllExpanded = false;
@@ -1187,7 +1204,8 @@
 bool cl::readConfigFile(StringRef CfgFile, StringSaver ,
 SmallVectorImpl ) {
   if (!ExpandResponseFile(CfgFile, Saver, cl::tokenizeConfigFile, Argv,
-  /*MarkEOLs*/ false, /*RelativeNames*/ true))
+  /*MarkEOLs*/ false, /*RelativeNames*/ true,
+  *llvm::vfs::getRealFileSystem()))
 return false;
   return ExpandResponseFiles(Saver, cl::tokenizeConfigFile, Argv,
  /*MarkEOLs*/ false, /*RelativeNames*/ true);
Index: llvm/include/llvm/Support/CommandLine.h
===
--- llvm/include/llvm/Support/CommandLine.h
+++ llvm/include/llvm/Support/CommandLine.h
@@ -29,6 +29,7 @@
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1964,10 +1965,13 @@
 /// with nullptrs in the Argv vector.
 /// \param [in] RelativeNames true if names of nested response files must

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: FAILURE - 
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-28 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231359.
Herald added a subscriber: mgorny.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  clang/include/clang/Tooling/CompilationDatabase.h
  clang/lib/Tooling/CMakeLists.txt
  clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
  clang/lib/Tooling/JSONCompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -859,5 +859,36 @@
 "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
 }
 
+class ExpandResponseFilesTest : public MemDBTest {
+protected:
+  void SetUp() override {
+FS = new llvm::vfs::InMemoryFileSystem;
+ASSERT_TRUE(addFile(path(StringRef("rsp1.rsp")), "-Dflag"));
+  }
+
+  bool addFile(StringRef File, StringRef Context) {
+return FS->addFile(File, 0, llvm::MemoryBuffer::getMemBufferCopy(Context));
+  }
+
+  std::string getCommand(llvm::StringRef F) {
+auto Results = expandResponseFiles(std::make_unique(Entries),
+   FS)
+   ->getCompileCommands(path(F));
+if (Results.empty()) {
+  return "none";
+}
+return llvm::join(Results[0].CommandLine, " ");
+  }
+
+  llvm::IntrusiveRefCntPtr FS;
+};
+
+TEST_F(ExpandResponseFilesTest, ExpandResponseFiles) {
+  add("foo.cpp", "clang", "@rsp1.rsp");
+  add("bar.cpp", "clang", "-Dflag");
+  EXPECT_EQ(getCommand("foo.cpp"), "clang foo.cpp -D foo.cpp -Dflag");
+  EXPECT_EQ(getCommand("bar.cpp"), "clang bar.cpp -D bar.cpp -Dflag");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -168,7 +168,9 @@
 auto Base = JSONCompilationDatabase::loadFromFile(
 JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
 return Base ? inferTargetAndDriverMode(
-  inferMissingCompileCommands(std::move(Base)))
+  inferMissingCompileCommands(expandResponseFiles(
+  std::move(Base),
+  llvm::vfs::createPhysicalFileSystem().release(
 : nullptr;
   }
 };
Index: clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
===
--- /dev/null
+++ clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -0,0 +1,93 @@
+//===- ExpandResponseFileCompilationDataBase.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+
+class ExpandResponseFilesDatabase : public CompilationDatabase {
+public:
+  ExpandResponseFilesDatabase(
+  std::unique_ptr Base,
+  llvm::cl::TokenizerCallback Tokenizer,
+  llvm::IntrusiveRefCntPtr FS)
+  : Base(std::move(Base)), Tokenizer(Tokenizer), FS(std::move(FS)) {
+assert(this->Base != nullptr);
+assert(this->Tokenizer != nullptr);
+assert(this->FS != nullptr);
+  }
+
+  std::vector getAllFiles() const override {
+return Base->getAllFiles();
+  }
+
+  std::vector
+  getCompileCommands(StringRef FilePath) const override {
+return expand(Base->getCompileCommands(FilePath));
+  }
+
+  std::vector getAllCompileCommands() const override {
+return expand(Base->getAllCompileCommands());
+  }
+
+private:
+  std::vector expand(std::vector Cmds) const {
+for (auto  : Cmds) {
+  // FIXME: we should rather propagate the current directory into
+  // ExpandResponseFiles as well in addition to FS and someone can take a
+  // look at it later on.
+  if (std::error_code EC = FS->setCurrentWorkingDirectory(Cmd.Directory)) {
+llvm::consumeError(llvm::errorCodeToError(EC));
+continue;
+  }
+  bool SeenRSPFile = false;
+  llvm::SmallVector Argv;
+  Argv.reserve(Cmd.CommandLine.size());
+  for (auto  : Cmd.CommandLine) {
+Argv.push_back(Arg.c_str());
+SeenRSPFile |= Arg.front() == '@';
+  }
+  if (!SeenRSPFile)
+

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231353.
lh123 added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -37,6 +37,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1046,11 +1047,19 @@
 static bool ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
-  ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
-  if (!MemBufOrErr)
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
+  llvm::ErrorOr CurrDirOrErr = FS.getCurrentWorkingDirectory();
+  if (!CurrDirOrErr) {
+llvm::consumeError(llvm::errorCodeToError(CurrDirOrErr.getError()));
 return false;
+  }
+  llvm::ErrorOr> MemBufOrErr =
+  FS.getBufferForFile(FName);
+  if (!MemBufOrErr) {
+llvm::consumeError(llvm::errorCodeToError(MemBufOrErr.getError()));
+return false;
+  }
   MemoryBuffer  = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
@@ -1084,9 +1093,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDirOrErr.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1101,8 +1108,8 @@
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1146,18 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  llvm::ErrorOr LHS = FS.status(FName);
+  if (!LHS) {
+llvm::consumeError(llvm::errorCodeToError(LHS.getError()));
+return false;
+  }
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!RHS) {
+llvm::consumeError(llvm::errorCodeToError(RHS.getError()));
+return false;
+  }
+  return LHS->equivalent(*RHS);
 };
 
 // Check for recursive response files.
@@ -1156,7 +1173,7 @@
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
 if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+RelativeNames, FS)) {
   // We couldn't read this file, so we leave it in the argument stream and
   // move on.
   AllExpanded = false;
@@ -1187,7 +1204,8 @@
 bool cl::readConfigFile(StringRef CfgFile, StringSaver ,
 SmallVectorImpl ) {
   if (!ExpandResponseFile(CfgFile, Saver, cl::tokenizeConfigFile, Argv,
-  /*MarkEOLs*/ false, /*RelativeNames*/ true))
+  /*MarkEOLs*/ false, /*RelativeNames*/ true,
+  *llvm::vfs::getRealFileSystem()))
 return false;
   return ExpandResponseFiles(Saver, cl::tokenizeConfigFile, Argv,
  /*MarkEOLs*/ false, /*RelativeNames*/ true);
Index: llvm/include/llvm/Support/CommandLine.h
===
--- llvm/include/llvm/Support/CommandLine.h
+++ llvm/include/llvm/Support/CommandLine.h
@@ -29,6 +29,7 @@
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1964,10 +1965,13 @@
 /// with nullptrs in the Argv vector.
 /// \param [in] RelativeNames true if names of nested response files must be
 ///

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231349.
lh123 marked 3 inline comments as done.
lh123 added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -37,6 +37,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1046,9 +1047,15 @@
 static bool ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
+  llvm::ErrorOr CurrDir = FS.getCurrentWorkingDirectory();
+  if (!CurrDir) {
+llvm::consumeError(errorCodeToError(CurrDir.getError()));
+return false;
+  }
   ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
+  FS.getBufferForFile(FName);
   if (!MemBufOrErr)
 return false;
   MemoryBuffer  = *MemBufOrErr.get();
@@ -1084,9 +1091,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDir.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1101,8 +1106,8 @@
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1144,18 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  llvm::ErrorOr LHS = FS.status(FName);
+  if (!LHS) {
+llvm::consumeError(errorCodeToError(LHS.getError()));
+return false;
+  }
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!RHS) {
+llvm::consumeError(errorCodeToError(RHS.getError()));
+return false;
+  }
+  return LHS->equivalent(*RHS);
 };
 
 // Check for recursive response files.
@@ -1156,7 +1171,7 @@
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
 if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+RelativeNames, FS)) {
   // We couldn't read this file, so we leave it in the argument stream and
   // move on.
   AllExpanded = false;
@@ -1187,7 +1202,8 @@
 bool cl::readConfigFile(StringRef CfgFile, StringSaver ,
 SmallVectorImpl ) {
   if (!ExpandResponseFile(CfgFile, Saver, cl::tokenizeConfigFile, Argv,
-  /*MarkEOLs*/ false, /*RelativeNames*/ true))
+  /*MarkEOLs*/ false, /*RelativeNames*/ true,
+  *llvm::vfs::getRealFileSystem()))
 return false;
   return ExpandResponseFiles(Saver, cl::tokenizeConfigFile, Argv,
  /*MarkEOLs*/ false, /*RelativeNames*/ true);
Index: llvm/include/llvm/Support/CommandLine.h
===
--- llvm/include/llvm/Support/CommandLine.h
+++ llvm/include/llvm/Support/CommandLine.h
@@ -29,6 +29,7 @@
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1964,10 +1965,13 @@
 /// with nullptrs in the Argv vector.
 /// \param [in] RelativeNames true if names of nested response files must be
 /// resolved relative to including file.
+/// \param [in] FS File system used for all file access when running the tool.
 /// \return true if all @files were expanded successfully or there were none.
-bool

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet added inline comments.



Comment at: llvm/lib/Support/CommandLine.cpp:1053
+  llvm::ErrorOr CurrDir = FS.getCurrentWorkingDirectory();
+  if (!CurrDir)
+return false;

this comment has moved into a irrelevant line and wasn't addressed, so 
re-stating it here:

```
it is sad that, ExpandResponseFile returns a bool, but that's a battle for 
another day.

unfortunately, it is not enough return false in this case you need to consume 
the error with llvm::consumeError before exiting the scope.
```



Comment at: llvm/lib/Support/CommandLine.cpp:1148
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!LHS || !RHS) {
+return false;

again you need to `consumeError`s before returning. I would first get `LHS`, 
consume and bail out if it was an error, then do the same for `RHS` and only 
after that return `equivalent`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: pass - 60338 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread liu hui via Phabricator via cfe-commits 
lh123 marked 3 inline comments as done.
lh123 added inline comments.



Comment at: llvm/include/llvm/Support/CommandLine.h:47
+class FileSystem;
+IntrusiveRefCntPtr getRealFileSystem();
+

kadircet wrote:
> now that we are also pulling the the function, I think it is OK to include 
> the header instead.
Yes, we need to include `VirtualFileSystem.h`, `IntrusiveRefCntPtr` needs to 
know the complete definition of `FileSystem`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231242.
lh123 marked an inline comment as done.
lh123 added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -37,6 +37,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1046,9 +1047,13 @@
 static bool ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
+  llvm::ErrorOr CurrDir = FS.getCurrentWorkingDirectory();
+  if (!CurrDir)
+return false;
   ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
+  FS.getBufferForFile(FName);
   if (!MemBufOrErr)
 return false;
   MemoryBuffer  = *MemBufOrErr.get();
@@ -1084,9 +1089,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDir.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1101,8 +1104,8 @@
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1142,13 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  llvm::ErrorOr LHS = FS.status(FName);
+  llvm::ErrorOr RHS = FS.status(RFile.File);
+  if (!LHS || !RHS) {
+return false;
+  }
+  return LHS->equivalent(*RHS);
 };
 
 // Check for recursive response files.
@@ -1156,7 +1164,7 @@
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
 if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+RelativeNames, FS)) {
   // We couldn't read this file, so we leave it in the argument stream and
   // move on.
   AllExpanded = false;
@@ -1187,7 +1195,8 @@
 bool cl::readConfigFile(StringRef CfgFile, StringSaver ,
 SmallVectorImpl ) {
   if (!ExpandResponseFile(CfgFile, Saver, cl::tokenizeConfigFile, Argv,
-  /*MarkEOLs*/ false, /*RelativeNames*/ true))
+  /*MarkEOLs*/ false, /*RelativeNames*/ true,
+  *llvm::vfs::getRealFileSystem()))
 return false;
   return ExpandResponseFiles(Saver, cl::tokenizeConfigFile, Argv,
  /*MarkEOLs*/ false, /*RelativeNames*/ true);
Index: llvm/include/llvm/Support/CommandLine.h
===
--- llvm/include/llvm/Support/CommandLine.h
+++ llvm/include/llvm/Support/CommandLine.h
@@ -29,6 +29,7 @@
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1964,10 +1965,13 @@
 /// with nullptrs in the Argv vector.
 /// \param [in] RelativeNames true if names of nested response files must be
 /// resolved relative to including file.
+/// \param [in] FS File system used for all file access when running the tool.
 /// \return true if all @files were expanded successfully or there were none.
-bool ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs = false, bool RelativeNames = false);
+bool ExpandResponseFiles(
+StringSaver ,

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet added inline comments.



Comment at: llvm/include/llvm/Support/CommandLine.h:47
+class FileSystem;
+IntrusiveRefCntPtr getRealFileSystem();
+

now that we are also pulling the the function, I think it is OK to include the 
header instead.



Comment at: llvm/lib/Support/CommandLine.cpp:1073
 
+  llvm::ErrorOr CurrDir = FS.getCurrentWorkingDirectory();
+  if (!CurrDir)

you can move this to the beginning of the function, to exit immediately, 
without checking for anything else.



Comment at: llvm/lib/Support/CommandLine.cpp:1074
+  llvm::ErrorOr CurrDir = FS.getCurrentWorkingDirectory();
+  if (!CurrDir)
+return false;

it is sad that, ExpandResponseFile returns a bool, but that's a battle for 
another day.

unfortunately, it is not enough return false in this case you need to consume 
the error with `llvm::consumeError` before exiting the scope.



Comment at: llvm/lib/Support/CommandLine.cpp:1146
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  SmallVector LHSPath;

there are `FileSystem::status` and `Status::equivalent` functions to implement 
`llvm::sys::fs:equivalent` behaviour


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread James Henderson via Phabricator via cfe-commits 
jhenderson added a comment.

Aside from one nit, this looks okay to me. I'd like someone else to look at it 
though too. I'm not at all familiar with the file system stuff to be confident 
to say everything there is good.




Comment at: llvm/lib/Support/CommandLine.cpp:1148
+  SmallVector RHSPath;
+  if (FS.getRealPath(FName, LHSPath) || FS.getRealPath(RFile.File, 
RHSPath)) {
+return false;

Has this been clang-formatted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: FAILURE - 
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231234.
lh123 added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769

Files:
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp

Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -37,6 +37,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -1046,9 +1047,10 @@
 static bool ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
SmallVectorImpl ,
-   bool MarkEOLs, bool RelativeNames) {
+   bool MarkEOLs, bool RelativeNames,
+   llvm::vfs::FileSystem ) {
   ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
+  FS.getBufferForFile(FName);
   if (!MemBufOrErr)
 return false;
   MemoryBuffer  = *MemBufOrErr.get();
@@ -1068,6 +1070,9 @@
   else if (hasUTF8ByteOrderMark(BufRef))
 Str = StringRef(BufRef.data() + 3, BufRef.size() - 3);
 
+  llvm::ErrorOr CurrDir = FS.getCurrentWorkingDirectory();
+  if (!CurrDir)
+return false;
   // Tokenize the contents into NewArgv.
   Tokenizer(Str, Saver, NewArgv, MarkEOLs);
 
@@ -1084,9 +1089,7 @@
 SmallString<128> ResponseFile;
 ResponseFile.append(1, '@');
 if (llvm::sys::path::is_relative(FName)) {
-  SmallString<128> curr_dir;
-  llvm::sys::fs::current_path(curr_dir);
-  ResponseFile.append(curr_dir.str());
+  ResponseFile.append(CurrDir.get());
 }
 llvm::sys::path::append(
 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
@@ -1101,8 +1104,8 @@
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver , TokenizerCallback Tokenizer,
- SmallVectorImpl ,
- bool MarkEOLs, bool RelativeNames) {
+ SmallVectorImpl , bool MarkEOLs,
+ bool RelativeNames, llvm::vfs::FileSystem ) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
 const char *File;
@@ -1139,8 +1142,13 @@
 }
 
 const char *FName = Arg + 1;
-auto IsEquivalent = [FName](const ResponseFileRecord ) {
-  return sys::fs::equivalent(RFile.File, FName);
+auto IsEquivalent = [FName, ](const ResponseFileRecord ) {
+  SmallVector LHSPath;
+  SmallVector RHSPath;
+  if (FS.getRealPath(FName, LHSPath) || FS.getRealPath(RFile.File, RHSPath)) {
+return false;
+  }
+  return LHSPath == RHSPath;
 };
 
 // Check for recursive response files.
@@ -1156,7 +1164,7 @@
 // contents.  Nested response files are expanded in subsequent iterations.
 SmallVector ExpandedArgv;
 if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-RelativeNames)) {
+RelativeNames, FS)) {
   // We couldn't read this file, so we leave it in the argument stream and
   // move on.
   AllExpanded = false;
@@ -1187,7 +1195,8 @@
 bool cl::readConfigFile(StringRef CfgFile, StringSaver ,
 SmallVectorImpl ) {
   if (!ExpandResponseFile(CfgFile, Saver, cl::tokenizeConfigFile, Argv,
-  /*MarkEOLs*/ false, /*RelativeNames*/ true))
+  /*MarkEOLs*/ false, /*RelativeNames*/ true,
+  *llvm::vfs::getRealFileSystem()))
 return false;
   return ExpandResponseFiles(Saver, cl::tokenizeConfigFile, Argv,
  /*MarkEOLs*/ false, /*RelativeNames*/ true);
Index: llvm/include/llvm/Support/CommandLine.h
===
--- llvm/include/llvm/Support/CommandLine.h
+++ llvm/include/llvm/Support/CommandLine.h
@@ -20,6 +20,7 @@
 #define LLVM_SUPPORT_COMMANDLINE_H
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
@@ -40,6 +41,12 @@
 #include 
 
 namespace llvm {
+namespace vfs {
+
+class FileSystem;
+IntrusiveRefCntPtr getRealFileSystem();
+
+} // namespace vfs
 
 class StringSaver;
 class raw_ostream;
@@ -1964,10 +1971,13 @@
 /// with nullptrs in the Argv vector.
 /// \param [in] RelativeNames true if names of nested response files must be
 /// resolved relative to including file.
+/// \param [in] FS File system used

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: fail - 60341 tests passed, 3 failed and 732 were skipped.

  failed: Clang.CodeGen/catch-implicit-integer-sign-changes-incdec.c
  failed: Clang.CodeGen/catch-implicit-signed-integer-truncations-incdec.c
  failed: libc++.std/thread/futures/futures_async/async.pass.cpp

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231215.
lh123 added a comment.

address comments and delete unused code.


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

https://reviews.llvm.org/D70769

Files:
  clang/tools/driver/driver.cpp
  lld/COFF/DriverUtils.cpp
  lld/ELF/DriverUtils.cpp
  lld/MinGW/Driver.cpp
  lld/tools/lld/lld.cpp
  lld/wasm/Driver.cpp
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
  llvm/tools/llvm-ar/llvm-ar.cpp
  llvm/tools/llvm-objcopy/llvm-objcopy.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "gtest/gtest.h"
 #include 
 #include 
@@ -830,7 +831,8 @@
   llvm::BumpPtrAllocator A;
   llvm::StringSaver Saver(A);
   bool Res = llvm::cl::ExpandResponseFiles(
-Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true);
+  Saver, llvm::cl::TokenizeGNUCommandLine, *llvm::vfs::getRealFileSystem(),
+  Argv, false, true);
   EXPECT_TRUE(Res);
   EXPECT_EQ(Argv.size(), 13U);
   EXPECT_STREQ(Argv[0], "test/test");
@@ -905,7 +907,8 @@
 #else
   cl::TokenizerCallback Tokenizer = cl::TokenizeGNUCommandLine;
 #endif
-  bool Res = cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false);
+  bool Res = cl::ExpandResponseFiles(
+  Saver, Tokenizer, *llvm::vfs::getRealFileSystem(), Argv, false, false);
   EXPECT_FALSE(Res);
 
   EXPECT_EQ(Argv.size(), 9U);
@@ -943,7 +946,8 @@
 
   BumpPtrAllocator A;
   StringSaver Saver(A);
-  bool Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
+  bool Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine,
+ *llvm::vfs::getRealFileSystem(), Argv,
  false, false);
   EXPECT_FALSE(Res);
 
Index: llvm/tools/llvm-objcopy/llvm-objcopy.cpp
===
--- llvm/tools/llvm-objcopy/llvm-objcopy.cpp
+++ llvm/tools/llvm-objcopy/llvm-objcopy.cpp
@@ -38,6 +38,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
@@ -338,7 +339,7 @@
   Triple(sys::getProcessTriple()).isOSWindows()
   ? cl::TokenizeWindowsCommandLine
   : cl::TokenizeGNUCommandLine,
-  NewArgv);
+  *llvm::vfs::getRealFileSystem(), NewArgv);
 
   auto Args = makeArrayRef(NewArgv).drop_front();
   Expected DriverConfig =
Index: llvm/tools/llvm-ar/llvm-ar.cpp
===
--- llvm/tools/llvm-ar/llvm-ar.cpp
+++ llvm/tools/llvm-ar/llvm-ar.cpp
@@ -32,6 +32,7 @@
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/ToolDrivers/llvm-dlltool/DlltoolDriver.h"
@@ -1096,7 +1097,8 @@
 static int ar_main(int argc, char **argv) {
   SmallVector Argv(argv, argv + argc);
   StringSaver Saver(Alloc);
-  cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
+  cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine,
+  *llvm::vfs::getRealFileSystem(), Argv);
   for (size_t i = 1; i < Argv.size(); ++i) {
 StringRef Arg = Argv[i];
 const char *match = nullptr;
Index: llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
===
--- llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
+++ llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
@@ -27,6 +27,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
@@ -267,7 +268,8 @@
 
   // Parse command line arguments.
   SmallVector NewArgs(ArgsArr.begin(), ArgsArr.end());
-  cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine, NewArgs);
+  cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine,
+  *llvm::vfs::getRealFileSystem(), NewArgs);
   ArgsArr = NewArgs;
 
   LibOptTable Table;
Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -37,6

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread liu hui via Phabricator via cfe-commits 
lh123 updated this revision to Diff 231213.
lh123 added a comment.

Address comments


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

https://reviews.llvm.org/D70769

Files:
  clang/tools/driver/driver.cpp
  lld/COFF/DriverUtils.cpp
  lld/ELF/DriverUtils.cpp
  lld/MinGW/Driver.cpp
  lld/tools/lld/lld.cpp
  lld/wasm/Driver.cpp
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
  llvm/tools/llvm-ar/llvm-ar.cpp
  llvm/tools/llvm-objcopy/llvm-objcopy.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -17,6 +17,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "gtest/gtest.h"
 #include 
 #include 
@@ -830,7 +831,8 @@
   llvm::BumpPtrAllocator A;
   llvm::StringSaver Saver(A);
   bool Res = llvm::cl::ExpandResponseFiles(
-Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true);
+  Saver, llvm::cl::TokenizeGNUCommandLine, *llvm::vfs::getRealFileSystem(),
+  Argv, false, true);
   EXPECT_TRUE(Res);
   EXPECT_EQ(Argv.size(), 13U);
   EXPECT_STREQ(Argv[0], "test/test");
@@ -905,7 +907,8 @@
 #else
   cl::TokenizerCallback Tokenizer = cl::TokenizeGNUCommandLine;
 #endif
-  bool Res = cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false);
+  bool Res = cl::ExpandResponseFiles(
+  Saver, Tokenizer, *llvm::vfs::getRealFileSystem(), Argv, false, false);
   EXPECT_FALSE(Res);
 
   EXPECT_EQ(Argv.size(), 9U);
@@ -943,7 +946,8 @@
 
   BumpPtrAllocator A;
   StringSaver Saver(A);
-  bool Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
+  bool Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine,
+ *llvm::vfs::getRealFileSystem(), Argv,
  false, false);
   EXPECT_FALSE(Res);
 
Index: llvm/tools/llvm-objcopy/llvm-objcopy.cpp
===
--- llvm/tools/llvm-objcopy/llvm-objcopy.cpp
+++ llvm/tools/llvm-objcopy/llvm-objcopy.cpp
@@ -38,6 +38,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
@@ -338,7 +339,7 @@
   Triple(sys::getProcessTriple()).isOSWindows()
   ? cl::TokenizeWindowsCommandLine
   : cl::TokenizeGNUCommandLine,
-  NewArgv);
+  *llvm::vfs::getRealFileSystem(), NewArgv);
 
   auto Args = makeArrayRef(NewArgv).drop_front();
   Expected DriverConfig =
Index: llvm/tools/llvm-ar/llvm-ar.cpp
===
--- llvm/tools/llvm-ar/llvm-ar.cpp
+++ llvm/tools/llvm-ar/llvm-ar.cpp
@@ -32,6 +32,7 @@
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/ToolDrivers/llvm-dlltool/DlltoolDriver.h"
@@ -1096,7 +1097,8 @@
 static int ar_main(int argc, char **argv) {
   SmallVector Argv(argv, argv + argc);
   StringSaver Saver(Alloc);
-  cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
+  cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine,
+  *llvm::vfs::getRealFileSystem(), Argv);
   for (size_t i = 1; i < Argv.size(); ++i) {
 StringRef Arg = Argv[i];
 const char *match = nullptr;
Index: llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
===
--- llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
+++ llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
@@ -27,6 +27,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
@@ -267,7 +268,8 @@
 
   // Parse command line arguments.
   SmallVector NewArgs(ArgsArr.begin(), ArgsArr.end());
-  cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine, NewArgs);
+  cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine,
+  *llvm::vfs::getRealFileSystem(), NewArgs);
   ArgsArr = NewArgs;
 
   LibOptTable Table;
Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -37,6 +37,7 @@
 #include

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread James Henderson via Phabricator via cfe-commits 
jhenderson added a comment.

In D70769#1761517 , @sammccall wrote:

> It's not just unit-tests, in D70222  we will 
> ultimately use FSes other than getRealFilesystem() in clangd.


I see, thank you.

> Having non-VFS-clean versions of functions around that silently use the real 
> FS is a bit of a scourge for users that need to be VFS-clean, honestly. 
> Parsing argv is exactly the sort of place where FS access isn't obvious and 
> exposing a function that doesn't accept a VFS encourages callers to miss this 
> aspect. At the same time this is mostly called from a bunch of drivers that 
> (presumably) don't need VFS support. Maybe allowing nullptr = real FS, or a 
> default argument, would be an OK compromise?

A default argument would be my preference there. It looks like we have some 
already.




Comment at: llvm/include/llvm/Support/CommandLine.h:1963
 /// \param [in] Tokenizer Tokenization strategy. Typically Unix or Windows.
+/// \param [in] FS VFS used for all file accesses when running the tool.
 /// \param [in,out] Argv Command line into which to expand response files.

Does `VFS` make sense here, given that it could be a real filesystem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added a comment.

In D70769#1761494 , @jhenderson wrote:

> In D70769#1761428 , @lh123 wrote:
>
> > In D70769#1761394 , @jhenderson 
> > wrote:
> >
> > > Are there any instances where we DON'T want to get the real file system? 
> > > If not, could the `*llvm::vfs::getRealFileSystem()` call be put inside 
> > > `cl::ExpandResponseFiles`?
> >
> >
> > This patch is part of D70222 .
> >
> >   This is for using InMemoryFileSystem in unit tests.
>
>
> Okay, that makes sense. Perhaps we should introduce a new overload of 
> `ExpandResponseFiles` that allows specifying a different file system then, 
> and having the current version call into that one, specifying the 
> getRealFileSystem call? I'm not overly keen on having yet another apparently 
> boiler-plate piece of functionality required in every single call of 
> `ExpandResponseFiles`.


It's not just unit-tests, in D70222  we will 
ultimately use FSes other than getRealFilesystem() in clangd.
Having non-VFS-clean versions of functions around that silently use the real FS 
is a bit of a scourge for users that need to be VFS-clean, honestly. Parsing 
argv is exactly the sort of place where FS access isn't obvious and exposing a 
function that doesn't accept a VFS encourages callers to miss this aspect. At 
the same time this is mostly called from a bunch of drivers that (presumably) 
don't need VFS support. Maybe allowing nullptr = real FS, or a default 
argument, would be an OK compromise?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread James Henderson via Phabricator via cfe-commits 
jhenderson added a comment.

In D70769#1761428 , @lh123 wrote:

> In D70769#1761394 , @jhenderson 
> wrote:
>
> > Are there any instances where we DON'T want to get the real file system? If 
> > not, could the `*llvm::vfs::getRealFileSystem()` call be put inside 
> > `cl::ExpandResponseFiles`?
>
>
> This patch is part of D70222 .
>
>   This is for using InMemoryFileSystem in unit tests.


Okay, that makes sense. Perhaps we should introduce a new overload of 
`ExpandResponseFiles` that allows specifying a different file system then, and 
having the current version call into that one, specifying the getRealFileSystem 
call? I'm not overly keen on having yet another apparently boiler-plate piece 
of functionality required in every single call of `ExpandResponseFiles`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread liu hui via Phabricator via cfe-commits 
lh123 added a comment.

In D70769#1761394 , @jhenderson wrote:

> Are there any instances where we DON'T want to get the real file system? If 
> not, could the `*llvm::vfs::getRealFileSystem()` call be put inside 
> `cl::ExpandResponseFiles`?


This patch is part of D70222 .
 This is for using InMemoryFileSystem in unit tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread James Henderson via Phabricator via cfe-commits 
jhenderson added a comment.

Are there any instances where we DON'T want to get the real file system? If 
not, could the `*llvm::vfs::getRealFileSystem()` call be put inside 
`cl::ExpandResponseFiles`?




Comment at: llvm/include/llvm/Support/CommandLine.h:32
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"

Can llvm::vfs::FileSystem be forward declared and this moved into the .cpp?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread pre-merge checks [bot] via Phabricator via cfe-commits 
merge_guards_bot added a comment.

Build result: pass - 60330 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769



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

[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

2019-11-27 Thread liu hui via Phabricator via cfe-commits 
lh123 created this revision.
lh123 added a reviewer: kadircet.
Herald added subscribers: llvm-commits, cfe-commits, seiya, rupprecht, MaskRay, 
jakehehrlich, aheejin, hiraditya, arichardson, sbc100, emaste.
Herald added a reviewer: espindola.
Herald added a reviewer: alexshap.
Herald added a reviewer: rupprecht.
Herald added a reviewer: jhenderson.
Herald added projects: clang, LLVM.

add vfs support for `ExpandResponseFiles`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70769

Files:
  clang/tools/driver/driver.cpp
  lld/COFF/DriverUtils.cpp
  lld/ELF/DriverUtils.cpp
  lld/MinGW/Driver.cpp
  lld/tools/lld/lld.cpp
  lld/wasm/Driver.cpp
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
  llvm/tools/llvm-ar/llvm-ar.cpp
  llvm/tools/llvm-objcopy/llvm-objcopy.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -830,7 +830,8 @@
   llvm::BumpPtrAllocator A;
   llvm::StringSaver Saver(A);
   bool Res = llvm::cl::ExpandResponseFiles(
-Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true);
+  Saver, llvm::cl::TokenizeGNUCommandLine, *llvm::vfs::getRealFileSystem(),
+  Argv, false, true);
   EXPECT_TRUE(Res);
   EXPECT_EQ(Argv.size(), 13U);
   EXPECT_STREQ(Argv[0], "test/test");
@@ -905,7 +906,8 @@
 #else
   cl::TokenizerCallback Tokenizer = cl::TokenizeGNUCommandLine;
 #endif
-  bool Res = cl::ExpandResponseFiles(Saver, Tokenizer, Argv, false, false);
+  bool Res = cl::ExpandResponseFiles(
+  Saver, Tokenizer, *llvm::vfs::getRealFileSystem(), Argv, false, false);
   EXPECT_FALSE(Res);
 
   EXPECT_EQ(Argv.size(), 9U);
@@ -943,7 +945,8 @@
 
   BumpPtrAllocator A;
   StringSaver Saver(A);
-  bool Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv,
+  bool Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine,
+ *llvm::vfs::getRealFileSystem(), Argv,
  false, false);
   EXPECT_FALSE(Res);
 
Index: llvm/tools/llvm-objcopy/llvm-objcopy.cpp
===
--- llvm/tools/llvm-objcopy/llvm-objcopy.cpp
+++ llvm/tools/llvm-objcopy/llvm-objcopy.cpp
@@ -338,7 +338,7 @@
   Triple(sys::getProcessTriple()).isOSWindows()
   ? cl::TokenizeWindowsCommandLine
   : cl::TokenizeGNUCommandLine,
-  NewArgv);
+  *llvm::vfs::getRealFileSystem(), NewArgv);
 
   auto Args = makeArrayRef(NewArgv).drop_front();
   Expected DriverConfig =
Index: llvm/tools/llvm-ar/llvm-ar.cpp
===
--- llvm/tools/llvm-ar/llvm-ar.cpp
+++ llvm/tools/llvm-ar/llvm-ar.cpp
@@ -1096,7 +1096,8 @@
 static int ar_main(int argc, char **argv) {
   SmallVector Argv(argv, argv + argc);
   StringSaver Saver(Alloc);
-  cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
+  cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine,
+  *llvm::vfs::getRealFileSystem(), Argv);
   for (size_t i = 1; i < Argv.size(); ++i) {
 StringRef Arg = Argv[i];
 const char *match = nullptr;
Index: llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
===
--- llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
+++ llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp
@@ -267,7 +267,8 @@
 
   // Parse command line arguments.
   SmallVector NewArgs(ArgsArr.begin(), ArgsArr.end());
-  cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine, NewArgs);
+  cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine,
+  *llvm::vfs::getRealFileSystem(), NewArgs);
   ArgsArr = NewArgs;
 
   LibOptTable Table;
Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -1045,10 +1045,11 @@
 
 static bool ExpandResponseFile(StringRef FName, StringSaver ,
TokenizerCallback Tokenizer,
+   llvm::vfs::FileSystem ,
SmallVectorImpl ,
bool MarkEOLs, bool RelativeNames) {
   ErrorOr> MemBufOrErr =
-  MemoryBuffer::getFile(FName);
+  FS.getBufferForFile(FName);
   if (!MemBufOrErr)
 return false;
   MemoryBuffer  = *MemBufOrErr.get();
@@ -1068,6 +1069,9 @@
   else if (hasUTF8ByteOrderMark(BufRef))
 Str = StringRef(BufRef.data() + 3, BufRef.size() - 3);
 
+  llvm::ErrorOr CurrDir = FS.getCurrentWorkingDirectory();
+  if (!CurrDir)
+return false;
   // Tokenize the