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