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

Richard Smith via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 18 15:21:50 PDT 2016


On Wed, Feb 26, 2014 at 4:25 PM, Ben Langmuir <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
> 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
>
> ==============================================================================
> --- 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
>
> ==============================================================================
> --- 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.


> +      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
>
> ==============================================================================
> --- 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
> 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/20160718/2e38da3c/attachment-0001.html>


More information about the cfe-commits mailing list