r371027 - Revert r361885 "[Driver] Fix -working-directory issues"

Mikael Holmén via cfe-commits cfe-commits at lists.llvm.org
Thu Sep 26 03:55:03 PDT 2019


Hi Hans,

I'm a bit suspicious against the part

 > This also revertes the part of r369938 which checked that 
-working-directory works.

in this revert.

You do:

 > +  SmallString<128> Buf;
 > +  if (!llvm::sys::fs::current_path(Buf))
 > +    Buf = ".";
 > +  CDB << "{ \"directory\": \"" << escape(Buf) << "\"";

But if I look at r369938 it seems like before r369938 it looked like

-  SmallString<128> Buf;
-  if (llvm::sys::fs::current_path(Buf))
-    Buf = ".";
-  CDB << "{ \"directory\": \"" << escape(Buf) << "\"";

Note the difference in the condition, where you do

  if (!llvm::sys::fs::current_path(Buf))

but before it was

  if (llvm::sys::fs::current_path(Buf))


We noticed this since

  clang -MJ test.json -O2 -nostdinc empty.c -S -o - && cat test.json

started to behave differently after this revert. The "directory: " part 
in the output suddenly became ".".

/Mikael

On 2019-09-05 10:43, Hans Wennborg via cfe-commits wrote:
> Author: hans
> Date: Thu Sep  5 01:43:00 2019
> New Revision: 371027
> 
> URL: https://protect2.fireeye.com/url?k=a390ce47-ff1a05b3-a3908edc-86cd58c48020-98ad75d673bc6e26&q=1&u=http%3A%2F%2Fllvm.org%2Fviewvc%2Fllvm-project%3Frev%3D371027%26view%3Drev
> Log:
> Revert r361885 "[Driver] Fix -working-directory issues"
> 
> This made clang unable to open files using relative paths on network shares on
> Windows (PR43204). On the bug it was pointed out that createPhysicalFileSystem()
> is not terribly mature, and using it is risky. Reverting for now until there's
> a clear way forward.
> 
>> Currently the `-working-directory` option does not actually impact the working
>> directory for all of the clang driver, it only impacts how files are looked up
>> to make sure they exist.  This means that that clang passes the wrong paths
>> to -fdebug-compilation-dir and -coverage-notes-file.
>>
>> This patch fixes that by changing all the places in the driver where we convert
>> to absolute paths to use the VFS, and then calling setCurrentWorkingDirectory on
>> the VFS.  This also changes the default VFS for `Driver` to use a virtualized
>> working directory, instead of changing the process's working directory.
>>
>> Differential Revision: https://protect2.fireeye.com/url?k=4c8bdb04-100110f0-4c8b9b9f-86cd58c48020-5e9c5a0eee8ac746&q=1&u=https%3A%2F%2Freviews.llvm.org%2FD62271
> 
> This also revertes the part of r369938 which checked that -working-directory works.
> 
> Modified:
>      cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
>      cfe/trunk/lib/Driver/Driver.cpp
>      cfe/trunk/lib/Driver/ToolChains/Clang.cpp
>      cfe/trunk/test/Driver/gen-cdb-fragment.c
>      cfe/trunk/test/Driver/working-directory.c
> 
> Modified: cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
> URL: https://protect2.fireeye.com/url?k=dc8ae2d2-80002926-dc8aa249-86cd58c48020-73e94a1494d49e2b&q=1&u=http%3A%2F%2Fllvm.org%2Fviewvc%2Fllvm-project%2Fcfe%2Ftrunk%2Finclude%2Fclang%2FBasic%2FDiagnosticDriverKinds.td%3Frev%3D371027%26r1%3D371026%26r2%3D371027%26view%3Ddiff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td Thu Sep  5 01:43:00 2019
> @@ -91,8 +91,6 @@ def err_no_external_assembler : Error<
>     "there is no external assembler that can be used on this platform">;
>   def err_drv_unable_to_remove_file : Error<
>     "unable to remove file: %0">;
> -def err_drv_unable_to_set_working_directory : Error <
> -  "unable to set working directory: %0">;
>   def err_drv_command_failure : Error<
>     "unable to execute command: %0">;
>   def err_drv_invalid_darwin_version : Error<
> 
> Modified: cfe/trunk/lib/Driver/Driver.cpp
> URL: https://protect2.fireeye.com/url?k=64c4a345-384e68b1-64c4e3de-86cd58c48020-5b165b055bfa7d40&q=1&u=http%3A%2F%2Fllvm.org%2Fviewvc%2Fllvm-project%2Fcfe%2Ftrunk%2Flib%2FDriver%2FDriver.cpp%3Frev%3D371027%26r1%3D371026%26r2%3D371027%26view%3Ddiff
> ==============================================================================
> --- cfe/trunk/lib/Driver/Driver.cpp (original)
> +++ cfe/trunk/lib/Driver/Driver.cpp Thu Sep  5 01:43:00 2019
> @@ -133,7 +133,7 @@ Driver::Driver(StringRef ClangExecutable
>   
>     // Provide a sane fallback if no VFS is specified.
>     if (!this->VFS)
> -    this->VFS = llvm::vfs::createPhysicalFileSystem().release();
> +    this->VFS = llvm::vfs::getRealFileSystem();
>   
>     Name = llvm::sys::path::filename(ClangExecutable);
>     Dir = llvm::sys::path::parent_path(ClangExecutable);
> @@ -1010,11 +1010,6 @@ Compilation *Driver::BuildCompilation(Ar
>       }
>     }
>   
> -  // Check for working directory option before accessing any files
> -  if (Arg *WD = Args.getLastArg(options::OPT_working_directory))
> -    if (VFS->setCurrentWorkingDirectory(WD->getValue()))
> -      Diag(diag::err_drv_unable_to_set_working_directory) << WD->getValue();
> -
>     // FIXME: This stuff needs to go into the Compilation, not the driver.
>     bool CCCPrintPhases;
>   
> @@ -1996,11 +1991,20 @@ bool Driver::DiagnoseInputExistence(cons
>     if (Value == "-")
>       return true;
>   
> -  if (getVFS().exists(Value))
> +  SmallString<64> Path(Value);
> +  if (Arg *WorkDir = Args.getLastArg(options::OPT_working_directory)) {
> +    if (!llvm::sys::path::is_absolute(Path)) {
> +      SmallString<64> Directory(WorkDir->getValue());
> +      llvm::sys::path::append(Directory, Value);
> +      Path.assign(Directory);
> +    }
> +  }
> +
> +  if (getVFS().exists(Path))
>       return true;
>   
>     if (IsCLMode()) {
> -    if (!llvm::sys::path::is_absolute(Twine(Value)) &&
> +    if (!llvm::sys::path::is_absolute(Twine(Path)) &&
>           llvm::sys::Process::FindInEnvPath("LIB", Value))
>         return true;
>   
> @@ -2026,12 +2030,12 @@ bool Driver::DiagnoseInputExistence(cons
>       if (getOpts().findNearest(Value, Nearest, IncludedFlagsBitmask,
>                                 ExcludedFlagsBitmask) <= 1) {
>         Diag(clang::diag::err_drv_no_such_file_with_suggestion)
> -          << Value << Nearest;
> +          << Path << Nearest;
>         return false;
>       }
>     }
>   
> -  Diag(clang::diag::err_drv_no_such_file) << Value;
> +  Diag(clang::diag::err_drv_no_such_file) << Path;
>     return false;
>   }
>   
> 
> Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
> URL: https://protect2.fireeye.com/url?k=29c8d80a-754213fe-29c89891-86cd58c48020-d3513617254de7d4&q=1&u=http%3A%2F%2Fllvm.org%2Fviewvc%2Fllvm-project%2Fcfe%2Ftrunk%2Flib%2FDriver%2FToolChains%2FClang.cpp%3Frev%3D371027%26r1%3D371026%26r2%3D371027%26view%3Ddiff
> ==============================================================================
> --- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
> +++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Thu Sep  5 01:43:00 2019
> @@ -608,15 +608,16 @@ getFramePointerKind(const ArgList &Args,
>   }
>   
>   /// Add a CC1 option to specify the debug compilation directory.
> -static void addDebugCompDirArg(const ArgList &Args, ArgStringList &CmdArgs,
> -                               const llvm::vfs::FileSystem &VFS) {
> +static void addDebugCompDirArg(const ArgList &Args, ArgStringList &CmdArgs) {
>     if (Arg *A = Args.getLastArg(options::OPT_fdebug_compilation_dir)) {
>       CmdArgs.push_back("-fdebug-compilation-dir");
>       CmdArgs.push_back(A->getValue());
> -  } else if (llvm::ErrorOr<std::string> CWD =
> -                 VFS.getCurrentWorkingDirectory()) {
> -    CmdArgs.push_back("-fdebug-compilation-dir");
> -    CmdArgs.push_back(Args.MakeArgString(*CWD));
> +  } else {
> +    SmallString<128> cwd;
> +    if (!llvm::sys::fs::current_path(cwd)) {
> +      CmdArgs.push_back("-fdebug-compilation-dir");
> +      CmdArgs.push_back(Args.MakeArgString(cwd));
> +    }
>     }
>   }
>   
> @@ -882,8 +883,13 @@ static void addPGOAndCoverageFlags(const
>         else
>           OutputFilename = llvm::sys::path::filename(Output.getBaseInput());
>         SmallString<128> CoverageFilename = OutputFilename;
> -      if (llvm::sys::path::is_relative(CoverageFilename))
> -        (void)D.getVFS().makeAbsolute(CoverageFilename);
> +      if (llvm::sys::path::is_relative(CoverageFilename)) {
> +        SmallString<128> Pwd;
> +        if (!llvm::sys::fs::current_path(Pwd)) {
> +          llvm::sys::path::append(Pwd, CoverageFilename);
> +          CoverageFilename.swap(Pwd);
> +        }
> +      }
>         llvm::sys::path::replace_extension(CoverageFilename, "gcno");
>         CmdArgs.push_back(Args.MakeArgString(CoverageFilename));
>   
> @@ -2013,14 +2019,13 @@ void Clang::DumpCompilationDatabase(Comp
>       CompilationDatabase = std::move(File);
>     }
>     auto &CDB = *CompilationDatabase;
> -  auto CWD = D.getVFS().getCurrentWorkingDirectory();
> -  if (!CWD)
> -    CWD = ".";
> -  CDB << "{ \"directory\": \"" << escape(*CWD) << "\"";
> +  SmallString<128> Buf;
> +  if (!llvm::sys::fs::current_path(Buf))
> +    Buf = ".";
> +  CDB << "{ \"directory\": \"" << escape(Buf) << "\"";
>     CDB << ", \"file\": \"" << escape(Input.getFilename()) << "\"";
>     CDB << ", \"output\": \"" << escape(Output.getFilename()) << "\"";
>     CDB << ", \"arguments\": [\"" << escape(D.ClangExecutable) << "\"";
> -  SmallString<128> Buf;
>     Buf = "-x";
>     Buf += types::getTypeName(Input.getType());
>     CDB << ", \"" << escape(Buf) << "\"";
> @@ -4464,7 +4469,7 @@ void Clang::ConstructJob(Compilation &C,
>       CmdArgs.push_back("-fno-autolink");
>   
>     // Add in -fdebug-compilation-dir if necessary.
> -  addDebugCompDirArg(Args, CmdArgs, D.getVFS());
> +  addDebugCompDirArg(Args, CmdArgs);
>   
>     addDebugPrefixMapArg(D, Args, CmdArgs);
>   
> @@ -6192,7 +6197,7 @@ void ClangAs::ConstructJob(Compilation &
>       DebugInfoKind = (WantDebug ? codegenoptions::LimitedDebugInfo
>                                  : codegenoptions::NoDebugInfo);
>       // Add the -fdebug-compilation-dir flag if needed.
> -    addDebugCompDirArg(Args, CmdArgs, C.getDriver().getVFS());
> +    addDebugCompDirArg(Args, CmdArgs);
>   
>       addDebugPrefixMapArg(getToolChain().getDriver(), Args, CmdArgs);
>   
> 
> Modified: cfe/trunk/test/Driver/gen-cdb-fragment.c
> URL: https://protect2.fireeye.com/url?k=98fca8be-c476634a-98fce825-86cd58c48020-8d98a9a164425ae2&q=1&u=http%3A%2F%2Fllvm.org%2Fviewvc%2Fllvm-project%2Fcfe%2Ftrunk%2Ftest%2FDriver%2Fgen-cdb-fragment.c%3Frev%3D371027%26r1%3D371026%26r2%3D371027%26view%3Ddiff
> ==============================================================================
> --- cfe/trunk/test/Driver/gen-cdb-fragment.c (original)
> +++ cfe/trunk/test/Driver/gen-cdb-fragment.c Thu Sep  5 01:43:00 2019
> @@ -15,14 +15,6 @@
>   // RUN: %clang -target x86_64-apple-macos10.15 -S %s -o -  -gen-cdb-fragment-path %t.cdb
>   // RUN: ls %t.cdb | FileCheck --check-prefix=CHECK-LS %s
>   
> -// Working directory arg is respected.
> -// RUN: rm -rf %t.cdb
> -// RUN: mkdir %t.cdb
> -// RUN: %clang -target x86_64-apple-macos10.15 -working-directory %t.cdb -c %s -o -  -gen-cdb-fragment-path "."
> -// RUN: ls %t.cdb | FileCheck --check-prefix=CHECK-LS %s
> -// RUN: cat %t.cdb/*.json | FileCheck --check-prefix=CHECK-CWD %s
> -// CHECK-CWD: "directory": "{{.*}}.cdb"
> -
>   // -### does not emit the CDB fragment
>   // RUN: rm -rf %t.cdb
>   // RUN: mkdir %t.cdb
> 
> Modified: cfe/trunk/test/Driver/working-directory.c
> URL: https://protect2.fireeye.com/url?k=e7c50cb3-bb4fc747-e7c54c28-86cd58c48020-1a2070795ddb99be&q=1&u=http%3A%2F%2Fllvm.org%2Fviewvc%2Fllvm-project%2Fcfe%2Ftrunk%2Ftest%2FDriver%2Fworking-directory.c%3Frev%3D371027%26r1%3D371026%26r2%3D371027%26view%3Ddiff
> ==============================================================================
> --- cfe/trunk/test/Driver/working-directory.c (original)
> +++ cfe/trunk/test/Driver/working-directory.c Thu Sep  5 01:43:00 2019
> @@ -1,11 +1,3 @@
>   // RUN: %clang -### -working-directory /no/such/dir/ input 2>&1 | FileCheck %s
> -// RUN: %clang -### -working-directory %p/Inputs no_such_file.cpp -c 2>&1 | FileCheck %s --check-prefix=CHECK_NO_FILE
> -// RUN: %clang -### -working-directory %p/Inputs pchfile.cpp -c 2>&1 | FileCheck %s --check-prefix=CHECK_WORKS
>   
> -// CHECK: unable to set working directory: /no/such/dir/
> -
> -// CHECK_NO_FILE: no such file or directory: 'no_such_file.cpp'
> -
> -// CHECK_WORKS: "-coverage-notes-file" "{{[^"]+}}test{{/|\\\\}}Driver{{/|\\\\}}Inputs{{/|\\\\}}pchfile.gcno"
> -// CHECK_WORKS: "-working-directory" "{{[^"]+}}test{{/|\\\\}}Driver{{/|\\\\}}Inputs"
> -// CHECK_WORKS: "-fdebug-compilation-dir" "{{[^"]+}}test{{/|\\\\}}Driver{{/|\\\\}}Inputs"
> +//CHECK: no such file or directory: '/no/such/dir/input'
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://protect2.fireeye.com/url?k=06063816-5a8cf3e2-0606788d-86cd58c48020-4a392f810e67be81&q=1&u=https%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fcfe-commits
> 


More information about the cfe-commits mailing list