<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Ok, no worries. I understand the reason for the revert and will rewrite the patch.<br class=""><div class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Oct 9, 2019, at 7:19 AM, Ilya Biryukov <<a href="mailto:ibiryukov@google.com" class="">ibiryukov@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Fair enough, sorry about that.<div class=""><br class=""></div><div class="">Nevertheless, I wanted to re-raise concerns about the approach taken in the patch. It seems to assume invariants about VFS that don't necessarily hold for filesystems outside those tested in upstream LLVM.<div class="">PTAL at the original patch - calling `getRealPath` on the FS and later using real-file-system with the results assumes the passed VFS is an overlay over the physical filesystem.</div><div class="">A simple unit-test with InMemoryFS would break right away, so the failing case is pretty obvious.</div><div class=""><br class=""></div><div class="">There's an obvious alternative: just pass VFS to SanitizerBlacklist (in fact, it already gets a source manager, which has a link to the VFS) and use it for file accesses.</div></div><div class="">I would suggest doing another round of review on this, happy to take part too.</div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Oct 9, 2019 at 3:55 PM Nico Weber <<a href="mailto:thakis@chromium.org" class="">thakis@chromium.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr" class="">FWIW reverting a patch for it breaking some internal system seems like poor form to me. It's really hard to reland in that case. Please make a reduced repro next time.</div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Oct 9, 2019 at 5:38 AM Ilya Biryukov via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" class="">cfe-commits@lists.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr" class="">Reverted in r374151.</div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Oct 9, 2019 at 11:03 AM Ilya Biryukov <<a href="mailto:ibiryukov@google.com" target="_blank" class="">ibiryukov@google.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr" class="">Hi Jan,<div class=""><br class=""></div><div class="">This patch seems to assume VFS will only re-map some paths, but still point into the physical filesystem.</div><div class="">This may not be the case, e.g. in unit tests.</div><div class=""><br class=""></div><div class="">This also manages to break some of our internal clang-tidy integrations for obscure reasons.</div><div class=""><br class=""></div><div class="">Can we instead just pass VFS instance to the SanitizerBlacklist and use relative paths?</div><div class=""><br class=""></div><div class="">We might also need to revert the patch to unbreak our release, sorry about the inconvenience.</div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 8, 2019 at 3:11 AM Jan Korous via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" class="">cfe-commits@lists.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: jkorous<br class="">
Date: Mon Oct 7 18:13:17 2019<br class="">
New Revision: 374006<br class="">
<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=374006&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=374006&view=rev</a><br class="">
Log:<br class="">
Reland 'Add VFS support for sanitizers' blacklist'<br class="">
<br class="">
The original patch broke the test for Windows.<br class="">
Trying to fix as per Reid's suggestions outlined here:<br class="">
<a href="https://reviews.llvm.org/rC371663" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/rC371663</a><br class="">
<br class="">
Differential Revision: <a href="https://reviews.llvm.org/D67742" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D67742</a><br class="">
<br class="">
Added:<br class="">
cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml<br class="">
Modified:<br class="">
cfe/trunk/lib/AST/ASTContext.cpp<br class="">
cfe/trunk/test/CodeGen/ubsan-blacklist.c<br class="">
<br class="">
Modified: cfe/trunk/lib/AST/ASTContext.cpp<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=374006&r1=374005&r2=374006&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=374006&r1=374005&r2=374006&view=diff</a><br class="">
==============================================================================<br class="">
--- cfe/trunk/lib/AST/ASTContext.cpp (original)<br class="">
+++ cfe/trunk/lib/AST/ASTContext.cpp Mon Oct 7 18:13:17 2019<br class="">
@@ -72,6 +72,7 @@<br class="">
#include "llvm/ADT/PointerUnion.h"<br class="">
#include "llvm/ADT/STLExtras.h"<br class="">
#include "llvm/ADT/SmallPtrSet.h"<br class="">
+#include "llvm/ADT/SmallString.h"<br class="">
#include "llvm/ADT/SmallVector.h"<br class="">
#include "llvm/ADT/StringExtras.h"<br class="">
#include "llvm/ADT/StringRef.h"<br class="">
@@ -81,6 +82,7 @@<br class="">
#include "llvm/Support/Compiler.h"<br class="">
#include "llvm/Support/ErrorHandling.h"<br class="">
#include "llvm/Support/MathExtras.h"<br class="">
+#include "llvm/Support/VirtualFileSystem.h"<br class="">
#include "llvm/Support/raw_ostream.h"<br class="">
#include <algorithm><br class="">
#include <cassert><br class="">
@@ -826,6 +828,18 @@ static bool isAddrSpaceMapManglingEnable<br class="">
llvm_unreachable("getAddressSpaceMapMangling() doesn't cover anything.");<br class="">
}<br class="">
<br class="">
+static std::vector<std::string><br class="">
+getRealPaths(llvm::vfs::FileSystem &VFS, llvm::ArrayRef<std::string> Paths) {<br class="">
+ std::vector<std::string> Result;<br class="">
+ llvm::SmallString<128> Buffer;<br class="">
+ for (const auto &File : Paths) {<br class="">
+ if (std::error_code EC = VFS.getRealPath(File, Buffer))<br class="">
+ llvm::report_fatal_error("can't open file '" + File + "': " + EC.message());<br class="">
+ Result.push_back(Buffer.str());<br class="">
+ }<br class="">
+ return Result;<br class="">
+}<br class="">
+<br class="">
ASTContext::ASTContext(LangOptions &LOpts, SourceManager &SM,<br class="">
IdentifierTable &idents, SelectorTable &sels,<br class="">
Builtin::Context &builtins)<br class="">
@@ -833,7 +847,10 @@ ASTContext::ASTContext(LangOptions &LOpt<br class="">
TemplateSpecializationTypes(this_()),<br class="">
DependentTemplateSpecializationTypes(this_()),<br class="">
SubstTemplateTemplateParmPacks(this_()), SourceMgr(SM), LangOpts(LOpts),<br class="">
- SanitizerBL(new SanitizerBlacklist(LangOpts.SanitizerBlacklistFiles, SM)),<br class="">
+ SanitizerBL(new SanitizerBlacklist(<br class="">
+ getRealPaths(SM.getFileManager().getVirtualFileSystem(),<br class="">
+ LangOpts.SanitizerBlacklistFiles),<br class="">
+ SM)),<br class="">
XRayFilter(new XRayFunctionFilter(LangOpts.XRayAlwaysInstrumentFiles,<br class="">
LangOpts.XRayNeverInstrumentFiles,<br class="">
LangOpts.XRayAttrListFiles, SM)),<br class="">
<br class="">
Added: cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml?rev=374006&view=auto" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml?rev=374006&view=auto</a><br class="">
==============================================================================<br class="">
--- cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml (added)<br class="">
+++ cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml Mon Oct 7 18:13:17 2019<br class="">
@@ -0,0 +1,15 @@<br class="">
+{<br class="">
+ 'version': 0,<br class="">
+ 'roots': [<br class="">
+ { 'name': '@DIR@', 'type': 'directory',<br class="">
+ 'contents': [<br class="">
+ { 'name': 'only-virtual-file.blacklist', 'type': 'file',<br class="">
+ 'external-contents': '@REAL_FILE@'<br class="">
+ },<br class="">
+ { 'name': 'invalid-virtual-file.blacklist', 'type': 'file',<br class="">
+ 'external-contents': '@NONEXISTENT_FILE@'<br class="">
+ }<br class="">
+ ]<br class="">
+ }<br class="">
+ ]<br class="">
+}<br class="">
<br class="">
Modified: cfe/trunk/test/CodeGen/ubsan-blacklist.c<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ubsan-blacklist.c?rev=374006&r1=374005&r2=374006&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ubsan-blacklist.c?rev=374006&r1=374005&r2=374006&view=diff</a><br class="">
==============================================================================<br class="">
--- cfe/trunk/test/CodeGen/ubsan-blacklist.c (original)<br class="">
+++ cfe/trunk/test/CodeGen/ubsan-blacklist.c Mon Oct 7 18:13:17 2019<br class="">
@@ -5,6 +5,17 @@<br class="">
// RUN: %clang_cc1 -fsanitize=unsigned-integer-overflow -fsanitize-blacklist=%t-func.blacklist -emit-llvm %s -o - | FileCheck %s --check-prefix=FUNC<br class="">
// RUN: %clang_cc1 -fsanitize=unsigned-integer-overflow -fsanitize-blacklist=%t-file.blacklist -emit-llvm %s -o - | FileCheck %s --check-prefix=FILE<br class="">
<br class="">
+// RUN: rm -f %t-vfsoverlay.yaml<br class="">
+// RUN: rm -f %t-nonexistent.blacklist<br class="">
+// RUN: sed -e "s|@DIR@|%/T|g" %S/Inputs/sanitizer-blacklist-vfsoverlay.yaml | sed -e "s|@REAL_FILE@|%/t-func.blacklist|g" | sed -e "s|@NONEXISTENT_FILE@|%/t-nonexistent.blacklist|g" > %t-vfsoverlay.yaml<br class="">
+// RUN: %clang_cc1 -fsanitize=unsigned-integer-overflow -ivfsoverlay %t-vfsoverlay.yaml -fsanitize-blacklist=%T/only-virtual-file.blacklist -emit-llvm %s -o - | FileCheck %s --check-prefix=FUNC<br class="">
+<br class="">
+// RUN: not %clang_cc1 -fsanitize=unsigned-integer-overflow -ivfsoverlay %t-vfsoverlay.yaml -fsanitize-blacklist=%T/invalid-virtual-file.blacklist -emit-llvm %s -o - 2>&1 | FileCheck %s --check-prefix=INVALID-MAPPED-FILE<br class="">
+// INVALID-MAPPED-FILE: invalid-virtual-file.blacklist': No such file or directory<br class="">
+<br class="">
+// RUN: not %clang_cc1 -fsanitize=unsigned-integer-overflow -ivfsoverlay %t-vfsoverlay.yaml -fsanitize-blacklist=%t-nonexistent.blacklist -emit-llvm %s -o - 2>&1 | FileCheck %s --check-prefix=INVALID<br class="">
+// INVALID: nonexistent.blacklist': No such file or directory<br class="">
+<br class="">
unsigned i;<br class="">
<br class="">
// DEFAULT: @hash<br class="">
<br class="">
<br class="">
_______________________________________________<br class="">
cfe-commits mailing list<br class="">
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" class="">cfe-commits@lists.llvm.org</a><br class="">
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank" class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br class="">
</blockquote></div><br clear="all" class=""><div class=""><br class=""></div>-- <br class=""><div dir="ltr" class=""><div dir="ltr" class=""><div class=""><div dir="ltr" class=""><div class="">Regards,</div><div class="">Ilya Biryukov</div></div></div></div></div>
</blockquote></div><br clear="all" class=""><div class=""><br class=""></div>-- <br class=""><div dir="ltr" class=""><div dir="ltr" class=""><div class=""><div dir="ltr" class=""><div class="">Regards,</div><div class="">Ilya Biryukov</div></div></div></div></div>
_______________________________________________<br class="">
cfe-commits mailing list<br class="">
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" class="">cfe-commits@lists.llvm.org</a><br class="">
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank" class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br class="">
</blockquote></div>
</blockquote></div><br clear="all" class=""><div class=""><br class=""></div>-- <br class=""><div dir="ltr" class="gmail_signature"><div dir="ltr" class=""><div class=""><div dir="ltr" class=""><div class="">Regards,</div><div class="">Ilya Biryukov</div></div></div></div></div>
</div></blockquote></div><br class=""></div></body></html>