On Wed, Jul 29, 2015 at 6:16 PM, Richard Smith <richard@metafoo.co.uk> wrote:

On Wed, Jul 29, 2015 at 6:13 PM, Sean Silva <chisophugis@gmail.com> wrote:

On Wed, Jul 29, 2015 at 6:11 PM, Richard Smith <richard@metafoo.co.uk> wrote:

On Wed, Jul 29, 2015 at 5:26 PM, Sean Silva <chisophugis@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.

I ran into this in the wild where they came from users.

So why do you want to remove them? (Note: I'm OK with removing blah/.. components on Windows, if indeed that is a correct thing to do there. But I'm not OK with it on Unix-like systems, where it's not correct in general.) Which of the three callers of this function did you need this change for?

As of r243600 we should only be hitting the '..' part of this in the !LLVM_ON_UNIX branch.

-- Sean Silva

 

 

-- Sean Silva

 

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@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits