filcab added a comment.
Why the switch to `if` instead of a fully-covered switch/case?
In https://reviews.llvm.org/D29369#664426, @vsk wrote:
> In https://reviews.llvm.org/D29369#664366, @regehr wrote:
>
> > Out of curiosity, how many of these superfluous checks are not subsequently
> >
filcab accepted this revision.
filcab added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D30285
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
filcab added a comment.
Minor nits, now.
LGTM, but having someone more familiar with clang chime in would be great.
Comment at: lib/CodeGen/CGExprScalar.cpp:1700
case LangOptions::SOB_Trapping:
+if (getUnwidenedIntegerType(CGF.getContext(), E->getSubExpr()).hasValue())
filcab abandoned this revision.
filcab added a comment.
I will post a new patch, using the features from
https://reviews.llvm.org/rL289444.
https://reviews.llvm.org/D19667
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
This revision was automatically updated to reflect the committed changes.
Closed by commit rL289444: [clang] Version support for UBSan handlers (authored
by filcab).
Changed prior to commit:
https://reviews.llvm.org/D21695?vs=61817=81089#toc
Repository:
rL LLVM
filcab added a comment.
Ping!
https://reviews.llvm.org/D21695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
filcab accepted this revision.
filcab added a reviewer: filcab.
filcab added a comment.
This revision is now accepted and ready to land.
Since Richard has already LGTMed the previous version, and this is a trivial
change, I'll go ahead with committing.
https://reviews.llvm.org/D28242
This revision was automatically updated to reflect the committed changes.
Closed by commit rL291236: [ubsan] Minimize size of data for type_mismatch
(Redo of D19667) (authored by filcab).
Changed prior to commit:
https://reviews.llvm.org/D28242?vs=82913=83362#toc
Repository:
rL LLVM
filcab added a comment.
Missing a `sanitizer-ld.c` test for freebsd.
Comment at: include/clang/Basic/Attr.td:1849
+ GNU<"no_sanitize_memory">,
+ GNU<"no_sanitize_tbaa">];
let Subjects = SubjectList<[Function, GlobalVar], ErrorDiag,
filcab added a comment.
Please make the tests tighter using `CHECK-NEXT` when possible. Much easier if
later anyone needs to debug differences in IR.
Comment at: docs/UndefinedBehaviorSanitizer.rst:102
+ violating nullability rules does not result in undefined behavior,
filcab added inline comments.
Comment at: test/CodeGenCXX/ubsan-bitfields.cpp:39
+ // CHECK-NOT: !nosanitize
+ return s->e3;
+}
Add checks/check-nots to make sure the thing you don't want isn't emitted, not
just `!nosanitize`
The checks help document what
filcab added a comment.
LGTM since my issue is only an issue on ObjC platforms and it seems those are
the semantics it should have there.
Comment at: test/CodeGenObjC/ubsan-bool.m:26
+ // OBJC: [[ICMP:%.*]] = icmp ule i8 [[ASHR]], 1, !nosanitize
+ // OBJC: call void
filcab added inline comments.
Comment at: test/CodeGenObjC/ubsan-bool.m:26
+ // OBJC: [[ICMP:%.*]] = icmp ule i8 [[ASHR]], 1, !nosanitize
+ // OBJC: call void @__ubsan_handle_load_invalid_value
+
jroelofs wrote:
> vsk wrote:
> > jroelofs wrote:
> > > vsk
filcab added a comment.
Should we simply not have `ubsan_blacklist.txt` if it's empty?
Otherwise LGTM
https://reviews.llvm.org/D32047
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
filcab added a comment.
Splitting the attrloc from the useloc might make sense since we would be able
to emit attrloc just once. But I don't see why we need to store/load those
pointers in runtime instead of just caching the `Constant*` in
`CodeGenFunction`.
I'd also like to have some asserts
filcab added a comment.
In https://reviews.llvm.org/D34299#788152, @vsk wrote:
> The source locations aren't constants. The ubsan runtime uses a bit inside of
> source location structures as a flag. When an issue is diagnosed at a
> particular source location, that bit is atomically set. This
filcab added a comment.
In https://reviews.llvm.org/D34299#788427, @vsk wrote:
> I hope I've cleared this up, but: we need to store the source location
> constant _somewhere_, before we emit the return value check. That's because
> we can't infer which return location to use at compile time.
filcab added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:3854
+ const Twine ) {
+ Value *GEPVal = Builder.CreateInBoundsGEP(Ptr, IdxList, Name);
+
You're creating the GEP first (possibly triggering
filcab created this revision.
Also makes it pass on Darwin, if the default target triple is a Linux triple.
https://reviews.llvm.org/D38364
Files:
test/Modules/builtin-import.mm
Index: test/Modules/builtin-import.mm
===
---
filcab updated this revision to Diff 117002.
filcab added a comment.
Add umbrella-header-include-builtin.mm too
https://reviews.llvm.org/D38364
Files:
test/Modules/builtin-import.mm
test/Modules/umbrella-header-include-builtin.mm
Index: test/Modules/umbrella-header-include-builtin.mm
This revision was automatically updated to reflect the committed changes.
Closed by commit rL314524: Fix
Modules/{builtin-import.mm,umbrella-header-include-builtin.mm} to be able…
(authored by filcab).
Repository:
rL LLVM
https://reviews.llvm.org/D38364
Files:
filcab created this revision.
If LLVM_DEFAULT_TARGET_TRIPLE is a non-darwin triple, the file might
fail. This might allow us to take out the FIXME and REQUIRES lines, but I'd
rather let the bots double-check that the test is ok first.
https://reviews.llvm.org/D38354
Files:
filcab added a comment.
Ping!
https://reviews.llvm.org/D38354
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
filcab created this revision.
filcab added reviewers: rjmccall, kcc, rsmith.
The C++ Itanium ABI says:
No cookie is required if the new operator being used is ::operator
new[](size_t, void*).
We should only avoid poisoning the cookie if we're calling this
operator, not others. This is dealt
filcab added inline comments.
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1733
// The PS4 uses C++11 as the default C++ standard.
- if (T.isPS4())
-LangStd = LangStandard::lang_gnucxx11;
- else
-LangStd = LangStandard::lang_gnucxx98;
+
filcab added inline comments.
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1733
// The PS4 uses C++11 as the default C++ standard.
- if (T.isPS4())
-LangStd = LangStandard::lang_gnucxx11;
- else
-LangStd = LangStandard::lang_gnucxx98;
+
filcab accepted this revision.
filcab added a comment.
This revision is now accepted and ready to land.
LGTM
Comment at: test/Driver/rtti-options.cpp:13
// RUN: %clang -### -c -fno-rtti -frtti %s 2>&1 | FileCheck
-check-prefix=CHECK-RTTI %s
-// RUN: %clang -### -c -frtti
filcab added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6729
def err_no_dynamic_cast_with_fno_rtti : Error<
- "cannot use dynamic_cast with -fno-rtti">;
+ "use of dynamic_cast requires enabling RTTI">;
I'd prefer to have the way
filcab added a comment.
I have a minor nit + what Paul mentioned (missing a `-NOT` check). Otherwise
LGTM.
Comment at: lib/Driver/ToolChains/Clang.cpp:3690
- // Add runtime flag for PS4 when PGO or Coverage are enabled.
- if (RawTriple.isPS4CPU())
+ // Add runtime flag
filcab updated this revision to Diff 133389.
filcab added a comment.
Update commit message.
Repository:
rC Clang
https://reviews.llvm.org/D43013
Files:
include/clang/Driver/Options.td
include/clang/Frontend/CodeGenOptions.def
lib/CodeGen/ItaniumCXXABI.cpp
filcab added a comment.
In https://reviews.llvm.org/D43013#1001006, @rjmccall wrote:
> I don't understand why your description of this patch mentions the void*
> placement new[] operator. There's no cookie to poison for that operator.
Hah, sorry. In writing this commit log I used parts of
This revision was automatically updated to reflect the committed changes.
Closed by commit rC324884: ASan+operator new[]: Add an option for more thorough
operator new[] cookie… (authored by filcab, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D43013?vs=133389=133830#toc
filcab created this revision.
filcab added reviewers: rjmccall, kcc, rsmith.
The C++ Itanium ABI says:
No cookie is required if the new operator being used is ::operator
new[](size_t, void*).
This commit adds a flag to tell clang to poison all operator new[]
cookies.
A previous review was
This revision was automatically updated to reflect the committed changes.
Closed by commit rL321645: ASan+operator new[]: Fix operator new[] cookie
poisoning (authored by filcab, committed by ).
Repository:
rL LLVM
https://reviews.llvm.org/D41301
Files:
filcab created this revision.
filcab added reviewers: rjmccall, kcc, rsmith.
Repository:
rC Clang
https://reviews.llvm.org/D52615
Files:
include/clang/Driver/SanitizerArgs.h
lib/Driver/SanitizerArgs.cpp
test/Driver/fsanitize.c
Index: test/Driver/fsanitize.c
filcab added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:305
enum ImplicitConversionCheckKind : unsigned char {
-ICCK_IntegerTruncation = 0,
+ICCK_IntegerTruncation = 0, // Legacy, no longer used.
+ICCK_UnsignedIntegerTruncation = 1,
filcab added a subscriber: aizatsky.
filcab added a comment.
Sorry about that. I’m away today but I don’t think you’ve answered my
questions about “why not support standalone UBSan in tests”. Sorry if I
missed the answers if you did. Will review the latest tomorrow.
Thank you,
Filipe
filcab added a comment.
In https://reviews.llvm.org/D52615#1254567, @rsmith wrote:
> This seems like an unreasonably long flag name. Can you try to find a shorter
> name for it?
`-fsanitize-poison-extra-operator-new`?
Not as explicit, but maybe ok if documented?
Thank you,
Filipe
filcab added a comment.
Ping!
Thank you,
Filipe
Repository:
rC Clang
https://reviews.llvm.org/D52615
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
filcab added a comment.
In https://reviews.llvm.org/D52615#1276938, @rjmccall wrote:
> In https://reviews.llvm.org/D52615#1275946, @filcab wrote:
>
> > In https://reviews.llvm.org/D52615#1272725, @rjmccall wrote:
> >
> > > In https://reviews.llvm.org/D52615#1266320, @filcab wrote:
> > >
> > > >
filcab updated this revision to Diff 171660.
filcab added a comment.
Update with name change, using rjmccall's suggestion
Repository:
rC Clang
https://reviews.llvm.org/D52615
Files:
include/clang/Driver/Options.td
include/clang/Driver/SanitizerArgs.h
filcab updated this revision to Diff 171661.
filcab added a comment.
Missed the change in some places
Repository:
rC Clang
https://reviews.llvm.org/D52615
Files:
docs/ClangCommandLineReference.rst
docs/UsersManual.rst
include/clang/Driver/Options.td
filcab updated this revision to Diff 172149.
filcab added a comment.
Expanded a bit more on the documentation, mentioning that user code might be
technically allowed to access those array cookies, but that users might not
want to allow it.
Repository:
rC Clang
filcab added a comment.
In https://reviews.llvm.org/D52615#1278000, @rjmccall wrote:
> So, three points:
>
> - That's not class-member-specific; you can have a placement `operator new[]`
> at global scope that isn't the special `void*` placement operator and
> therefore still has a cookie, and
filcab added a comment.
In https://reviews.llvm.org/D52615#1272725, @rjmccall wrote:
> In https://reviews.llvm.org/D52615#1266320, @filcab wrote:
>
> > In https://reviews.llvm.org/D52615#1254567, @rsmith wrote:
> >
> > > This seems like an unreasonably long flag name. Can you try to find a
> >
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346001: Change
-fsanitize-address-poison-class-member-array-new-cookie to -fsanitize…
(authored by filcab, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
filcab added a comment.
Thanks for the review.
Unfortunately, I forgot to edit the commit message. Let's hope no one gets too
confused (phab reviews will be up anyway, so should be easy to figure out).
Filipe
Repository:
rL LLVM
https://reviews.llvm.org/D52615
This revision was automatically updated to reflect the committed changes.
Closed by commit rC346001: Change
-fsanitize-address-poison-class-member-array-new-cookie to -fsanitize…
(authored by filcab, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D52615?vs=172376=172392#toc
filcab updated this revision to Diff 172376.
filcab added a comment.
Reworded with feedback from the review.
Repository:
rC Clang
https://reviews.llvm.org/D52615
Files:
docs/ClangCommandLineReference.rst
docs/UsersManual.rst
include/clang/Driver/Options.td
filcab updated this revision to Diff 172179.
filcab added a comment.
Expand yet a bit more.
Repository:
rC Clang
https://reviews.llvm.org/D52615
Files:
docs/ClangCommandLineReference.rst
docs/UsersManual.rst
include/clang/Driver/Options.td
include/clang/Driver/SanitizerArgs.h
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,
filcab added a comment.
LGTM on the clang side too.
Thank you,
Filipe
Repository:
rC Clang
https://reviews.llvm.org/D50901
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
filcab added a comment.
Hi Eric,
I know this is old, but are you interested in reviving this patch? I don't know
enough about clang's extensions to LGTM such a patch (updated for the current
code), but would really like to have a way to know if extensions are supported.
We just now had people
filcab added subscribers: pcc, filcab.
filcab added a comment.
It seems there's a FIXME anticipating this problem.
@pcc: Can you double-check, please?
Thank you,
Filipe
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67985/new/
filcab added a comment.
Hi @hctim, thanks for the patch.
I have one question, though. Do you really need to remove the information you
removed?
Some people might be testing ASan binaries without access to debug info
(because it's been stripped or it's not been loaded and is not available for
55 matches
Mail list logo
