Ok, no worries. I understand the reason for the revert and will rewrite the patch.
> On Oct 9, 2019, at 7:19 AM, Ilya Biryukov <ibiryu...@google.com> wrote: > > Fair enough, sorry about that. > > 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. > 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. > A simple unit-test with InMemoryFS would break right away, so the failing > case is pretty obvious. > > 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. > I would suggest doing another round of review on this, happy to take part too. > > On Wed, Oct 9, 2019 at 3:55 PM Nico Weber <tha...@chromium.org > <mailto:tha...@chromium.org>> wrote: > 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. > > On Wed, Oct 9, 2019 at 5:38 AM Ilya Biryukov via cfe-commits > <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote: > Reverted in r374151. > > On Wed, Oct 9, 2019 at 11:03 AM Ilya Biryukov <ibiryu...@google.com > <mailto:ibiryu...@google.com>> wrote: > Hi Jan, > > This patch seems to assume VFS will only re-map some paths, but still point > into the physical filesystem. > This may not be the case, e.g. in unit tests. > > This also manages to break some of our internal clang-tidy integrations for > obscure reasons. > > Can we instead just pass VFS instance to the SanitizerBlacklist and use > relative paths? > > We might also need to revert the patch to unbreak our release, sorry about > the inconvenience. > > On Tue, Oct 8, 2019 at 3:11 AM Jan Korous via cfe-commits > <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote: > Author: jkorous > Date: Mon Oct 7 18:13:17 2019 > New Revision: 374006 > > URL: http://llvm.org/viewvc/llvm-project?rev=374006&view=rev > <http://llvm.org/viewvc/llvm-project?rev=374006&view=rev> > Log: > Reland 'Add VFS support for sanitizers' blacklist' > > The original patch broke the test for Windows. > Trying to fix as per Reid's suggestions outlined here: > https://reviews.llvm.org/rC371663 <https://reviews.llvm.org/rC371663> > > Differential Revision: https://reviews.llvm.org/D67742 > <https://reviews.llvm.org/D67742> > > Added: > cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml > Modified: > cfe/trunk/lib/AST/ASTContext.cpp > cfe/trunk/test/CodeGen/ubsan-blacklist.c > > Modified: cfe/trunk/lib/AST/ASTContext.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=374006&r1=374005&r2=374006&view=diff > > <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=374006&r1=374005&r2=374006&view=diff> > ============================================================================== > --- cfe/trunk/lib/AST/ASTContext.cpp (original) > +++ cfe/trunk/lib/AST/ASTContext.cpp Mon Oct 7 18:13:17 2019 > @@ -72,6 +72,7 @@ > #include "llvm/ADT/PointerUnion.h" > #include "llvm/ADT/STLExtras.h" > #include "llvm/ADT/SmallPtrSet.h" > +#include "llvm/ADT/SmallString.h" > #include "llvm/ADT/SmallVector.h" > #include "llvm/ADT/StringExtras.h" > #include "llvm/ADT/StringRef.h" > @@ -81,6 +82,7 @@ > #include "llvm/Support/Compiler.h" > #include "llvm/Support/ErrorHandling.h" > #include "llvm/Support/MathExtras.h" > +#include "llvm/Support/VirtualFileSystem.h" > #include "llvm/Support/raw_ostream.h" > #include <algorithm> > #include <cassert> > @@ -826,6 +828,18 @@ static bool isAddrSpaceMapManglingEnable > llvm_unreachable("getAddressSpaceMapMangling() doesn't cover anything."); > } > > +static std::vector<std::string> > +getRealPaths(llvm::vfs::FileSystem &VFS, llvm::ArrayRef<std::string> Paths) { > + std::vector<std::string> Result; > + llvm::SmallString<128> Buffer; > + for (const auto &File : Paths) { > + if (std::error_code EC = VFS.getRealPath(File, Buffer)) > + llvm::report_fatal_error("can't open file '" + File + "': " + > EC.message()); > + Result.push_back(Buffer.str()); > + } > + return Result; > +} > + > ASTContext::ASTContext(LangOptions &LOpts, SourceManager &SM, > IdentifierTable &idents, SelectorTable &sels, > Builtin::Context &builtins) > @@ -833,7 +847,10 @@ ASTContext::ASTContext(LangOptions &LOpt > TemplateSpecializationTypes(this_()), > DependentTemplateSpecializationTypes(this_()), > SubstTemplateTemplateParmPacks(this_()), SourceMgr(SM), > LangOpts(LOpts), > - SanitizerBL(new SanitizerBlacklist(LangOpts.SanitizerBlacklistFiles, > SM)), > + SanitizerBL(new SanitizerBlacklist( > + getRealPaths(SM.getFileManager().getVirtualFileSystem(), > + LangOpts.SanitizerBlacklistFiles), > + SM)), > XRayFilter(new XRayFunctionFilter(LangOpts.XRayAlwaysInstrumentFiles, > LangOpts.XRayNeverInstrumentFiles, > LangOpts.XRayAttrListFiles, SM)), > > Added: cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml?rev=374006&view=auto > > <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml?rev=374006&view=auto> > ============================================================================== > --- cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml (added) > +++ cfe/trunk/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml Mon Oct > 7 18:13:17 2019 > @@ -0,0 +1,15 @@ > +{ > + 'version': 0, > + 'roots': [ > + { 'name': '@DIR@', 'type': 'directory', > + 'contents': [ > + { 'name': 'only-virtual-file.blacklist', 'type': 'file', > + 'external-contents': '@REAL_FILE@' > + }, > + { 'name': 'invalid-virtual-file.blacklist', 'type': 'file', > + 'external-contents': '@NONEXISTENT_FILE@' > + } > + ] > + } > + ] > +} > > Modified: cfe/trunk/test/CodeGen/ubsan-blacklist.c > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ubsan-blacklist.c?rev=374006&r1=374005&r2=374006&view=diff > > <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ubsan-blacklist.c?rev=374006&r1=374005&r2=374006&view=diff> > ============================================================================== > --- cfe/trunk/test/CodeGen/ubsan-blacklist.c (original) > +++ cfe/trunk/test/CodeGen/ubsan-blacklist.c Mon Oct 7 18:13:17 2019 > @@ -5,6 +5,17 @@ > // RUN: %clang_cc1 -fsanitize=unsigned-integer-overflow > -fsanitize-blacklist=%t-func.blacklist -emit-llvm %s -o - | FileCheck %s > --check-prefix=FUNC > // RUN: %clang_cc1 -fsanitize=unsigned-integer-overflow > -fsanitize-blacklist=%t-file.blacklist -emit-llvm %s -o - | FileCheck %s > --check-prefix=FILE > > +// RUN: rm -f %t-vfsoverlay.yaml > +// RUN: rm -f %t-nonexistent.blacklist > +// 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 > +// 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 > + > +// 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 > +// INVALID-MAPPED-FILE: invalid-virtual-file.blacklist': No such file or > directory > + > +// 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 > +// INVALID: nonexistent.blacklist': No such file or directory > + > unsigned i; > > // DEFAULT: @hash > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > <https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits> > > > -- > Regards, > Ilya Biryukov > > > -- > Regards, > Ilya Biryukov > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > <https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits> > > > -- > Regards, > Ilya Biryukov
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
