[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added inline comments. Comment at: compiler-rt/lib/ubsan/ubsan_handlers.cpp:659 + uptr PtrOrSize) { GET_REPORT_OPTIONS(true); + handleInvalidBuiltin(Data, Opts, PtrOrSize); vsk wrote: > It looks like

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 283402. jfb marked an inline comment as done. jfb added a comment. Use 'access size' instead of 'element size'. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2446 +order in which they occur (and in which they are observable) can only be +guaranteed using appropriate fences around the function call. Element size must +therefore be a lock-free size for the target

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 283406. jfb marked 5 inline comments as done. jfb added a comment. Remove restrict, update docs, call isCompleteType Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2197118 , @rsmith wrote: > Two observations that are new to me in this review: > > 1. We already treat all builtins as being overloaded on address space. > 2. The revised patch treats `__builtin_mem*_overloaded` as being

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 283390. jfb added a comment. Do UBSan change suggested by Vedant. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/docs/LanguageExtensions.rst

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 274847. jfb edited the summary of this revision. jfb added a comment. Overload a new builtin instead. For now I only did memcpy, to get feedback on the approach. I'll add other mem* functions if this makes sense. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D82832: Correctly generate invert xor value for Binary Atomics of int size > 64

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Herald added a subscriber: dexonsmith. This amused me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82832/new/ https://reviews.llvm.org/D82832

[PATCH] D77493: [clang-tidy] Add do-not-refer-atomic-twice check

jfb added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-do-not-refer-atomic-twice.cpp:10 +_Atomic int n2 = ATOMIC_VAR_INIT(0); +_Atomic(int) n3 = ATOMIC_VAR_INIT(0); + Can you cover `std::atomic` as well?

[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

jfb added a comment. It seems like you don't want this check to trigger on POSIX platforms, given: > Exceptions CON37-C-EX1: Implementations such as POSIX that provide defined > behavior when multithreaded programs use custom signal handlers are exempt > from this rule [IEEE Std 1003.1-2013].

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 275266. jfb marked 18 inline comments as done. jfb added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:489 BUILTIN(__builtin_memcpy_inline, "vv*vC*Iz", "nt") +BUILTIN(__builtin_overloaded_memcpy, "v*v*vC*z", "nt") BUILTIN(__builtin_memmove, "v*v*vC*z", "nF") gchatelet wrote: >

[PATCH] D83509: CrashTracer: clang at clang: llvm::BitstreamWriter::ExitBlock

jfb accepted this revision. jfb added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:123 +def warn_fe_serialized_diag_failure_during_finalisation : Warning< +"Received warning after

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 274949. jfb added a comment. Arcanist ate the rest of my commit and I am confused. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 274948. jfb added a comment. This builtin doesn't return anything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/include/clang/Basic/Builtins.def Index:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 275005. jfb added a comment. Add memmove and memset (but still missing the codegen tests) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D83509: CrashTracer: clang at clang: llvm::BitstreamWriter::ExitBlock

jfb added a comment. I don't remember if this will auto-close, since I forgot to add the Phabricator ID to the commit message. In any case it's in: 00c9a504aeed2603bd8bc9b89d753534e929c8e8 Repository: rG LLVM Github

[PATCH] D80055: Diagnose union tail padding

jfb abandoned this revision. jfb added a comment. I've gotten what I wanted out of this (diagnosed a particular codebase), and am not sure it's worth pursuing further. If anyone is interested, please let me know. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D79249: [NOT FOR REVIEW] Experimental support for zero-or-trap behavior foruninitialized variables.

jfb added a comment. Overall I like this approach. I think it needs three more things to make it: - Better size optimizations. I measured the code size impact on a codebase which deploys variable auto-init already, and it's a 0.5% code size hit. Considering that auto-init itself was 1%, it

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. Can you add a test for the diagnostic firing after the correct number of initializations? This should include a few types of auto-init, including VLAs. Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:489 +

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Two minor corrections, looks good otherwise. Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:491 +def err_drv_trivial_auto_var_init_stop_after_invalid_value :

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 279984. jfb marked 7 inline comments as done. jfb added a comment. Address all but one of John's comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8917 + +def err_atomic_type_must_be_lock_free : Error<"_Atomic type must always be lock-free, %0 isn't">; + rjmccall wrote: > I don't know why you're adding a bunch of new

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2168479 , @rjmccall wrote: > Is there a need for an atomic memcpy at all? Why is it useful to allow this > operation to take on "atomic" semantics — which aren't actually atomic > because the loads and stores to elements

[PATCH] D84666: [NFC] Sema: use checkArgCount instead of custom checking

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG389f009c5757: [NFC] Sema: use checkArgCount instead of custom checking (authored by jfb). Repository: rG LLVM Github

[PATCH] D84666: [NFC] Sema: use checkArgCount instead of custom checking

jfb created this revision. jfb added a reviewer: rsmith. Herald added subscribers: cfe-commits, ributzka, dexonsmith, jkorous, kbarton, nemanjai. Herald added a project: clang. As requested in D79279 . Repository: rG LLVM Github Monorepo

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8793 +BuiltinOp == Builtin::BI__builtin_memmove_overloaded || BuiltinOp == Builtin::BI__builtin_wmemmove; If we end up

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a subscriber: dneilson. jfb added a comment. I've addressed @rsmith @rjmccall suggestions (unless noted), thanks! An open question: as of 6e4aa1e48138182685431c76184dfc36e620aea2 @dneilson added an assertion on `CreateElementUnorderedAtomicMemCpy` to check that the pointer arguments

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 281087. jfb marked 15 inline comments as done. jfb added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 280136. jfb added a comment. Re-update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/docs/LanguageExtensions.rst clang/include/clang/Basic/Builtins.def

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 280135. jfb added a comment. Improve documentation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/docs/LanguageExtensions.rst Index:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2168649 , @rjmccall wrote: > In D79279#2168533 , @jfb wrote: > > > In D79279#2168479 , @rjmccall > > wrote: > > > > > Is there a need for an

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. > My point is that this has nothing to do with the ordinary semantics of > `_Atomic`. You're basically just looking at the word "atomic" and saying > that, hey, a minimum access size is sortof related to atomicity. > > If you want this to be able to control the minimum

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2170157 , @rjmccall wrote: > I think the argument is treated as if it were 1 if not given. That's all > that ordinary memcpy formally guarantees, which seems to work fine > (semantically, if not performance-wise) for

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2170095 , @rjmccall wrote: > I don't think any of these should allow _Atomic unless we're going to give it > some sort of consistent atomic semantics (which is hard to imagine being > useful), and I think you should just

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. >>> Do you think it'd be useful to have different guarantees for different >>> operands? I guess it could come up, but it'd be a whole lot of extra >>> complexity that I can't imagine we'd ever support. >> >> You mean, if `element_size` is passed then you get different

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 279896. jfb added a comment. Follow John's suggestions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/include/clang/Basic/Builtins.def

[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

jfb added a comment. Most of the tests as written should be failing right now, at least on macOS and Linux, because they likely should be identified as POSIX, right? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75229/new/

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

jfb added a comment. Let's make sure that we follow the same semantics that GCC does, particularly w.r.t. union, bitfields, and padding at the end of an object (whether it's in an array or not). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D104975: Implement P1949

jfb added a comment. It would be more user friendly to say which character is not allowed in the diagnostic. Do we need to have a backwards compatible flag to preserve the old behavior, or do we believe that nobody will be affected by the change? We should make this choice explicitly and

[PATCH] D98995: [CGAtomic] Lift stronger requirements on cmpxch and add support for acquire failure mode

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Maybe refer to http://wg21.link/p0418 directly in the commit message? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98995/new/ https://reviews.llvm.org/D98995

[PATCH] D100057: Remove warning "suggest braces" for aggregate initialization of an empty class with an aggregate base class.

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. A few nits, but this is good otherwise! Comment at: clang/lib/Sema/SemaInit.cpp:1013 - auto *ParentRD = - Entity.getParent()->getType()->castAs()->getDecl(); - if

[PATCH] D108469: Improve handling of static assert messages.

jfb added a subscriber: hwright. jfb added a comment. I worry that changing the general `static_assert` printing (adding a colon, and dropping the quotes) will get @hwright's law to drop on us. We can try and see if e.g. users of clang have automated checks for `static_assert` in their CI

[PATCH] D108469: Improve handling of static assert messages.

jfb added a comment. Can you test all the values in this? https://godbolt.org/z/h7n54fa5x Comment at: clang/lib/Basic/Diagnostic.cpp:792 +static void pushEscapedString(StringRef Str, SmallVectorImpl ) { + OutStr.reserve(OutStr.size() + Str.size()); + const unsigned char

[PATCH] D115440: Provide __builtin_alloca*_uninitialized variants

jfb added a comment. In D115440#3183778 , @melver wrote: > GCC devs say that initializing explicit alloca() is a bug, because they > aren't "automatic storage": > https://lkml.kernel.org/r/20211209201616.gu...@gate.crashing.org > .. which is also the

[PATCH] D125142: [clang][auto-init] Remove -enable flag for "zero" mode

jfb added a subscriber: Florian. jfb added a comment. I think the most relevant post from @rsmith is: https://discourse.llvm.org/t/making-ftrivial-auto-var-init-zero-a-first-class-option/55143/40 He has a prototype: https://reviews.llvm.org/D79249 I assume he would like someone to pursue it

[PATCH] D155580: [trivial-auto-var-init] Do not emit initialization code for empty class

jfb added a comment. (sorry for sending twice, looks like email reply failed) This is the same as padding, and is initialized on purpose. If it’s truly unused then the optimizer will eliminate it… unless it can’t prove whether the store is dead. Does this show up in any meaningless

<    1   2   3   4   5   6