r243597 - Avoid failure to canonicalize '..'.

[Richard Smith]

On Wed, Jul 29, 2015 at 5:26 PM, Sean Silva <chisophugis at gmail.com> wrote:

> Author: silvas
> Date: Wed Jul 29 19:26:34 2015
> New Revision: 243597
>
> URL: http://llvm.org/viewvc/llvm-project?rev=243597&view=rev
> Log:
> Avoid failure to canonicalize '..'.
>
> Also fix completely broken and untested code which was hiding the
> primary bug. The !LLVM_ON_UNIX branch of the ifdef was actually a no-op.
>
> I ran into this in the wild. It was causing failures in our SDK build.
>
> Ideally we'd have a perfect llvm::sys::fs::canonical, but at least this
> is a step in the right direction, and fixes an obviously broken case.
> In some sense the test case I've added here is an integration test. We
> should have these routines thoroughly unit tested in llvm::sys::fs.
>

This may be correct for the !LLVM_ON_UNIX case (I'm not sure whether the
path foo/bar/.. is always the same as foo on Windows), but it's wrong for
the LLVM_ON_UNIX case. Please revert (except for the bugfix in
getCanonicalName); the right fix is to fix the places that are introducing
the unwanted .. path components.

Added:
>     cfe/trunk/test/Modules/Inputs/module-map-path-hash/
>     cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h
>     cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap
>     cfe/trunk/test/Modules/module-map-path-hash.cpp
> Modified:
>     cfe/trunk/lib/Basic/FileManager.cpp
>
> Modified: cfe/trunk/lib/Basic/FileManager.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=243597&r1=243596&r2=243597&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
> +++ cfe/trunk/lib/Basic/FileManager.cpp Wed Jul 29 19:26:34 2015
> @@ -514,7 +514,7 @@ void FileManager::modifyFileEntry(FileEn
>    File->ModTime = ModificationTime;
>  }
>
> -/// Remove '.' path components from the given absolute path.
> +/// Remove '.' and '..' path components from the given absolute path.
>  /// \return \c true if any changes were made.
>  // FIXME: Move this to llvm::sys::path.
>  bool FileManager::removeDotPaths(SmallVectorImpl<char> &Path) {
> @@ -525,24 +525,24 @@ bool FileManager::removeDotPaths(SmallVe
>
>    // Skip the root path, then look for traversal in the components.
>    StringRef Rel = path::relative_path(P);
> -  bool AnyDots = false;
>    for (StringRef C : llvm::make_range(path::begin(Rel), path::end(Rel))) {
> -    if (C == ".") {
> -      AnyDots = true;
> +    if (C == ".")
> +      continue;
> +    if (C == "..") {
> +      if (!ComponentStack.empty())
> +        ComponentStack.pop_back();
>        continue;
>      }
>      ComponentStack.push_back(C);
>    }
>
> -  if (!AnyDots)
> -    return false;
> -
>    SmallString<256> Buffer = path::root_path(P);
>    for (StringRef C : ComponentStack)
>      path::append(Buffer, C);
>
> +  bool Changed = (Path != Buffer);
>    Path.swap(Buffer);
> -  return true;
> +  return Changed;
>  }
>
>  StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
> @@ -567,6 +567,9 @@ StringRef FileManager::getCanonicalName(
>    llvm::sys::fs::make_absolute(CanonicalNameBuf);
>    llvm::sys::path::native(CanonicalNameBuf);
>    removeDotPaths(CanonicalNameBuf);
> +  char *Mem =
> CanonicalNameStorage.Allocate<char>(CanonicalNameBuf.size());
> +  memcpy(Mem, CanonicalNameBuf.data(), CanonicalNameBuf.size());
> +  CanonicalName = StringRef(Mem, CanonicalNameBuf.size());
>  #endif
>
>    CanonicalDirNames.insert(std::make_pair(Dir, CanonicalName));
>
> Added: cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h?rev=243597&view=auto
>
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h (added)
> +++ cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h Wed Jul 29
> 19:26:34 2015
> @@ -0,0 +1,2 @@
> +#pragma once
> +int a = 42;
>
> Added: cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap?rev=243597&view=auto
>
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap
> (added)
> +++ cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap
> Wed Jul 29 19:26:34 2015
> @@ -0,0 +1,3 @@
> +module a {
> +  header "a.h"
> +}
>
> Added: cfe/trunk/test/Modules/module-map-path-hash.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/module-map-path-hash.cpp?rev=243597&view=auto
>
> ==============================================================================
> --- cfe/trunk/test/Modules/module-map-path-hash.cpp (added)
> +++ cfe/trunk/test/Modules/module-map-path-hash.cpp Wed Jul 29 19:26:34
> 2015
> @@ -0,0 +1,9 @@
> +// rm -rf %t
> +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++ -Rmodule-build
> -I%S/Inputs/module-map-path-hash -fmodules-cache-path=%t -fsyntax-only %s
> +// xUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++ -Rmodule-build
> -I%S/Inputs//module-map-path-hash -fmodules-cache-path=%t -fsyntax-only %s
> 2>&1 | FileCheck -allow-empty %s
> +// xUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++ -Rmodule-build
> -I%S/Inputs/./module-map-path-hash -fmodules-cache-path=%t -fsyntax-only %s
> 2>&1 | FileCheck -allow-empty %s
>

Did you mean to commit with disabled RUN lines here?


> +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++ -Rmodule-build
> -I%S/Inputs/../Inputs/module-map-path-hash -fmodules-cache-path=%t
> -fsyntax-only %s 2>&1 | FileCheck -allow-empty %s
> +
> +#include "a.h"
> +
> +// CHECK-NOT: remark: building module
>
>
> _______________________________________________
> 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/20150729/0526e101/attachment.html>