On Wed, Jul 29, 2015 at 6:16 PM, Richard Smith <[email protected]> wrote:
> 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? > 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 >>>> [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
