r374006 - Reland 'Add VFS support for sanitizers' blacklist'
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 10 00:54:59 PDT 2019
Thanks, Jan. And one more time, sorry about the inconvenience.
On Thu, Oct 10, 2019 at 12:47 AM Jan Korous <jkorous at 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 <ibiryukov at 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 <thakis at 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 at lists.llvm.org> wrote:
>>
>>> Reverted in r374151.
>>>
>>> On Wed, Oct 9, 2019 at 11:03 AM Ilya Biryukov <ibiryukov at 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 at 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 at 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 at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>
> --
> Regards,
> Ilya Biryukov
>
>
>
--
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191010/0e0bf228/attachment.html>
More information about the cfe-commits
mailing list