[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: r243597 - Avoid failure to canonicalize '..'.
From: Richard Smith <richard () metafoo ! co ! uk>
Date: 2015-07-30 1:11:23
Message-ID: CAOfiQqmFprGmS3g4nv2k6gsZ7WQiS1oHP65h63vQBXf=qhxfjw () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Wed, Jul 29, 2015 at 5:26 PM, Sean Silva <chisophugis@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.
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@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
[Attachment #5 (text/html)]
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, \
Jul 29, 2015 at 5:26 PM, Sean Silva <span dir="ltr"><<a \
href="mailto:chisophugis@gmail.com" \
target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">Author: silvas<br>
Date: Wed Jul 29 19:26:34 2015<br>
New Revision: 243597<br>
<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_v \
iewvc_llvm-2Dproject-3Frev-3D243597-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rB \
PlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=YiMOo6p7tpIwCnb7tEHxVAqPEX9aS1Q9UXzUtAZzr68&s=BaC2aUdfE-HzK8629S9SQe1TsJq0SiPZ6dWwQa68dBQ&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=243597&view=rev</a><br>
Log:<br>
Avoid failure to canonicalize '..'.<br>
<br>
Also fix completely broken and untested code which was hiding the<br>
primary bug. The !LLVM_ON_UNIX branch of the ifdef was actually a \
no-op.<br> <br>
I ran into this in the wild. It was causing failures in our SDK build.<br>
<br>
Ideally we'd have a perfect llvm::sys::fs::canonical, but at least \
this<br> is a step in the right direction, and fixes an obviously broken \
case.<br> In some sense the test case I've added here is an integration \
test. We<br> should have these routines thoroughly unit tested in \
llvm::sys::fs.<br></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" \
style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> \
Added:<br> cfe/trunk/test/Modules/Inputs/module-map-path-hash/<br>
cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h<br>
cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap<br>
cfe/trunk/test/Modules/module-map-path-hash.cpp<br>
Modified:<br>
cfe/trunk/lib/Basic/FileManager.cpp<br>
<br>
Modified: cfe/trunk/lib/Basic/FileManager.cpp<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_v \
iewvc_llvm-2Dproject_cfe_trunk_lib_Basic_FileManager.cpp-3Frev-3D243597-26r1 \
-3D243596-26r2-3D243597-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BS \
qEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=YiMOo6p7tpIwCnb7tEHxVAqPEX9aS1Q9UXzUtAZzr68&s=Uhn1SMWa2ncGQgbsV7YAdy7LIIDRZjiLKQJtUJjUmC8&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/tru \
nk/lib/Basic/FileManager.cpp?rev=243597&r1=243596&r2=243597&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Basic/FileManager.cpp (original)<br>
+++ cfe/trunk/lib/Basic/FileManager.cpp Wed Jul 29 19:26:34 2015<br>
@@ -514,7 +514,7 @@ void FileManager::modifyFileEntry(FileEn<br>
File->ModTime = ModificationTime;<br>
}<br>
<br>
-/// Remove '.' path components from the given absolute path.<br>
+/// Remove '.' and '..' path components from the given \
absolute path.<br> /// \return \c true if any changes were made.<br>
// FIXME: Move this to llvm::sys::path.<br>
bool FileManager::removeDotPaths(SmallVectorImpl<char> &Path) \
{<br> @@ -525,24 +525,24 @@ bool FileManager::removeDotPaths(SmallVe<br>
<br>
// Skip the root path, then look for traversal in the components.<br>
StringRef Rel = path::relative_path(P);<br>
- bool AnyDots = false;<br>
for (StringRef C : llvm::make_range(path::begin(Rel), path::end(Rel))) \
{<br>
- if (C == ".") {<br>
- AnyDots = true;<br>
+ if (C == ".")<br>
+ continue;<br>
+ if (C == "..") {<br>
+ if (!ComponentStack.empty())<br>
+ ComponentStack.pop_back();<br>
continue;<br>
}<br>
ComponentStack.push_back(C);<br>
}<br>
<br>
- if (!AnyDots)<br>
- return false;<br>
-<br>
SmallString<256> Buffer = path::root_path(P);<br>
for (StringRef C : ComponentStack)<br>
path::append(Buffer, C);<br>
<br>
+ bool Changed = (Path != Buffer);<br>
Path.swap(Buffer);<br>
- return true;<br>
+ return Changed;<br>
}<br>
<br>
StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {<br>
@@ -567,6 +567,9 @@ StringRef FileManager::getCanonicalName(<br>
llvm::sys::fs::make_absolute(CanonicalNameBuf);<br>
llvm::sys::path::native(CanonicalNameBuf);<br>
removeDotPaths(CanonicalNameBuf);<br>
+ char *Mem = CanonicalNameStorage.Allocate<char>(CanonicalNameBuf.size());<br>
+ memcpy(Mem, CanonicalNameBuf.data(), CanonicalNameBuf.size());<br>
+ CanonicalName = StringRef(Mem, CanonicalNameBuf.size());<br>
#endif<br>
<br>
CanonicalDirNames.insert(std::make_pair(Dir, CanonicalName));<br>
<br>
Added: cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_v \
iewvc_llvm-2Dproject_cfe_trunk_test_Modules_Inputs_module-2Dmap-2Dpath-2Dhas \
h_a.h-3Frev-3D243597-26view-3Dauto&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv \
9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=YiMOo6p7tpIwCnb7tEHxVAqPEX9aS1Q9UXzUtAZzr68&s=jCbbQj8I3rlm88pqhdigKfmG5vkbOqT0JFUxXEpFgmo&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h?rev=243597&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h (added)<br>
+++ cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h Wed Jul 29 \
19:26:34 2015<br> @@ -0,0 +1,2 @@<br>
+#pragma once<br>
+int a = 42;<br>
<br>
Added: cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_v \
iewvc_llvm-2Dproject_cfe_trunk_test_Modules_Inputs_module-2Dmap-2Dpath-2Dhas \
h_module.modulemap-3Frev-3D243597-26view-3Dauto&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBP \
lktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=YiMOo6p7tpIwCnb7tEHxVAqPEX9aS1Q9UXzUtAZzr68&s=0ezqa97k9BBbHVE580AQ_tv5CzHcyITUEPDG59IaHhM&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/tru \
nk/test/Modules/Inputs/module-map-path-hash/module.modulemap?rev=243597&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap \
(added)<br>
+++ cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap Wed \
Jul 29 19:26:34 2015<br> @@ -0,0 +1,3 @@<br>
+module a {<br>
+ header "a.h"<br>
+}<br>
<br>
Added: cfe/trunk/test/Modules/module-map-path-hash.cpp<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_v \
iewvc_llvm-2Dproject_cfe_trunk_test_Modules_module-2Dmap-2Dpath-2Dhash.cpp-3 \
Frev-3D243597-26view-3Dauto&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_ \
Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=YiMOo6p7tpIwCnb7tEHxVAqPEX9aS1Q9UXzUtAZzr68&s=95CRpBwaBzUGntGcVZnE3mOcnOwvvlwDuzYDf0ytx-U&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/module-map-path-hash.cpp?rev=243597&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/Modules/module-map-path-hash.cpp (added)<br>
+++ cfe/trunk/test/Modules/module-map-path-hash.cpp Wed Jul 29 19:26:34 \
2015<br> @@ -0,0 +1,9 @@<br>
+// rm -rf %t<br>
+// 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<br> +// 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<br> +// 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<br></blockquote><div><br></div><div>Did you mean to commit with disabled \
RUN lines here?</div><div> </div><blockquote class="gmail_quote" \
style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +// \
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<br> +<br>
+#include "a.h"<br>
+<br>
+// CHECK-NOT: remark: building module<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" \
rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic