r243597 - Avoid failure to canonicalize '..'.
Richard Smith
richard at metafoo.co.uk
Wed Jul 29 18:16:28 PDT 2015
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?
> -- 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/c407febf/attachment.html>
More information about the cfe-commits
mailing list