r374006 - Reland 'Add VFS support for sanitizers' blacklist'
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 9 07:19:53 PDT 2019
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191009/f513f85f/attachment-0001.html>
More information about the cfe-commits
mailing list