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

[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 clang/include/clang

[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: clang/doc

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

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

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

[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: clang/docs/LanguageEx

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

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

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

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

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

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

[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: clang/docs/LanguageExtensions.

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

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

[PATCH] D68115: Zero initialize padding in unions

jfb added a comment. In D68115#1962833 , @rsmith wrote: > In D68115#1946990 , @jfb wrote: > > > In D68115#1946757 , @rsmith wrote: > > > > > In D68115#1946668

[PATCH] D68115: Zero initialize padding in unions

jfb added a comment. > That sounds reasonable to me. So the behavior we're looking for is: > > - If `-ftrivial-auto-init` is off, then we guarantee to zero padding when the > language spec requires it, and otherwise provide no such guarantee. > - If `-ftrivial-auto-init=zeroes` then we guarantee

[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 + "-ftrivial-auto-var-init-stop-afte

[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 : Error<

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

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

[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: clang/include/clang/Basic/Builti

[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: clang/include/clang/Basic

[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: > `overloa

[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: clang/include/clang/Basic/Bui

[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? Com

[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] 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 diagnostic

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

[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 https://review

[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] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. In D77168#1984109 , @jcai19 wrote: > > Right, support needs to be added, and I was wondering if you'd want to do > > this because it seems like a tool that would help you out. > > Sorry for the delay. Yes the biggest advantage of this

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

jfb added a comment. In D77168#1988049 , @srhines wrote: > `pragma clang attribute` is interesting, but how do you apply that in a > selective fashion to local variables (especially in a way that can be > automated)? At first, I didn't think the goal for

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

jfb added a comment. Automatic narrowing of bugs is indeed compelling, so I'd support that as long as it: - Allows bracketing as John suggested (lower / upper bounds where to stop / start). - Is implemented in a way which makes it really hard to regress the security mitigation. Maybe this requ

[PATCH] D78441: Delete NaCl supportSee https://developer.chrome.com/native-client/migration

jfb added a comment. I think someone from the current NaCl team should OK this change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78441/new/ https://reviews.llvm.org/D78441 ___ cfe-commits mailing l

[PATCH] D78693: Make "#pragma clang attribute" support uninitialized attribute.

jfb added a comment. Looks pretty good to me. Not my areas of expertise so I’d like to have others look too. Thanks for doing this! Comment at: clang/test/Parser/pragma-attribute.cpp:190 +#pragma clang attribute pop +#pragma clang attribute push([[clang::uninitialized]], apply

[PATCH] D78693: Make "#pragma clang attribute" support uninitialized attribute.

jfb accepted this revision. jfb added inline comments. Comment at: clang/test/Parser/pragma-attribute.cpp:190 +#pragma clang attribute pop +#pragma clang attribute push([[clang::uninitialized]], apply_to = variable) // expected-error {{attribute 'uninitialized' can't be applied

[PATCH] D78190: Add Bfloat IR type

jfb added a comment. Thanks for adding the IR type. At a high-level this looks good to me, but I haven't done an in-depth review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78190/new/ https://reviews.llvm.org/D78190 _

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

jfb added a comment. I think you want to emit a diagnostic any time auto-init doesn't happen because of this new flag. With the code simplification I propose, it would be pretty hard to introduce a regression... and it someone does it wouldn't be a silent one :) Comment at:

[PATCH] D79279: Allow volatile parameters to __builtin_mem{cpy,move,set}

jfb created this revision. Herald added subscribers: cfe-commits, ributzka, dexonsmith, jkorous, JDevlieghere. Herald added a project: clang. These builtins are often used (or should be used) in places where time-of-check time-of-use security issues are important (e.g. copying from untrusted buff

[PATCH] D79279: Allow volatile parameters to __builtin_mem{cpy,move,set}

jfb added a comment. In D79279#2015983 , @rjmccall wrote: > Most of the complexity of this patch is introduced by the decision to > type-check these calls with a volatile-typed parameter, which seems like it > does nothing but cause problems. If your go

[PATCH] D79279: Allow volatile parameters to __builtin_mem{cpy,move,set}

jfb added a comment. In D79279#2016573 , @rjmccall wrote: > In D79279#2016570 , @rjmccall wrote: > > > I do think this is a somewhat debatable change in the behavior of these > > builtins, though. > > > Let me put

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

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

[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] 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: clang/docs/LanguageE

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

[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, &bar, __ATOMIC_RELAXED); // expected-warning {{misaligned atomic operation may inc

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

[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 https:

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

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

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

[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 clang/include

[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: clang/docs/Lan

[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 `__ubsan_ha

[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: cla

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

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

[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#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 k

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

[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 clang/include/clang/Basic/Builtins.def

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

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

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

[PATCH] D79504: [Clang] Wrong return type of atomic_is_lock_free

jfb added a comment. Just a guess, but maybe the return type was intended to match what a C implementation does with `atomic_is_lock_free` as a macro: https://en.cppreference.com/w/c/atomic/atomic_is_lock_free If so, maybe the expectation is that a macro could define `atomic_is_lock_free` using

[PATCH] D79504: [Clang] Wrong return type of atomic_is_lock_free

jfb added a comment. In D79504#2023274 , @rsmith wrote: > Clang's `` uses the builtin as follows: > > #define atomic_is_lock_free(obj) __c11_atomic_is_lock_free(sizeof(*(obj))) > > > ... which, combined with the builtin returning `int`, results in a call

[PATCH] D79279: Allow volatile parameters to __builtin_mem{cpy,move,set}

jfb added a comment. Here's the RFC: http://lists.llvm.org/pipermail/cfe-dev/2020-May/065385.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 ___ cfe-commit

[PATCH] D79588: [llvm][Support] Use std::atomic for llvm::call_once

jfb added a subscriber: brad. jfb added a comment. is this code still relevant? #if defined(_MSC_VER) // MSVC's call_once implementation worked since VS 2015, which is the minimum // supported version as of this writing. #define LLVM_THREADING_USE_STD_CALL_ONCE 1 #elif defined(LLVM_ON_U

[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 -Wunion-tail-paddi

[PATCH] D80055: Diagnose union tail padding

jfb created this revision. jfb added reviewers: rsmith, hubert.reinterpretcast. Herald added subscribers: cfe-commits, ributzka, dexonsmith, jkorous. Herald added a project: clang. jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/test/CodeGen/uni

[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' field

[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/include/clang/Basic/DiagnosticGroups.td:825 +def Padding : DiagGroup<"padding", [UnionTailPadding]>, + DiagCategory<"Padding Issue">; + I'd like to hear what other dia

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

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

[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] 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' field

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

[PATCH] D67023: Diagnose use of ATOMIC_VAR_INIT

jfb added a comment. Is atomic initialization now correct in all modes (including C++) without this macro? I don’t think we should diagnose until such a time because some code uses to macro to be portably correct. IIRC we only ended up fixing C++ in 20 with Nico’s paper (after Olivier and I fai

[PATCH] D67023: Diagnose use of ATOMIC_VAR_INIT

jfb added a comment. I refer you to http://wg21.link/p0883 I don’t think this diagnostic should be added to clang until p0883 is fully implemented, even just for C. Otherwise we leave users with no portable way to do the right thing without diagnostic. CHANGES SINCE LAST ACTION https://revie

[PATCH] D67023: Diagnose use of ATOMIC_VAR_INIT

jfb added a comment. In D67023#1654704 , @aaron.ballman wrote: > In D67023#1654070 , @jfb wrote: > > > I refer you to http://wg21.link/p0883 > > I don’t think this diagnostic should be added to clang until p0883 i

[PATCH] D66822: Hardware cache line size builtins

jfb requested changes to this revision. jfb added a comment. This revision now requires changes to proceed. Sorry for the delayed response, I was on vacation. Thanks for tackling it! I don't think this is the approach I would take. From my dev meeting lightning talk I would instead: 1. Add to t

[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

jfb added inline comments. Comment at: lib/Sema/SemaExpr.cpp:11067 + + // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1. + if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64)) xbolva00 wrote: > xbolva00 wrote: > > aaron.ballman wr

[PATCH] D68115: Zero initialize padding in unions

jfb added a comment. @vitalybuka could we move this patch forward? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68115/new/ https://reviews.llvm.org/D68115 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D71600: PowerPC 32-bit - forces 8 byte lock/lock_free decisions at compiled time

jfb added a comment. In D71600#1816834 , @adalava wrote: > In D71600#1816160 , @MaskRay wrote: > > > I am still confused why you need the special rule for > > `__atomic_is_lock_free` (GCC/clang) and `__c11_atomic_i

[PATCH] D72579: Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth

jfb added a subscriber: jwakely. jfb added a comment. This changes the expression to a constant expression as well, right? You should point this out in the commit message. The divergence with GCC is unfortunate, @jwakely do you think you'd be able to get GCC to match this behavior as well? It's

[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 cfe-commits@lists.ll

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

[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: clang/test/CodeGenCXX/bu

[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] 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 cfe-commits@list

[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 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: clang/docs/Language

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

<    1   2   3   4   5   6   >