[libcxx-commits] [PATCH] D99624: [libc++] Remove UB in std::list

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/list:273
 {
-    _Tp __value_;
+    union { _Tp __value_; };
 
----------------
Quuxplusone wrote:
> I wonder whether you want to have a `char` in this union too, as the first element. (Have I been cargo-culting that idiom around? Godbolt seems to indicate that maybe the rules changed between '03 and '11?)
> 
> Also, of course, you never call the destructor of `_Tp` anywhere anymore, so I //really// hope some tests fail on buildkite. ;)
I'm not aware of the `char`-in-the-union idiom, can you explain what it does?

Also, we do call the destructor for `_Tp` in `__list_imp<_Tp, _Alloc>::clear()`. I haven't fully audited the code, but I don't think we called the destructor for `__list_node` anywhere previously, so adding the `union` doesn't change anything.


================
Comment at: libcxx/include/list:1122
       return __hold_pointer(__p, __node_destructor(__na, 1));
     }
 
----------------
Quuxplusone wrote:
> As I understand it, there is no UB here as of C++20: this is the main point of @rsmith 's [P0593 "Implicit creation of objects for low-level object manipulation"](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0593r4.html#objects-provided-as-byte-representation). When we get the memory for a node from the allocator, the allocator ultimately gets it from some operation that "implicitly creates objects." Essentially, the C++20 compiler is required to assume that `malloc` //might// be implemented as
> ```
> void *malloc(size_t n) {
>     __node *p = real_malloc(n);
>     ::new (p) __node();
>     p->__value_.~_Tp();
>     return p;
> }
> ```
> (I mean, the compiler can't prove `malloc` //isn't// implemented that way!) So libc++'s code //is// allowed to assume that the implicitly created object was in fact created, and the compiler //isn't// allowed to optimize as-if the object wasn't created. Thus: no UB. (Compiler vendors are, technically, allowed to follow a different object model in C++03/11/14/17 mode; but I would be shocked.)
> 
> I describe this "compiler magic" also in ["Trivially Relocatable"](https://www.youtube.com/watch?v=SGdfPextuAU&t=2715s), using `std::list` specifically as my example.
>From P0593:

> A class `S` is an implicit-lifetime class if it is an aggregate ([dcl.aggr]) or has at least one trivial, non-deleted constructor and a trivial, non-deleted destructor.

`__list_node` is not an aggregate (it has a user-declared constructor). It doesn't have a trivial non-deleted constructor AFAICT either, so it's not an implicit-lifetime type. Hence, my understanding is that no object is implicitly created, and it's still UB. I could be wrong, but that's my understanding.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99624/new/

https://reviews.llvm.org/D99624