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> 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> wrote: > >> Reverted in r374151. >> >> On Wed, Oct 9, 2019 at 11:03 AM Ilya Biryukov <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> 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 >>>> 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 >>>> >>>> Differential Revision: 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 >>>> >>>> ============================================================================== >>>> --- 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 >>>> >>>> ============================================================================== >>>> --- 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 >>>> >>>> ============================================================================== >>>> --- 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 >>>> 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 >> 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
