r243597 - Avoid failure to canonicalize '..'.
Sean Silva
chisophugis at gmail.com
Wed Jul 29 18:27:48 PDT 2015
On Wed, Jul 29, 2015 at 6:16 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
> On Wed, Jul 29, 2015 at 6:13 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>> On Wed, Jul 29, 2015 at 6:11 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>>
>>> 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.
>>>
>>
>> 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 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/94d307c2/attachment.html>
More information about the cfe-commits
mailing list