User Details
- User Since
- Jan 27 2014, 9:36 AM (503 w, 4 d)
Jul 18 2023
(sorry for sending twice, looks like email reply failed)
Nov 22 2022
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.
May 11 2022
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
Dec 13 2021
Sep 27 2021
Can you test all the values in this? https://godbolt.org/z/h7n54fa5x
Sep 8 2021
Aug 20 2021
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?
Jul 12 2021
I agree with @eli.friedman's breakdown of options here: https://reviews.llvm.org/D105338#2870156
Jun 26 2021
It would be more user friendly to say which character is not allowed in the diagnostic.
Jun 15 2021
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.
May 4 2021
May 3 2021
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.
Apr 16 2021
Apr 7 2021
A few nits, but this is good otherwise!
Mar 24 2021
I'm no longer in the security group, but (from the peanut gallery) I endorse Google having a second representative, and George in particular.
Mar 20 2021
Maybe refer to http://wg21.link/p0418 directly in the commit message?
Feb 19 2021
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.
Dec 2 2020
LGTM. Maybe someone like @STL_MSFT can see if MS folks are OK with this change.
Nov 23 2020
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.
Nov 20 2020
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).
Nov 19 2020
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?
Nov 10 2020
Oct 8 2020
ping
Sep 23 2020
Sep 19 2020
Sep 18 2020
Sep 17 2020
Please provide documentation in this patch.
Sep 8 2020
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.
Sep 7 2020
ping
Sep 3 2020
Sep 1 2020
Is this really NFC?
Aug 29 2020
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.
Aug 28 2020
Please consider these changes, and whether this is relevant as implemented: http://wg21.link/p0270
Aug 27 2020
I'll wait 5 days to do this, since our process says:
Remove the "suppress this" in release notes.
Fix notes.
Address comments
Aug 26 2020
Re-upload with full context.
As Vedant suggested, make it part of 'integer' sanitizer.
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.
Aug 24 2020
Ping, I think I've addressed all comments here.
Aug 18 2020
Aug 17 2020
Can you add a test that this works, e.g. by having a program which uses these functions, but doesn't link to libatomic?
Aug 14 2020
@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
Aug 13 2020
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?
Aug 12 2020
Does this x86 change actually match what the runtime does in the same file?
Aug 11 2020
Aug 10 2020
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.
Aug 5 2020
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.
Aug 4 2020
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.