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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added subscribers: rsmith, Quuxplusone.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/list:273
 {
-    _Tp __value_;
+    union { _Tp __value_; };
 
----------------
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. ;)


================
Comment at: libcxx/include/list:1122
       return __hold_pointer(__p, __node_destructor(__na, 1));
     }
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99624