[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

[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] 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] 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] 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] 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] 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] 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] 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] 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] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

jfb added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1647 +QualType Ty) { + auto *I8Ptr = CGF.Builder.CreateBitCast(Ptr, CGF.Int8PtrTy); + auto *Zero = ConstantInt::get(CGF.Int8Ty, 0); I'd like to hear

[PATCH] D87974: Summary: [Builtin] Add __builtin_zero_non_value_bits.

jfb added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1682 + + size_t NumFeilds = std::distance(R->field_begin(), R->field_end()); + auto CurrentField = R->field_begin(); Typo in "fields". Comment at:

[PATCH] D87858: [hip] Add HIP scope atomic ops.

jfb added a comment. In D87858#2282150 , @yaxunl wrote: > In D87858#2280429 , @jfb wrote: > >> Please provide documentation in this patch. > > opencl atomic builtins are documented as notes to `__c11_atomic

[PATCH] D87858: [hip] Add HIP scope atomic ops.

jfb added a comment. Please provide documentation in this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87858/new/ https://reviews.llvm.org/D87858 ___ cfe-commits mailing list

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

jfb added a comment. On C++20 mode rotting: it won't if someone sets up a bot. If it rots, then it's easier to un-rot with Barry's patch. Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:805-817 + return IsASCII ? "^" : (const char *)u8"\u2548"; case

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

jfb added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

jfb requested changes to this revision. jfb added a comment. This revision now requires changes to proceed. MSC54-CPP refers to POF, which as I pointed out above isn't relevant anymore. I'd much rather have a diagnostic which honors the state of things after http://wg21.link/p0270.

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

jfb requested changes to this revision. jfb added a comment. This revision now requires changes to proceed. Herald added a subscriber: dexonsmith. Please consider these changes, and whether this is relevant as implemented: http://wg21.link/p0270 Comment at:

[PATCH] D86000: Add an unsigned shift base sanitizer

This revision was automatically updated to reflect the committed changes. Closed by commit rG82d29b397bb2: Add an unsigned shift base sanitizer (authored by jfb). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb added inline comments. Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6 + volatile unsigned _v = (val);\ + volatile unsigned _a = (amount); \ + unsigned res = _v << _a; \ vsk wrote: > jfb wrote: > > vsk wrote: > > >

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288444. jfb marked 4 inline comments as done. jfb added a comment. Remove the "suppress this" in release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files:

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:153 + + `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs` + lebedev.ri wrote: > Surely not `~1U`. Indeed, fixed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288388. jfb marked an inline comment as done. jfb added a comment. Fix notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files:

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3872-3884 llvm::Value *BitsShiftedOff = Builder.CreateLShr( Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros", /*NUW*/ true, /*NSW*/

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288383. jfb marked 6 inline comments as done. jfb added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288133. jfb added a comment. Re-upload with full context. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files: clang/docs/UndefinedBehaviorSanitizer.rst

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288132. jfb added a comment. As Vedant suggested, make it part of 'integer' sanitizer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files:

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

jfb added a comment. In D79279#2235085 , @rsmith wrote: > Thanks, I'm happy with this approach. > > If I understand correctly, the primary (perhaps sole) purpose of > `__builtin_memcpy_sized` is to provide a primitive from which an atomic > operation

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

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

[PATCH] D77219: UBSan ␇ runtime

jfb abandoned this revision. jfb added a comment. Herald added a subscriber: dang. I don't think the world is ready for this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77219/new/ https://reviews.llvm.org/D77219

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

jfb added a comment. Ping, I think I've addressed all comments here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 ___ cfe-commits mailing list

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb added a comment. In D86000#2219288 , @vsk wrote: > It'd be nice to fold the new check into an existing sanitizer group to bring > this to a wider audience. Do you foresee adoption issues for existing > -fsanitize=integer adopters? Fwiw some

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb created this revision. jfb added a reviewer: vsk. Herald added subscribers: Sanitizers, cfe-commits, ributzka, dexonsmith, jkorous. Herald added projects: clang, Sanitizers. jfb requested review of this revision. It's not undefined behavior for an unsigned left shift to overflow (i.e. to

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

jfb added a comment. Actually I think any subsequent updates to tests can be done in a follow-up patch, since I'm not changing the status-quo on address space here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/

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

jfb updated this revision to Diff 285479. jfb added a comment. Fix a test. 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 285414. jfb added a comment. Update overloading as discussed: on the original builtins, and separate the _sized variant, making its 4th parameter non-optional. If this looks good, I'll need to update codege for a few builtins (to handle volatile), as well as

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

jfb added a comment. In D79279#2201484 , @rsmith wrote: > I think it would be reasonable in general to guarantee that our `__builtin_` > functions have contracts at least as wide as the underlying C function, but > allow them to have extensions, and to

[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 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 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: 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 283400. jfb marked an inline comment as done. jfb added a comment. Add loop test requested by Vedant 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 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 added a comment. In D79279#2195299 , @rjmccall wrote: > Patch looks basically okay to me, although I'll second Richard's concern that > we shouldn't absent-mindedly start producing overloaded `memcpy`s for > ordinary `__builtin_memcpy`. Yeah I

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

jfb updated this revision to Diff 283121. jfb marked 2 inline comments as done. jfb added a comment. Update docs 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 added inline comments. Comment at: compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp:1 +// REQUIRES: arch=x86_64 +// Phab is confused I did a git rename of `compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp` and it thinks this is new, and I

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

jfb added a comment. Thanks for the detailed comments, I think I've addressed all of them! I also added UBSan support to check the builtin invocation. I think this patch is pretty much ready to go. A follow-up will need to add the support functions to compiler-rt (they're currently optional,

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

jfb updated this revision to Diff 283078. jfb marked 6 inline comments as done. jfb added a comment. Herald added a project: Sanitizers. Herald added a subscriber: Sanitizers. Address Richard's comments, add UBSan support. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D85102: [clang] improve diagnostics for misaligned and large atomics

This revision was automatically updated to reflect the committed changes. Closed by commit rGe18c6ef6b41a: [clang] improve diagnostics for misaligned and large atomics (authored by tschuett, committed by jfb). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D85102: [clang] improve diagnostics for misaligned and large atomics

jfb accepted this revision. jfb added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/CodeGen/atomics-sema-alignment.c:43 + Foo bar; + __atomic_load(foo, , __ATOMIC_RELAXED); // expected-warning {{misaligned atomic operation may incur

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

jfb added a comment. This is almost ready I think! There are a few things still open, I'd love feedback on them. Comment at: clang/docs/LanguageExtensions.rst:2435-2437 +* ``__builtin_memcpy_overloaded(QUAL void *dst, QUAL const void *src, size_t byte_size, size_t

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

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

[PATCH] D85009: [Sema][BFloat] Forbid arithmetic on vectors of bfloat.

jfb added a comment. In D85009#2187631 , @LukeGeeson wrote: > In D85009#2187621 , @jfb wrote: > >> In D85009#2187603 , @simon_tatham >> wrote: >> >>> In D85009#2187549

[PATCH] D85009: [Sema][BFloat] Forbid arithmetic on vectors of bfloat.

jfb added a comment. In D85009#2187603 , @simon_tatham wrote: > In D85009#2187549 , @jfb wrote: > >> Is that true of all vector bfloat implementations? It seems like arithmetic >> on these types is something

[PATCH] D85009: [Sema][BFloat] Forbid arithmetic on vectors of bfloat.

jfb added a comment. Is that true of all vector bfloat implementations? It seems like arithmetic on these types is something implementations would likely support. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85009/new/

[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] 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] 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 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 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. > 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 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. 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] 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 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 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] D71726: Let clang atomic builtins fetch add/sub support floating point types

jfb added a comment. In D71726#2165445 , @yaxunl wrote: > In D71726#2165424 , @jyknight wrote: > > > Why not have clang always emit atomicrmw for floats, and let > > AtomicExpandPass handle legalizing that into

[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] 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] 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] 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] 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] 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] 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 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 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] 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 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] 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] D80055: Diagnose union tail padding

jfb added a comment. In D80055#2042630 , @rsmith wrote: > I'm not convinced that this is an especially useful diagnostic (much like > with `-Wpadded`), but I'm also not opposed if you are aware of cases where it > would be used. I wrote it to help

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

jfb accepted this revision. jfb added a comment. One suggestions, otherwise looks good. Thanks for doing this :) Comment at: llvm/include/llvm/ADT/DirectedGraph.h:97 + } + friend bool operator!=(const NodeType , const NodeType ) { !(M == N); } davidstone

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

jfb added a comment. In D71726#2047566 , @ldionne wrote: > In D71726#1791904 , @jfb wrote: > > > This generally seems fine. Does it work on most backends? I want to make > > sure it doesn't fail in backends :) > >

[PATCH] D80055: Diagnose union tail padding

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/test/CodeGen/union-tail-padding.c:28-36 +union Front { + int i; + long long ll; +}; + +union Front front1; +union Front front2 = {};// expected-warning {{Initializing union 'Front'

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. This seems fine, assuming you've run usual tests? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78938/new/ https://reviews.llvm.org/D78938

[PATCH] D68115: Zero initialize padding in unions

jfb added a comment. In D68115#2040696 , @tschuett wrote: > As an outsider: In Swift, reading an uninitialized variable is a compile-time > error. Clang is not powerful enough to do this analysis. Aren't you really > looking for the Clang Intermediate

[PATCH] D80055: Diagnose union tail padding

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:264 +def warn_union_tail_padding_uninitialized : Warning< + "Initializing union %0 field %1 only initializes the first %2 of %3 bytes, leaving the

[PATCH] D80055: Diagnose union tail padding

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/include/clang/Basic/DiagnosticGroups.td:825 +def Padding : DiagGroup<"padding", [UnionTailPadding]>, + DiagCategory<"Padding Issue">; + I'd like to hear what other

[PATCH] D68115: Zero initialize padding in unions

jfb added a comment. To get this unblocked a bit, I implemented a diagnostic: https://reviews.llvm.org/D80055 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68115/new/ https://reviews.llvm.org/D68115

[PATCH] D80055: Diagnose union tail padding

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/test/CodeGen/union-tail-padding.c:28-36 +union Front { + int i; + long long ll; +}; + +union Front front1; +union Front front2 = {};// expected-warning {{Initializing union 'Front'

[PATCH] D80055: Diagnose union tail padding

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/test/CodeGen/union-tail-padding.c:9 +// RUN: %clang_cc1 -Wunion-tail-padding -xc++ -std=c++17 -triple aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify +// RUN: %clang_cc1

  1   2   3   4   5   6   >