[PATCH] D46836: Fix some rtti-options tests

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 -fno

[PATCH] D47291: Proposal to make rtti errors more generic.

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

[PATCH] D38354: Fix test by using -target instead of cc1 arguments (c-index-test goes through the driver)

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: test/Index/pch-from

[PATCH] D38364: Fix Modules/builtin-import.mm to be able to handle non-Darwin targets

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 === --- tes

[PATCH] D38364: Fix Modules/builtin-import.mm to be able to handle non-Darwin targets

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

[PATCH] D38364: Fix Modules/builtin-import.mm to be able to handle non-Darwin targets

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: cfe/trunk/test/Modules/builti

[PATCH] D38354: Fix test by using -target instead of cc1 arguments (c-index-test goes through the driver)

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

[PATCH] D43013: ASan+operator new[]: Fix operator new[] cookie poisoning

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 poiso

[PATCH] D43013: ASan+operator new[]: Fix operator new[] cookie poisoning

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 the

[PATCH] D43013: ASan+operator new[]: Fix operator new[] cookie poisoning

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 lib/Frontend/CompilerInvocat

[PATCH] D43013: ASan+operator new[]: Fix operator new[] cookie poisoning

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&id=133830#toc

[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)

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 a

[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)

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 i

[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)

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.

[PATCH] D41301: ASan+operator new[]: Fix operator new[] cookie poisoning

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 with

[PATCH] D41301: ASan+operator new[]: Fix operator new[] cookie poisoning

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: cfe/trunk/lib/CodeGen/ItaniumCXXABI.c

[PATCH] D47375: [Driver] Add flag "--dependent-lib=..." when enabling asan or ubsan on PS4.

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 fo

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

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

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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 Repository

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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 > > >

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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: > > > > > > > I

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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 include/clang/Frontend/CodeGenOptio

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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 include/clang/Driver/SanitizerArgs

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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 https://reviews.llvm.org/D5261

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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 inclu

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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 include/clang/Driver/Saniti

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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&id=172392#t

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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 https://reviews.

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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

[PATCH] D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found.

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, problematic

[PATCH] D32950: Support C++1z features in `__has_extension`

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 a

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

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

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

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,

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

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 Repository

[PATCH] D67985: CFI: wrong type passed to llvm.type.test with multiple inheritance devirtualization

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/ https://reviews.llvm

[PATCH] D40948: Switch Clang's default C++ language target to C++14

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; +

[PATCH] D40948: Switch Clang's default C++ language target to C++14

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; +

[PATCH] D124493: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals

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 th

[PATCH] D19667: [ubsan] Minimize size of data for type_mismatch

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 http://lists.llvm.or

[PATCH] D28242: [ubsan] Minimize size of data for type_mismatch (Redo of D19667)

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 ___

[PATCH] D28242: [ubsan] Minimize size of data for type_mismatch (Redo of D19667)

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&id=83362#toc Repository: rL LLVM http

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

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 > > elimina

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

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()) +

[PATCH] D32047: [Driver] Add support for default UBSan blacklists

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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30285: [ubsan] Don't check alignment if the alignment is 1

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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D30423: [ubsan] Detect UB loads from bitfields

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 wrote

[PATCH] D30729: [ubsan] Skip range checks for width-limited values

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 you'

[PATCH] D30423: [ubsan] Detect UB loads from bitfields

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 @__ubsa

[PATCH] D30762: [ubsan] Add a nullability sanitizer

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, it

[PATCH] D21695: [clang] Version support for UBSan handlers

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

[PATCH] D21695: [clang] Version support for UBSan handlers

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&id=81089#toc Repository: rL LLVM https://reviews.llvm.org/D

[PATCH] D32199: [TBAASan] A TBAA Sanitizer (Clang)

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, ---

[PATCH] D33305: [ubsan] Add a check for pointer overflow UB

filcab added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:3854 + const Twine &Name) { + Value *GEPVal = Builder.CreateInBoundsGEP(Ptr, IdxList, Name); + You're creating the GEP first (possibly triggering