[PATCH] D17065: Defer CWD in MCContext lookup as late as possible.

[Justin Bogner via llvm-commits]

Chandler Carruth via llvm-commits <llvm-commits at lists.llvm.org> writes:
> Owen Anderson via llvm-commits <llvm-commits at lists.llvm.org> writes:
>> resistor created this revision.
>> resistor added a reviewer: chandlerc.
>> resistor added a subscriber: llvm-commits.
>> resistor set the repository for this revision to rL LLVM.
>>
>> Currently MCContext tries to lookup CWD on construction.  This causes
>> sandboxing violations when using LLVM in a daemon without filesystem
>> access.  The solution is defer CWD lookup until absolutely necessary.
>
> The goal here makes perfect sense, but I wonder if this is the right
> fix... It seems almost too surgical, and makes it really easy to
> change MC in a way that yet again violates things.
>
> I feel like the MCContext shouldn't be looking at the filesystem *at
> all*, and it should be the client that sets up the MCContext that
> provides a compilation directory, potentially via the filesystem
> query, where appropriate. Does that make sense here?

For all intents and purposes, this is already done and the code Owen's
removing here is functionally dead.

- Clang calls setCompilationDir (by passing -fdebug-compilation-dir to
  clang -cc1) with the result of current_path unless current_path fails,
  but it would just fail again if that happened.

- opt and llc don't generate debug info, so they just pass along the
  AT_comp_dir directive in the IR if it's there.

- llvm-mc does rely on this, but (1) it's a testing tool, and (2) it's
  trivial to make it call current_path.

I think we just want to remove the call to current_path in LLVMContext's
constructor and add a call to setCompilationDir in llvm-mc, like in the
attached patch. WDYT?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: debug-compilation-dir.patch
Type: text/x-patch
Size: 2299 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160321/832fd0da/attachment.bin>
-------------- next part --------------


>> Repository:
>>   rL LLVM
>>
>> http://reviews.llvm.org/D17065
>>
>> Files:
>>   include/llvm/MC/MCContext.h
>>   lib/MC/MCContext.cpp
>>
>> Index: lib/MC/MCContext.cpp
>> ===================================================================
>> --- lib/MC/MCContext.cpp
>> +++ lib/MC/MCContext.cpp
>> @@ -42,11 +42,6 @@
>>        GenDwarfForAssembly(false), GenDwarfFileNumber(0), DwarfVersion(4),
>>        AllowTemporaryLabels(true), DwarfCompileUnitID(0),
>>        AutoReset(DoAutoReset) {
>> -
>> -  std::error_code EC = llvm::sys::fs::current_path(CompilationDir);
>> -  if (EC)
>> -    CompilationDir.clear();
>> -
>>    SecureLogFile = getenv("AS_SECURE_LOG_FILE");
>>    SecureLog = nullptr;
>>    SecureLogUsed = false;
>> @@ -67,6 +62,17 @@
>>    delete (raw_ostream *)SecureLog;
>>  }
>>
>> +StringRef MCContext::getCompilationDir() const {
>> +  if (!CompilationDir) {
>> +    SmallString<128> Cwd;
>> +    std::error_code EC = llvm::sys::fs::current_path(Cwd);
>> +    if (EC)
>> +      Cwd.clear();
>> +    CompilationDir = Cwd;
>> +  }
>> +  return *CompilationDir;
>> +}
>> +
>>  //===----------------------------------------------------------------------===//
>>  // Module Lifetime Management
>>  //===----------------------------------------------------------------------===//
>> @@ -86,7 +92,7 @@
>>    SectionSymbols.clear();
>>    Allocator.Reset();
>>    Instances.clear();
>> -  CompilationDir.clear();
>> +  CompilationDir.reset();
>>    MainFileName.clear();
>>    MCDwarfLineTablesCUMap.clear();
>>    SectionsForRanges.clear();
>> Index: include/llvm/MC/MCContext.h
>> ===================================================================
>> --- include/llvm/MC/MCContext.h
>> +++ include/llvm/MC/MCContext.h
>> @@ -11,6 +11,7 @@
>>  #define LLVM_MC_MCCONTEXT_H
>>
>>  #include "llvm/ADT/DenseMap.h"
>> +#include "llvm/ADT/Optional.h"
>>  #include "llvm/ADT/SetVector.h"
>>  #include "llvm/ADT/SmallString.h"
>>  #include "llvm/ADT/SmallVector.h"
>> @@ -116,7 +117,7 @@
>>      bool SecureLogUsed;
>>
>>      /// The compilation directory to use for DW_AT_comp_dir.
>> -    SmallString<128> CompilationDir;
>> +    mutable Optional<SmallString<128>> CompilationDir;
>>
>>      /// The main file name if passed in explicitly.
>>      std::string MainFileName;
>> @@ -392,11 +393,13 @@
>>      /// compilation directory and have it be something other than the current
>>      /// working directory.
>>      /// Returns an empty string if the current directory cannot be determined.
>> -    StringRef getCompilationDir() const { return CompilationDir; }
>> +    StringRef getCompilationDir() const;
>>
>>      /// \brief Set the compilation directory for DW_AT_comp_dir
>>      /// Override the default (CWD) compilation directory.
>> -    void setCompilationDir(StringRef S) { CompilationDir = S.str(); }
>> +    void setCompilationDir(StringRef S) {
>> +      CompilationDir = SmallString<128>(S.str());
>> +    }
>>
>>      /// \brief Get the main file name for use in error messages and debug
>>      /// info. This can be set to ensure we've got the correct file name
>>