On Wed, Jul 29, 2015 at 6:27 PM, Sean Silva <[email protected]> wrote:
> 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. > Thank you, that seems fine to me. > -- 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
