pcc added a comment.
In https://reviews.llvm.org/D46403#1291697, @filcab wrote:
> Sorry to ressurect this review, but I have a few questions:
>
> - What kind of functions fail?
> - Are there bugzillas to track these?
> - How can a compiler expect to have blacklists for "all" its CFI clients?
>
filcab added a comment.
Sorry to ressurect this review, but I have a few questions:
- What kind of functions fail?
- Are there bugzillas to track these?
- How can a compiler expect to have blacklists for "all" its CFI clients? (I'm
ok with having a default for "most-used, well-known,
This revision was automatically updated to reflect the committed changes.
Closed by commit rC331674: [CFI] Force LLVM to die if the implicit blacklist
files cannot be found. (authored by pcc, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D46403
Files:
pcc accepted this revision.
pcc added a comment.
LGTM
https://reviews.llvm.org/D46403
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
cmtice updated this revision to Diff 145540.
cmtice added a comment.
Make cfi_blacklist.txt be an empty file.
https://reviews.llvm.org/D46403
Files:
lib/Driver/SanitizerArgs.cpp
test/Driver/Inputs/resource_dir_with_cfi_blacklist/share/cfi_blacklist.txt
test/Driver/fsanitize-blacklist.c
cmtice updated this revision to Diff 145537.
cmtice added a comment.
Make -resource-dir point to correct directory, in test case; move
cfi_blacklist.txt file to 'share' subdirectory in test resource dir.
https://reviews.llvm.org/D46403
Files:
lib/Driver/SanitizerArgs.cpp
pcc added inline comments.
Comment at: test/Frontend/dependency-gen.c:24
// RUN: echo "fun:foo" > %t.blacklist
-// RUN: %clang -MD -MF - %s -fsyntax-only -fsanitize=cfi-vcall -flto
-fvisibility=hidden -fsanitize-blacklist=%t.blacklist -I ./ | FileCheck
cmtice added a comment.
I'm not sure if I have commit access or not; Peter was working with me on
trying to commit the change.
https://reviews.llvm.org/D46403
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
vsk added a comment.
Do you have commit access? If not I'd be happy to land this for you.
Comment at:
test/Driver/Inputs/resource_dir_with_cfi_blacklist/cfi_blacklist.txt:19
+# in order to call std::allocator_traits::construct.
+fun:_ZNSt23_Sp_counted_ptr_inplace*
cmtice updated this revision to Diff 145524.
cmtice added a comment.
Fix test failure that my previous changes caused.
https://reviews.llvm.org/D46403
Files:
lib/Driver/SanitizerArgs.cpp
test/Driver/Inputs/resource_dir_with_cfi_blacklist/cfi_blacklist.txt
pcc accepted this revision.
pcc added a comment.
LGTM
https://reviews.llvm.org/D46403
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.
In https://reviews.llvm.org/D46403#1088759, @pcc wrote:
> We should be installing `compiler-rt/lib/cfi/cfi_blacklist.txt`, no?
Oh, I see. This is already taken care of, then.
@cmtice This looks
pcc added a comment.
We should be installing `compiler-rt/lib/cfi/cfi_blacklist.txt`, no?
https://reviews.llvm.org/D46403
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
cmtice added a comment.
vsk: Are you asking me to put together a cfi blacklist to ship in the resource
directory in the default install as part of this code review? Or is that
something you want to see in a different code reivew?
https://reviews.llvm.org/D46403
cmtice updated this revision to Diff 145266.
cmtice added a comment.
Added comment to change in source code.
https://reviews.llvm.org/D46403
Files:
lib/Driver/SanitizerArgs.cpp
test/Driver/fsanitize-blacklist.c
Index: test/Driver/fsanitize-blacklist.c
vsk added a comment.
One problem with this direction is that clang doesn't ship a cfi blacklist in
its default install, so, this leaves cfi users with stock toolchains to fend
for themselves. I think it'd be a good idea to ship a basic cfi blacklist in
clang's resource dir to avoid inadvertent
vsk added inline comments.
Comment at: lib/Driver/SanitizerArgs.cpp:118
BlacklistFiles.push_back(Path.str());
+else if (BL.Mask == CFI)
+ D.Diag(clang::diag::err_drv_no_such_file) << Path;
vsk wrote:
> CFI can be enabled with other sanitizers,
vsk added inline comments.
Comment at: lib/Driver/SanitizerArgs.cpp:118
BlacklistFiles.push_back(Path.str());
+else if (BL.Mask == CFI)
+ D.Diag(clang::diag::err_drv_no_such_file) << Path;
CFI can be enabled with other sanitizers, such as ubsan.
cmtice updated this revision to Diff 145243.
cmtice added a comment.
Updated the error to only occur for CFI blacklist, and added test case.
https://reviews.llvm.org/D46403
Files:
lib/Driver/SanitizerArgs.cpp
test/Driver/fsanitize-blacklist.c
Index: test/Driver/fsanitize-blacklist.c
vsk added a comment.
In https://reviews.llvm.org/D46403#1086923, @cmtice wrote:
> Ok, I'll work on making this CFI-specific and adding a test case.
Sounds good, thanks! I'd suggest modifying test/Driver/fsanitize-blacklist.c
for your purposes. It should be possible to pair an empty resource
cmtice added a comment.
Ok, I'll work on making this CFI-specific and adding a test case.
https://reviews.llvm.org/D46403
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
cmtice updated this revision to Diff 145073.
cmtice added a comment.
Tried to upload the diff from the correct location.
https://reviews.llvm.org/D46403
Files:
lib/Driver/SanitizerArgs.cpp
Index: lib/Driver/SanitizerArgs.cpp
vsk requested changes to this revision.
vsk added a comment.
This revision now requires changes to proceed.
Toolchain vendors aren't currently required to provide default blacklists for
every sanitizer clang supports. We don't ship a default ubsan blacklist on
macOS, so this patch would break
lebedev.ri added a comment.
Test?
Repository:
rC Clang
https://reviews.llvm.org/D46403
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri added a comment.
Also, please upload the correct patch, from the root of the repo, not from the
directory of the file.
Repository:
rC Clang
https://reviews.llvm.org/D46403
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
cmtice created this revision.
cmtice added a reviewer: pcc.
Herald added a subscriber: cfe-commits.
Currently LLVM CFI tries to use an implicit blacklist file, currently in
/usr/lib64/clang//share. If the file is not there, LLVM happily
continues, which causes CFI to add checks to
26 matches
Mail list logo
