(sorry for sending twice, looks like email reply failed)

Are there any worries about moving stores that are on atomic/volatile "objects", either because the auto-init store itself is atomic/volatile, or because a subsequent access is marked as such? I don't think there's a worry because auto-init stores are technically outside the abstract machine (they "don't exist"), but I'm not 100% convinced.

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

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.GU614@gate.crashing.org
.. which is also the reason why GCC's behaviour differs here at the moment.

I actually agree with that, and alloca auto-init behaviour needs a new -mllvm param.

Can you test all the values in this? https://godbolt.org/z/h7n54fa5x

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 pipelines or something. I think your new format looks better, but Hyrum is finicky that way... What do others think?

I agree with @eli.friedman's breakdown of options here: https://reviews.llvm.org/D105338#2870156

It would be more user friendly to say which character is not allowed in the diagnostic.

FWIW I support something along the lines of what Palmer is trying to do: many users of the STL would like to be able to handle particular types of library UB instead of performing potentially unbounded effects. Which ones is a bit of an art that we need to figure out, for example I wouldn't want to unduly affect performance (by e.g. changing algorithmic complexity) or code size (here, with diagnostic bloat). In a way we're experimenting on the edges of what contract checking would be for the STL, and I would approach it with this optic: could we have a mode of the STL which enforces contracts, and a mode where you get whatever. If so, what should be checked and what should remain unchecked, and what are the principles to distinguish these. I think in this discussion it's important to separate the invariants that are details of libc++'s implementation, from what are actual contracts at the STL API level.

In D101501#2735352, @rjmccall wrote:

Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.

I would recommend checking with @MadCoder about the implementation and ABI, since the kernel has Opinions on this. IIRC he looked at Olivier's original patch.

A few nits, but this is good otherwise!

I'm no longer in the security group, but (from the peanut gallery) I endorse Google having a second representative, and George in particular.

Maybe refer to http://wg21.link/p0418 directly in the commit message?

Does it line up property with ./clang/lib/Headers/stdatomic.h ? I think this other header might need some adjustment as well, for example the hosted/freestanding part.

LGTM. Maybe someone like @STL_MSFT can see if MS folks are OK with this change.

In D92500#2429186, @dexonsmith wrote:

@jfb, any reason you didn't gut this in 80b67baaedd50ded121908f9a4c03f5939b84f24?

This is a sensible thing to support IMO. It's indeed weird to have native handle in other places, but not here.

This is a sensible thing to support IMO. It's indeed weird to have native handle in other places, but not here.

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

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?

ping

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 part of https://clang.llvm.org/docs/LanguageExtensions.html#builtin-functions. These new atomic builtins can be documented in a similar way after that.

Please provide documentation in this patch.

Generally this looks good, but I didn't go into details.
It would be good to have an MS person like @STL_MSFT check that the std tests match, at least for what they've implemented.

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.

ping

In D86917#2250709, @vitalybuka wrote:

In D86917#2249793, @jfb wrote:

Is this really NFC?

I guess so, do you have particular concern?
I don't know particular cases where it was broken, but reading values some times as atomic and sometimes as not, plus unclear order of bit fields looks suspicions.

Is this really NFC?

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.

Please consider these changes, and whether this is relevant as implemented: http://wg21.link/p0270

I'll wait 5 days to do this, since our process says:

Remove the "suppress this" in release notes.

Fix notes.

Address comments

Re-upload with full context.

As Vedant suggested, make it part of 'integer' sanitizer.

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 be produced. That being the case, I wonder if the name is emphasizing the wrong thing, and a name that contains atomic would be more obvious. As a concrete alternative, __atomic_unordered_memcpy is not much longer and seems to have the right kinds of implications. WDYT?

Address Richard's comments.

This is probably still useful, if someone wants to pick it up.

I don't think the world is ready for this.

Ping, I think I've addressed all comments here.

In D86043#2223293, @luismarques wrote:

In D86043#2222622, @jfb wrote:

Can you add a test that this works, e.g. by having a program which uses these functions, but doesn't link to libatomic?

This can be tested by my previous (under review) D81348. With this patch you can compile and link that test from D81348 when targeting RISC-V rv32imac, while without this patch that test will fail to link. (Tested with a complementary GNU baremetal newlib toolchain, which doesn't even have libatomic to link to.)

Can you add a test that this works, e.g. by having a program which uses these functions, but doesn't link to libatomic?

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-added implicit conversion checks were folded in without much/any pushback.

@ldionne should sign off on the libcxx test, but the rest seems fine to me. Eventually we probably want to enable something, but need to be consistent about it.

The typo fix is fine, I'm not sure it's worth fixing variable names (since there's an ongoing discussion about renaming everything anyways).
FWIW C++ doesn't really have a lattice anymore: https://wg21.link/p0418

See for example r360421 D58251 D50491 D26266 D26266 r264845 D15471
Maybe talk to folks such as @reames ?

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.

Fix a test.

I'd expect us to go the other way, and allow this. There have been patches in the past to allow usage of atomics with float. Could you look with their authors and see what they advise here?

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 tests for their codegen and address space (which should already work, but isn't tested).

Can you please add tests for this?

In D85628#2213940, @rjf wrote:

In D85628#2213919, @vsk wrote:

I’m not convinced this is a good idea. In what use case is it not possible to mark up relevant functions? It doesn’t make sense to me to make alternations to standard library functions within the compiler. It seems better to simply patch the standard library. In some cases llvm does infer function attributes for library functions, but these are generally lower level attributes that can’t be specified at the source level, and the attribute is made available to other passes in the pipeline.

Do you mean this patch isn't a good idea in general, or the recent revision isn't a good idea? For the latter, I'm not sure if you meant we should not outline declarations or we should not split the original loop into two (e.g. marking as cold before outlining). IMO splitting the loop into two simply addresses what the original intent of what we're doing, which is to mark certain functions as cold before outlining. Whereas, if we don't outline declarations via user-provided input, it renders @hiraditya 's proposed testcase useless. Alternatively, we don't have to make the testcase involving standard library functions if that's what you want :).

Does this x86 change actually match what the runtime does in the same file?

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 keep the plain C functions unextended. I had actually thought we already did that in more cases than we do (but perhaps I was thinking about the LLVM math intrinsics that guarantee to not set errno). That would mean that a C standard library implementation is still free to #define foo(x,y,z) __builtin_foo(x,y,z), but if they do, they may pick up extensions.

In D85691#2208348, @cuviper wrote:

In D85691#2208323, @jfb wrote:

You're probably better off putting the std::chrono::nanoseconds::rep behind a lock if it's not always lock free on all platforms. Some platforms just don't have libatomic, so this patch might cause other issues.

Wouldn't such a platform get a fatal error in llvm/cmake/modules/CheckAtomic.cmake?

You're probably better off putting the std::chrono::nanoseconds::rep behind a lock if it's not always lock free on all platforms. Some platforms just don't have libatomic, so this patch might cause other issues.

I didn't check that all of these compare all the required state for all opcodes.

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 overloaded *only* on address space, volatility, and atomicity. (We've tuned the design to a point where the other qualifiers don't matter any more.)

So, we're adding three features here: overloading on (a) address space, (b) volatility, and (c) atomicity. (a) is already available in the non-_overloaded form, and we seem to be approaching agreement that (b) should be available in the non-_overloaded form too. So that only leaves (c), which is really not _overloaded but _atomic.

Based on those observations I'm now thinking that we might prefer a somewhat different approach (but one that should require only minimal changes to the patch in front of us). Specifically:

1. Stop treating lib builtins (eg, plain memcpy) as overloaded on address space. That's a (pre-existing) conformance bug, at least for the Embedded C TR.
2. Keep __builtin_ forms of lib builtins overloaded on address space. (No change.)
3. Also overload __builtin_ forms of lib builtins on volatility where it makes sense, instead of adding new builtin names __builtin_mem*_overloaded.
4. Add a new name for the builtin for the atomic forms of memcpy and memset (__builtin_memcpy_unordered_atomic maybe?).
5. Convert the "trivial types" check from an error to a warning and apply it to all the mem* overloads. (Though I think we might actually already have such a check, so this might only require extending it to cover the atomic builtin.)

What do you think?

Remove restrict, update docs, call isCompleteType

Use 'access size' instead of 'element size'.

Add loop test requested by Vedant

Do UBSan change suggested by Vedant.

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 memcpys for ordinary __builtin_memcpy.

Update docs

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 per https://reviews.llvm.org/D33240), and in cases where size is known we can inline them (as we do for memcpy and friends).

Address Richard's comments, add UBSan support.