r202329 - Add a 'use-external-names' option to VFS overlay files

Ben Langmuir via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 20 09:43:38 PDT 2016


> On Jul 18, 2016, at 3:21 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 
> On Wed, Feb 26, 2014 at 4:25 PM, Ben Langmuir <blangmuir at apple.com <mailto:blangmuir at apple.com>> wrote:
> Author: benlangmuir
> Date: Wed Feb 26 18:25:12 2014
> New Revision: 202329
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=202329&view=rev <http://llvm.org/viewvc/llvm-project?rev=202329&view=rev>
> Log:
> Add a 'use-external-names' option to VFS overlay files
> 
> When true, sets the name of the file to be the name from
> 'external-contents'. Otherwise, you get the virtual path that the file
> was looked up by. This will not affect any non-virtual paths, or fully
> virtual paths (for which there is no reasonable 'external' name anyway).
> 
> The setting is available globally, but can be overriden on a per-file
> basis.
> 
> The goal is that this setting will control which path you see in debug
> info, diagnostics, etc. which are sensitive to which path is used. That
> will come in future patches that pass the name through to FileManager.
> 
> Modified:
>     cfe/trunk/include/clang/Basic/VirtualFileSystem.h
>     cfe/trunk/lib/Basic/VirtualFileSystem.cpp
>     cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
> 
> Modified: cfe/trunk/include/clang/Basic/VirtualFileSystem.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/VirtualFileSystem.h?rev=202329&r1=202328&r2=202329&view=diff <http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/VirtualFileSystem.h?rev=202329&r1=202328&r2=202329&view=diff>
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/VirtualFileSystem.h (original)
> +++ cfe/trunk/include/clang/Basic/VirtualFileSystem.h Wed Feb 26 18:25:12 2014
> @@ -29,7 +29,6 @@ namespace vfs {
>  /// \brief The result of a \p status operation.
>  class Status {
>    std::string Name;
> -  std::string ExternalName;
>    llvm::sys::fs::UniqueID UID;
>    llvm::sys::TimeValue MTime;
>    uint32_t User;
> @@ -46,16 +45,9 @@ public:
>           uint64_t Size, llvm::sys::fs::file_type Type,
>           llvm::sys::fs::perms Perms);
> 
> -  /// \brief Returns the name this status was looked up by.
> +  /// \brief Returns the name that should be used for this file or directory.
>    StringRef getName() const { return Name; }
> -
> -  /// \brief Returns the name to use outside the compiler.
> -  ///
> -  /// For example, in diagnostics or debug info we should use this name.
> -  StringRef getExternalName() const { return ExternalName; }
> -
>    void setName(StringRef N) { Name = N; }
> -  void setExternalName(StringRef N) { ExternalName = N; }
> 
>    /// @name Status interface from llvm::sys::fs
>    /// @{
> 
> Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=202329&r1=202328&r2=202329&view=diff <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=202329&r1=202328&r2=202329&view=diff>
> ==============================================================================
> --- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original)
> +++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Wed Feb 26 18:25:12 2014
> @@ -35,8 +35,8 @@ Status::Status(const file_status &Status
>  Status::Status(StringRef Name, StringRef ExternalName, UniqueID UID,
>                 sys::TimeValue MTime, uint32_t User, uint32_t Group,
>                 uint64_t Size, file_type Type, perms Perms)
> -    : Name(Name), ExternalName(ExternalName), UID(UID), MTime(MTime),
> -      User(User), Group(Group), Size(Size), Type(Type), Perms(Perms) {}
> +    : Name(Name), UID(UID), MTime(MTime), User(User), Group(Group), Size(Size),
> +      Type(Type), Perms(Perms) {}
> 
>  bool Status::equivalent(const Status &Other) const {
>    return getUniqueID() == Other.getUniqueID();
> @@ -145,7 +145,6 @@ ErrorOr<Status> RealFileSystem::status(c
>      return EC;
>    Status Result(RealStatus);
>    Result.setName(Path.str());
> -  Result.setExternalName(Path.str());
>    return Result;
>  }
> 
> @@ -253,12 +252,22 @@ public:
>  };
> 
>  class FileEntry : public Entry {
> +public:
> +  enum NameKind {
> +    NK_NotSet,
> +    NK_External,
> +    NK_Virtual
> +  };
> +private:
>    std::string ExternalContentsPath;
> -
> +  NameKind UseName;
>  public:
> -  FileEntry(StringRef Name, StringRef ExternalContentsPath)
> -      : Entry(EK_File, Name), ExternalContentsPath(ExternalContentsPath) {}
> +  FileEntry(StringRef Name, StringRef ExternalContentsPath, NameKind UseName)
> +      : Entry(EK_File, Name), ExternalContentsPath(ExternalContentsPath),
> +        UseName(UseName) {}
>    StringRef getExternalContentsPath() const { return ExternalContentsPath; }
> +  /// \brief whether to use the external path as the name for this file.
> +  NameKind useName() const { return UseName; }
>    static bool classof(const Entry *E) { return E->getKind() == EK_File; }
>  };
> 
> @@ -280,6 +289,7 @@ public:
>  ///
>  /// All configuration options are optional.
>  ///   'case-sensitive': <boolean, default=true>
> +///   'use-external-names': <boolean, default=true>
>  ///
>  /// Virtual directories are represented as
>  /// \verbatim
> @@ -304,6 +314,7 @@ public:
>  /// {
>  ///   'type': 'file',
>  ///   'name': <string>,
> +///   'use-external-name': <boolean> # Optional
>  ///   'external-contents': <path to external file>)
>  /// }
>  /// \endverbatim
> @@ -324,14 +335,18 @@ class VFSFromYAML : public vfs::FileSyst
>    /// \brief Whether to perform case-sensitive comparisons.
>    ///
>    /// Currently, case-insensitive matching only works correctly with ASCII.
> -  bool CaseSensitive; ///< Whether to perform case-sensitive comparisons.
> +  bool CaseSensitive;
> +
> +  /// \brief Whether to use to use the value of 'external-contents' for the
> +  /// names of files.  This global value is overridable on a per-file basis.
> +  bool UseExternalNames;
>    /// @}
> 
>    friend class VFSFromYAMLParser;
> 
>  private:
>    VFSFromYAML(IntrusiveRefCntPtr<FileSystem> ExternalFS)
> -      : ExternalFS(ExternalFS), CaseSensitive(true) {}
> +      : ExternalFS(ExternalFS), CaseSensitive(true), UseExternalNames(true) {}
> 
>    /// \brief Looks up \p Path in \c Roots.
>    ErrorOr<Entry *> lookupPath(const Twine &Path);
> @@ -446,7 +461,8 @@ class VFSFromYAMLParser {
>        KeyStatusPair("name", true),
>        KeyStatusPair("type", true),
>        KeyStatusPair("contents", false),
> -      KeyStatusPair("external-contents", false)
> +      KeyStatusPair("external-contents", false),
> +      KeyStatusPair("use-external-name", false),
>      };
> 
>      DenseMap<StringRef, KeyStatus> Keys(
> @@ -456,6 +472,7 @@ class VFSFromYAMLParser {
>      std::vector<Entry *> EntryArrayContents;
>      std::string ExternalContentsPath;
>      std::string Name;
> +    FileEntry::NameKind UseExternalName = FileEntry::NK_NotSet;
>      EntryKind Kind;
> 
>      for (yaml::MappingNode::iterator I = M->begin(), E = M->end(); I != E;
> @@ -519,6 +536,11 @@ class VFSFromYAMLParser {
>          if (!parseScalarString(I->getValue(), Value, Buffer))
>            return NULL;
>          ExternalContentsPath = Value;
> +      } else if (Key == "use-external-name") {
> +        bool Val;
> +        if (!parseScalarBool(I->getValue(), Val))
> +          return NULL;
> +        UseExternalName = Val ? FileEntry::NK_External : FileEntry::NK_Virtual;
>        } else {
>          llvm_unreachable("key missing from Keys");
>        }
> @@ -535,6 +557,12 @@ class VFSFromYAMLParser {
>      if (!checkMissingKeys(N, Keys))
>        return NULL;
> 
> +    // check invalid configuration
> +    if (Kind == EK_Directory && UseExternalName != FileEntry::NK_NotSet) {
> 
> [Found by Coverity]
> 
> If the YAML entry has a name field and a contents field but no kind field, we can reach this line with Kind uninitialized, resulting in UB.

This should be caught right above here with “checkMissingKeys”.  The field “type” which we use to set “Kind” is a required field.  False positive?

Ben

>  
> +      error(N, "'use-external-name' is not supported for directories");
> +      return NULL;
> +    }
> +
>      // Remove trailing slash(es)
>      StringRef Trimmed(Name);
>      while (Trimmed.size() > 1 && sys::path::is_separator(Trimmed.back()))
> @@ -545,7 +573,8 @@ class VFSFromYAMLParser {
>      Entry *Result = 0;
>      switch (Kind) {
>      case EK_File:
> -      Result = new FileEntry(LastComponent, llvm_move(ExternalContentsPath));
> +      Result = new FileEntry(LastComponent, llvm_move(ExternalContentsPath),
> +                             UseExternalName);
>        break;
>      case EK_Directory:
>        Result = new DirectoryEntry(LastComponent, llvm_move(EntryArrayContents),
> @@ -583,6 +612,7 @@ public:
>      KeyStatusPair Fields[] = {
>        KeyStatusPair("version", true),
>        KeyStatusPair("case-sensitive", false),
> +      KeyStatusPair("use-external-names", false),
>        KeyStatusPair("roots", true),
>      };
> 
> @@ -635,6 +665,9 @@ public:
>        } else if (Key == "case-sensitive") {
>          if (!parseScalarBool(I->getValue(), FS->CaseSensitive))
>            return false;
> +      } else if (Key == "use-external-names") {
> +        if (!parseScalarBool(I->getValue(), FS->UseExternalNames))
> +          return false;
>        } else {
>          llvm_unreachable("key missing from Keys");
>        }
> @@ -736,17 +769,15 @@ ErrorOr<Status> VFSFromYAML::status(cons
>    std::string PathStr(Path.str());
>    if (FileEntry *F = dyn_cast<FileEntry>(*Result)) {
>      ErrorOr<Status> S = ExternalFS->status(F->getExternalContentsPath());
> -    if (S) {
> -      assert(S->getName() == S->getExternalName() &&
> -             S->getName() == F->getExternalContentsPath());
> +    assert(!S || S->getName() == F->getExternalContentsPath());
> +    if (S && (F->useName() == FileEntry::NK_Virtual ||
> +              (F->useName() == FileEntry::NK_NotSet && !UseExternalNames)))
>        S->setName(PathStr);
> -    }
>      return S;
>    } else { // directory
>      DirectoryEntry *DE = cast<DirectoryEntry>(*Result);
>      Status S = DE->getStatus();
>      S.setName(PathStr);
> -    S.setExternalName(PathStr);
>      return S;
>    }
>  }
> 
> Modified: cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp?rev=202329&r1=202328&r2=202329&view=diff <http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp?rev=202329&r1=202328&r2=202329&view=diff>
> ==============================================================================
> --- cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp (original)
> +++ cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp Wed Feb 26 18:25:12 2014
> @@ -290,8 +290,7 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
>    // file
>    ErrorOr<vfs::Status> S = O->status("/file1");
>    ASSERT_EQ(errc::success, S.getError());
> -  EXPECT_EQ("/file1", S->getName());
> -  EXPECT_EQ("/foo/bar/a", S->getExternalName());
> +  EXPECT_EQ("/foo/bar/a", S->getName());
> 
>    ErrorOr<vfs::Status> SLower = O->status("/foo/bar/a");
>    EXPECT_EQ("/foo/bar/a", SLower->getName());
> @@ -467,6 +466,57 @@ TEST_F(VFSFromYAMLTest, IllegalVFSFile)
>    EXPECT_EQ(24, NumDiagnostics);
>  }
> 
> +TEST_F(VFSFromYAMLTest, UseExternalName) {
> +  IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
> +  Lower->addRegularFile("/external/file");
> +
> +  IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLString(
> +      "{ 'roots': [\n"
> +      "  { 'type': 'file', 'name': '/A',\n"
> +      "    'external-contents': '/external/file'\n"
> +      "  },\n"
> +      "  { 'type': 'file', 'name': '/B',\n"
> +      "    'use-external-name': true,\n"
> +      "    'external-contents': '/external/file'\n"
> +      "  },\n"
> +      "  { 'type': 'file', 'name': '/C',\n"
> +      "    'use-external-name': false,\n"
> +      "    'external-contents': '/external/file'\n"
> +      "  }\n"
> +      "] }", Lower);
> +  ASSERT_TRUE(NULL != FS.getPtr());
> +
> +  // default true
> +  EXPECT_EQ("/external/file", FS->status("/A")->getName());
> +  // explicit
> +  EXPECT_EQ("/external/file", FS->status("/B")->getName());
> +  EXPECT_EQ("/C", FS->status("/C")->getName());
> +
> +  // global configuration
> +  FS = getFromYAMLString(
> +      "{ 'use-external-names': false,\n"
> +      "  'roots': [\n"
> +      "  { 'type': 'file', 'name': '/A',\n"
> +      "    'external-contents': '/external/file'\n"
> +      "  },\n"
> +      "  { 'type': 'file', 'name': '/B',\n"
> +      "    'use-external-name': true,\n"
> +      "    'external-contents': '/external/file'\n"
> +      "  },\n"
> +      "  { 'type': 'file', 'name': '/C',\n"
> +      "    'use-external-name': false,\n"
> +      "    'external-contents': '/external/file'\n"
> +      "  }\n"
> +      "] }", Lower);
> +  ASSERT_TRUE(NULL != FS.getPtr());
> +
> +  // default
> +  EXPECT_EQ("/A", FS->status("/A")->getName());
> +  // explicit
> +  EXPECT_EQ("/external/file", FS->status("/B")->getName());
> +  EXPECT_EQ("/C", FS->status("/C")->getName());
> +}
> +
>  TEST_F(VFSFromYAMLTest, MultiComponentPath) {
>    IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
>    Lower->addRegularFile("/other");
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits <http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160720/63ed5228/attachment-0001.html>


More information about the cfe-commits mailing list