On Wed, Jul 29, 2015 at 6:13 PM, Sean Silva <[email protected]> wrote:
> On Wed, Jul 29, 2015 at 6:11 PM, Richard Smith <[email protected]> > wrote: > >> On Wed, Jul 29, 2015 at 5:26 PM, Sean Silva <[email protected]> >> 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? > -- 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 >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
