Thanks, Jan. And one more time, sorry about the inconvenience. On Thu, Oct 10, 2019 at 12:47 AM Jan Korous <jkor...@apple.com> wrote:
> 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> 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 > > > -- Regards, Ilya Biryukov
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
