[libc++] Remove UB in list, forward_list and __hash_table

[ldionne]

Summary

This patch removes undefined behavior in list and forward_list and __hash_table
caused by improperly beginning and ending the lifetime of the various node
classes. It allows removing the _LIBCPP_STANDALONE_DEBUG macro from
these node types since we now properly begin and end their lifetime,
meaning that we won't trip up constructor homing.

See https://reviews.llvm.org/D98750 for more information on what prompted
this patch.

[akhuang]

remove commented out code

[akhuang]

remove commented code

[Mordante]

This looks somewhat similar to D99624. Does it attempt to address the same issue? @ldionne you might want to have a look at this.

[• Quuxplusone]

@Mordante: yes it does. :)
@akhuang: My understanding is that removing-the-UB is needed (or at least desired) for the benefit of some particular sanitizer; could you add info to the review summary (and commit message) about that?
Also, are we really interested in getting that benefit for C++03 code? Arguably it would be better to preserve the existing behavior for C++03 and fix the behavior only for C++>=11. Is it physically possible to go that route? and if so, does that eliminate a lot of the ifdefs in this PR? (I'm thinking we could just have one ifdef around the union itself, and then the rest of the code wouldn't need ifdefs at all.) The behavior would remain UB, but unchanged, in C++03; and we could say "if you want to use such-and-such sanitizer, please compile as C++>=11." That won't be a hardship for the user, will it?

[akhuang]

In D101206#2714550, @Quuxplusone wrote:

@Mordante: yes it does. :)
@akhuang: My understanding is that removing-the-UB is needed (or at least desired) for the benefit of some particular sanitizer; could you add info to the review summary (and commit message) about that?
Also, are we really interested in getting that benefit for C++03 code? Arguably it would be better to preserve the existing behavior for C++03 and fix the behavior only for C++>=11. Is it physically possible to go that route? and if so, does that eliminate a lot of the ifdefs in this PR? (I'm thinking we could just have one ifdef around the union itself, and then the rest of the code wouldn't need ifdefs at all.) The behavior would remain UB, but unchanged, in C++03; and we could say "if you want to use such-and-such sanitizer, please compile as C++>=11." That won't be a hardship for the user, will it?

So the motivation for this is for a debug info optimization that homes debug info for classes to their constructors, and it relies on constructors being called (and we'd like to enable that mode by default in llvm). We don't necessarily care whether there's UB or not, but in this situation we wouldn't have debug info for these types.

I think making it only work for only C++>=11 could be fine, though.

[akhuang]

For the debug info issue, we can also just add an attribute to these types (https://reviews.llvm.org/D98750); I think @ldionne preferred solving this by fixing the UB directly though.

[akhuang]

Remove fix for c++03.

[• Quuxplusone]

LGTM at this point. So I guess the next question (besides how it looks to @ldionne!) is, should you do it for forward_list etc. in this same PR? Do you know already the complete list of types that need this kind of patch done for them? If so, I would think it's appropriate to do them all together in this PR.

libcxx/include/list

285–288

Any reason not to =default this constructor? (And I'd make it explicit, since there's no reason not to, and it might ease overload resolution just a tiny bit. In my codebases, the explicit keyword basically means "Look out, here comes a constructor!")

[• Quuxplusone]

libcxx/include/list

274

Nit: Indent 4 spaces to match the indentation everywhere else in this file. (I blame clang-format. ;))

[akhuang]

Apply union fix to forward_list_node and hash_node

[akhuang]

Seems like it's a little less straightforward for some of the other types (__tree_node, __value_type and __hash_value_type), especially since they explicitly delete the constructors/destructors and I don't have a great understanding of why. Should probably get @EricWF's opinion on this?

I'm still somewhat leaning toward just adding an attribute to those types to solve the debug info issue (https://reviews.llvm.org/D98750)

libcxx/include/list

285–288

apparently using =default complains because __value_ doesn't have a default constructor

[EricWF]

Has this been tested with various sanitizers on? Does it give warnings about weird object lifetimes?

libcxx/include/list

274

I'm a bit worried that there are combinations of types and compiler flags that will align a subobject differently when it's in a union than when it's not.
I need to investigate further.

1162

Maybe we can sink these repeated lines of code into a function?

[EricWF]

In D101206#2721174, @akhuang wrote:

Seems like it's a little less straightforward for some of the other types (__tree_node, __value_type and __hash_value_type), especially since they explicitly delete the constructors/destructors and I don't have a great understanding of why. Should probably get @EricWF's opinion on this?

I'm still somewhat leaning toward just adding an attribute to those types to solve the debug info issue (https://reviews.llvm.org/D98750)

I agree that the attribute is a non-complicated and good way to address those types.

[akhuang]

In D101206#2725743, @EricWF wrote:

Has this been tested with various sanitizers on? Does it give warnings about weird object lifetimes?

Tried running tests with sanitizers (Address, Memory, Undefined) - doesn't look like there are any new failures.

[akhuang]

Move some of the #ifndefs in forward_list into a function

[akhuang]

Fix #ifdef for function

[ldionne]

[Github PR transition cleanup]

Commandeering to poke CI, rebase, etc.

[ldionne]

Update. This is still a WIP. This rebases the patch and reworks how it's achieved to

1. Increase consistency
2. Remove code duplication
3. Simplify the code

The resulting patch touches more lines, but it becomes a lot easier to verify the correctness of the resulting code.

This is still a WIP, I need to update std::list but this is enough for today.

[ldionne]

I am opting to do a refactoring in https://github.com/llvm/llvm-project/pull/65614 first.

[ldionne]

Rebase on top of refactoring in https://github.com/llvm/llvm-project/pull/65614

[dblaikie]

This also allows removing the _LIBCPP_STANDALONE_DEBUG attribute from a few types since constructor homing should now work for them. For more details, see https://reviews.llvm.org/D98750

Awesome! I'm really glad to see this & the UB being cleaned up.

(though this does mean the debug info will be broken for C++03 mode, yeah? I don't think we/Google care about that, but other people might... )

[ldionne]

In D101206#4646845, @dblaikie wrote:

(though this does mean the debug info will be broken for C++03 mode, yeah? I don't think we/Google care about that, but other people might... )

I could keep the _LIBCPP_STANDALONE_DEBUG attribute on the classes for the C++03 case, I guess!

[ldionne]

Rebase onto main, which contains the refactoring. Keep _LIBCPP_STANDALONE_DEBUG on the node types to avoid breaking debug info in C++03.

I don't know how to add a test for this change, suggestions welcome.

[dblaikie]

In D101206#4647403, @ldionne wrote:

Rebase onto main, which contains the refactoring. Keep _LIBCPP_STANDALONE_DEBUG on the node types to avoid breaking debug info in C++03.

I don't know how to add a test for this change, suggestions welcome.

I don't think the original commit had test coverage for this, probably - so "just as good" to still not have it.

I think the way to test it, if you wanted to, would be to llvm-dwarfdump a simple usage and check that the type definition is available.

It'd look something like...

RUN: clang x.cpp -g -c -o %t.o
RUN: llvm-dwarfdump %t.o | FileCheck %s

CHECK:   DW_AT_name "SomeType"
CHECK-NOT: DW_AT_declaration
CHECK-NOT: DW_TAG
CHECK:  DW_AT_byte_size
CHECK: DW_TAG

(This is sort of hedging my bets a bit - checking both that it's not a declaration, and that it does have a size (size isn't included in declarations) - there's some risk that the attributes are out of order and that declaration or size could come before name, but I don't think LLVM produces type attributes in that order - pretty sure the name comes first)

[ldionne]

In D101206#4647492, @dblaikie wrote:

RUN: clang x.cpp -g -c -o %t.o
RUN: llvm-dwarfdump %t.o | FileCheck %s

CHECK:   DW_AT_name "SomeType"
CHECK-NOT: DW_AT_declaration
CHECK-NOT: DW_TAG
CHECK:  DW_AT_byte_size
CHECK: DW_TAG

(This is sort of hedging my bets a bit - checking both that it's not a declaration, and that it does have a size (size isn't included in declarations) - there's some risk that the attributes are out of order and that declaration or size could come before name, but I don't think LLVM produces type attributes in that order - pretty sure the name comes first)

Thanks for the suggestion! We don't have access to FileCheck in the libc++ tests unfortunately, so this would require a bunch of additional work (as outlined in https://github.com/llvm/llvm-project/pull/65917). I also tried running this locally and I don't see anything that looks like DW_AT_name "SomeType" (where I imagine SomeType should be e.g. __list_node in some form)?

[dblaikie]

In D101206#4647500, @ldionne wrote:

In D101206#4647492, @dblaikie wrote:

RUN: clang x.cpp -g -c -o %t.o
RUN: llvm-dwarfdump %t.o | FileCheck %s

CHECK:   DW_AT_name "SomeType"
CHECK-NOT: DW_AT_declaration
CHECK-NOT: DW_TAG
CHECK:  DW_AT_byte_size
CHECK: DW_TAG

(This is sort of hedging my bets a bit - checking both that it's not a declaration, and that it does have a size (size isn't included in declarations) - there's some risk that the attributes are out of order and that declaration or size could come before name, but I don't think LLVM produces type attributes in that order - pretty sure the name comes first)

Thanks for the suggestion! We don't have access to FileCheck in the libc++ tests unfortunately, so this would require a bunch of additional work (as outlined in https://github.com/llvm/llvm-project/pull/65917). I also tried running this locally and I don't see anything that looks like DW_AT_name "SomeType" (where I imagine SomeType should be e.g. __list_node in some form)?

looks like it's __forward_list_node in this case? Here's an example with/without the attribute at some version of libc++ I have installed locally:

$ clang++-tot test.cpp -g -c -stdlib=libc++ && llvm-dwarfdump-tot test.o | grep "DW_AT_name.*\"__forward_list_node<" -A5
                    DW_AT_name  ("__forward_list_node<int, void *>")
                    DW_AT_byte_size     (0x10)
                    DW_AT_decl_file     ("/usr/local/google/home/blaikie/dev/llvm/build/default/bin/../include/c++/v1/forward_list")
                    DW_AT_decl_line     (324)
...

$ clang++-tot test.cpp -g -c -stdlib=libc++ && llvm-dwarfdump-tot test.o | grep "DW_AT_name.*\"__forward_list_node<" -A5
                    DW_AT_name  ("__forward_list_node<int, void *>")
                    DW_AT_declaration   (true)
...

[EricWF]

In D101206#4647500, @ldionne wrote:

In D101206#4647492, @dblaikie wrote:

RUN: clang x.cpp -g -c -o %t.o
RUN: llvm-dwarfdump %t.o | FileCheck %s

CHECK:   DW_AT_name "SomeType"
CHECK-NOT: DW_AT_declaration
CHECK-NOT: DW_TAG
CHECK:  DW_AT_byte_size
CHECK: DW_TAG

(This is sort of hedging my bets a bit - checking both that it's not a declaration, and that it does have a size (size isn't included in declarations) - there's some risk that the attributes are out of order and that declaration or size could come before name, but I don't think LLVM produces type attributes in that order - pretty sure the name comes first)

Thanks for the suggestion! We don't have access to FileCheck in the libc++ tests unfortunately, so this would require a bunch of additional work (as outlined in https://github.com/llvm/llvm-project/pull/65917). I also tried running this locally and I don't see anything that looks like DW_AT_name "SomeType" (where I imagine SomeType should be e.g. __list_node in some form)?

We do have access to filecheck sometimes. We could just check for the binary, and make our decision based off that. Though, that means we'll likely not have the test running on the bots (right away), but we can still write the test.
I don't know that it'll provide a ton of value though.

---

On another note, I think this patch could be a lot smaller and introduce fewer #ifdef branches. I think we should simply default-construct the node, then initialize each member as we were before. We can also skip the calls to the destructors because they have no observable effects.
This has the benefit of reducing the amount of code duplication between the branches, as well as keeping the pre/post conditions of the node type after construction the same. You don't need to add additional constructors.

Let me know if I've missed something.

libcxx/include/__hash_table

1394 ↗

(On Diff #556950)

I don't know that we need to call the destructor here.
Do we?

1394 ↗

(On Diff #556950)

Lets drop the calls to the destructor. They should have no affect, and so it's more readable without them as it makes clear they don't perform any user-observable behaviors.

2235 ↗

(On Diff #556950)

Why are we initializing more in this branch than we are in the other?

I would rather keep the two sets of behavior as convergent as possible.

2235 ↗

(On Diff #556950)

In fact, why not simply make the constructor an nop, then keep using the code below to initialize the members.

It's easier to reason about one conditional compilation block than it is two.

2235 ↗

(On Diff #556950)

Same note as above. Simply default construct the object, then initialize it the same way we were.

libcxx/include/forward_list

330 ↗

(On Diff #556950)

Is there a correctness reason for using the union over some other form of aligned storage?

331 ↗

(On Diff #556950)

I don't like macros that get defined mid-file like this. We typically don't do it, and I would like to avoid it.

621 ↗

(On Diff #556950)

Same comment here.

libcxx/include/list

287

Do we cause ourselves any problems not starting the lifetime of the object in C++03 mode now that it's got a user defined destructor?

I suspect not, because it's not like the type was always trivially destructible, but maybe?

[ldionne]

libcxx/include/__hash_table

1394 ↗

(On Diff #556950)

As discussed during live review, we're going to call the destructor for symmetry with calling construct_at.

2235 ↗

(On Diff #556950)

As discussed during live review, the new approach will initialize the node in all standard modes.

libcxx/include/forward_list

330 ↗

(On Diff #556950)

As discussed, I think the main benefit is readability and probably the debugging experience in non-C++03 mode.

libcxx/include/list

287

This will become moot with the new approach we discussed.

[ldionne]

Update after discussion with Eric. This allows fixing the UB in all standard modes and simplifies the code quite a bit!

[dblaikie]

libcxx/include/forward_list

330 ↗

(On Diff #556950)

Readability seems a bit limited if the cost is #ifdefs and having the other implementation here too, though, yeah? Like this code would be easier, I think, to read and modify if there was only one implementation even if it's the aligned storage one, rather than having two. Less is easier to read, generally.

Debuggability - yeah, that's true, though probably not super important except for standard library authors? (& sounds like you're not convinced/certain it'd be especially helpful to you personally?) - if you want to inspect the value in the node directly, without invoking any code (eg: debugging a core dump) then the union is easier (you don't have to write out the cast in a debugger expression evaluator to get the value from the aligned storage)

(I'm not making some firm statement here - just my 2c, feel free to disregard)

[dblaikie]

In D101206#4651782, @ldionne wrote:

Update after discussion with Eric. This allows fixing the UB in all standard modes and simplifies the code quite a bit!

Oh, also meant to say: This is a great improvement!

[ldionne]

Fix CI issues.

[ldionne]

libcxx/include/forward_list

330 ↗

(On Diff #556950)

I don't have a strong opinion, but I think the "inspecting a value from the debugger is easier with a union" was the main motivating factor for doing this.

[dblaikie]

libcxx/include/forward_list

330 ↗

(On Diff #556950)

Fair - yeah, if I were maintaining only one version and I could write the union one, I would. But if it comes to maintaining two versions - I probably wouldn't bother with two versions for that benefit. *shrug*

[ldionne]

Fix gnu_cxx test on GCC. Remove CFI tests which are unnecessary according to EricWF (after offline discussion).

[Michael137]

Looks like this broke the LLDB buildbot on Darwin: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/61029/console

Specifically the formatters:

Failed Tests (2):
  lldb-api :: commands/expression/import-std-module/queue/TestQueueFromStdModule.py
  lldb-api :: functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py

AssertionError: Ran command:
"frame variable map"

Got output:
(std::unordered_map<int, std::string>) map = size=5 {}

Expecting regex pattern: "std::unordered_map" (was found, matched "std::unordered_map")
Expecting regex pattern: "\[0\] = \{\s*first = " (was not found)

Presumably because we removed the _LIBCPP_STANDALONE_DEBUG. I don't think we do constructor homing on Darwin since we do -fstandalone-debug

[krasimir]

Temporarily rolled back due to the LLDB failures.

[dblaikie]

In D101206#4653253, @Michael137 wrote:

Looks like this broke the LLDB buildbot on Darwin: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/61029/console

Specifically the formatters:

Failed Tests (2):
  lldb-api :: commands/expression/import-std-module/queue/TestQueueFromStdModule.py
  lldb-api :: functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py

AssertionError: Ran command:
"frame variable map"

Got output:
(std::unordered_map<int, std::string>) map = size=5 {}

Expecting regex pattern: "std::unordered_map" (was found, matched "std::unordered_map")
Expecting regex pattern: "\[0\] = \{\s*first = " (was not found)

Presumably because we removed the _LIBCPP_STANDALONE_DEBUG. I don't think we do constructor homing on Darwin since we do -fstandalone-debug

Adding/removing the attribute shouldn't make any difference on Darwin. (the standalone debug attribute says "treat this type as though it were compiled with -fstandalone-debug" and Darwin's default is -fstandalone-debug - so adding/removing the attribute shouldn't change the behavior - it's possible there are bugs here, but I'd wager that's not the case/is a red herring)

[Michael137]

In D101206#4653303, @dblaikie wrote:

In D101206#4653253, @Michael137 wrote:

Looks like this broke the LLDB buildbot on Darwin: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/61029/console

Specifically the formatters:

Failed Tests (2):
  lldb-api :: commands/expression/import-std-module/queue/TestQueueFromStdModule.py
  lldb-api :: functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py

AssertionError: Ran command:
"frame variable map"

Got output:
(std::unordered_map<int, std::string>) map = size=5 {}

Expecting regex pattern: "std::unordered_map" (was found, matched "std::unordered_map")
Expecting regex pattern: "\[0\] = \{\s*first = " (was not found)

Presumably because we removed the _LIBCPP_STANDALONE_DEBUG. I don't think we do constructor homing on Darwin since we do -fstandalone-debug

Adding/removing the attribute shouldn't make any difference on Darwin. (the standalone debug attribute says "treat this type as though it were compiled with -fstandalone-debug" and Darwin's default is -fstandalone-debug - so adding/removing the attribute shouldn't change the behavior - it's possible there are bugs here, but I'd wager that's not the case/is a red herring)

Good point, though I did confirm that this patch caused the breakage.
Reason I pointed it at the attribute was following error:

error: warning: warning: got name from symbols: C
error: Couldn't look up symbols:
  __ZNSt3__19__voidifyB7v180000INS_11__list_nodeI1CPvEEEES3_RT_

Which on first glance sounds like a "lack of debug info" problem. Though your point makes sense, it shouldn't affect Darwin since it's already the default. Will investigate what else it could be

[Michael137]

In D101206#4653387, @Michael137 wrote:

In D101206#4653303, @dblaikie wrote:

In D101206#4653253, @Michael137 wrote:

Looks like this broke the LLDB buildbot on Darwin: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/61029/console

Specifically the formatters:

Failed Tests (2):
  lldb-api :: commands/expression/import-std-module/queue/TestQueueFromStdModule.py
  lldb-api :: functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py

AssertionError: Ran command:
"frame variable map"

Got output:
(std::unordered_map<int, std::string>) map = size=5 {}

Expecting regex pattern: "std::unordered_map" (was found, matched "std::unordered_map")
Expecting regex pattern: "\[0\] = \{\s*first = " (was not found)

Presumably because we removed the _LIBCPP_STANDALONE_DEBUG. I don't think we do constructor homing on Darwin since we do -fstandalone-debug

Adding/removing the attribute shouldn't make any difference on Darwin. (the standalone debug attribute says "treat this type as though it were compiled with -fstandalone-debug" and Darwin's default is -fstandalone-debug - so adding/removing the attribute shouldn't change the behavior - it's possible there are bugs here, but I'd wager that's not the case/is a red herring)

Good point, though I did confirm that this patch caused the breakage.
Reason I pointed it at the attribute was following error:

error: warning: warning: got name from symbols: C
error: Couldn't look up symbols:
  __ZNSt3__19__voidifyB7v180000INS_11__list_nodeI1CPvEEEES3_RT_

Which on first glance sounds like a "lack of debug info" problem. Though your point makes sense, it shouldn't affect Darwin since it's already the default. Will investigate what else it could be

Ah the problem is most likely because __value_ is now wrapped in an anonymous union. Which breaks the formatter's assumption:

// In LibCxxUnorderedMap.cpp
node_sp = m_next_element->Cast(m_node_type.GetPointerType()) 
        ->Dereference(error);                                            
if (!node_sp || error.Fail())                                               
    return nullptr;                                                         
                                                                            
value_sp = node_sp->GetChildMemberWithName("__value_");

Should be able to fix this fairly quick. The other test is failing for a different reason by the looks of it. But it is for a much less widely used feature and I don't think it really needs to block this from landing if we don't get to the bottom of it soon.

Side note, we should really improve the error reporting when these formatters fail (currently I don't see any). So we can get to the bottom of the failures more quickly.

[Michael137]

Proposed LLDB fix: https://github.com/llvm/llvm-project/pull/68574

[dblaikie]

In D101206#4653421, @Michael137 wrote:

Proposed LLDB fix: https://github.com/llvm/llvm-project/pull/68574

Personally - maybe this added complexity (combined with the existing slight increase in complexity of having #ifdef'd code) is enough to tip in favor of having the one portable implementation - despite the possible slight reduction in implementation debuggability, since it doesn't sound like anyone's here saying they really need that feature?

[ldionne]

In D101206#4653478, @dblaikie wrote:

In D101206#4653421, @Michael137 wrote:

Proposed LLDB fix: https://github.com/llvm/llvm-project/pull/68574

Personally - maybe this added complexity (combined with the existing slight increase in complexity of having #ifdef'd code) is enough to tip in favor of having the one portable implementation - despite the possible slight reduction in implementation debuggability, since it doesn't sound like anyone's here saying they really need that feature?

I agree. However, after speaking with @Michael137 , it would actually make LLDB's life harder if I were to always use the C++03 variant because right now they only handle the union version of the code and the previous version of the code. @Michael137 tells me I can re-commit this now, so I'll do that.

[ldionne]

Actually I'll wait for tomorrow morning so the LLDB folks can follow-up with the fix quickly after.

[dblaikie]

In D101206#4653863, @ldionne wrote:

In D101206#4653478, @dblaikie wrote:

In D101206#4653421, @Michael137 wrote:

Proposed LLDB fix: https://github.com/llvm/llvm-project/pull/68574

Personally - maybe this added complexity (combined with the existing slight increase in complexity of having #ifdef'd code) is enough to tip in favor of having the one portable implementation - despite the possible slight reduction in implementation debuggability, since it doesn't sound like anyone's here saying they really need that feature?

I agree. However, after speaking with @Michael137 , it would actually make LLDB's life harder if I were to always use the C++03 variant because right now they only handle the union version of the code and the previous version of the code. @Michael137 tells me I can re-commit this now, so I'll do that.

Oh, it looks like it would handle the 03 variant today - because it'd find the __value_ member on the first lookup, then it only falls back to looking through the anonymous union if that fails? Or does that fail later on because the __value_ member isn't of the expected type in the 03 case (since it's just the raw buffer).

Pity to lose the ability to pretty print the 03 version :/

[Michael137]

In D101206#4653905, @dblaikie wrote:

In D101206#4653863, @ldionne wrote:

In D101206#4653478, @dblaikie wrote:

In D101206#4653421, @Michael137 wrote:

Proposed LLDB fix: https://github.com/llvm/llvm-project/pull/68574

Personally - maybe this added complexity (combined with the existing slight increase in complexity of having #ifdef'd code) is enough to tip in favor of having the one portable implementation - despite the possible slight reduction in implementation debuggability, since it doesn't sound like anyone's here saying they really need that feature?

I agree. However, after speaking with @Michael137 , it would actually make LLDB's life harder if I were to always use the C++03 variant because right now they only handle the union version of the code and the previous version of the code. @Michael137 tells me I can re-commit this now, so I'll do that.

Oh, it looks like it would handle the 03 variant today - because it'd find the __value_ member on the first lookup, then it only falls back to looking through the anonymous union if that fails? Or does that fail later on because the __value_ member isn't of the expected type in the 03 case (since it's just the raw buffer).

Pity to lose the ability to pretty print the 03 version :/

We can certainly add support for the C++03 format too. I don't think have any test coverage for C++03 but wouldn't be too difficult to add. Will have a look at that next week

libcxx/include/list

__node_pointer __as_node() {
         return static_cast<__node_pointer>(__self());
     }
 };
 
 template <class _Tp, class _VoidPtr>
 struct __list_node
     : public __list_node_base<_Tp, _VoidPtr>
 {
+
+#ifndef _LIBCPP_CXX03_LANG
+    union {
       _Tp __value_;
+    };
+#else
+    typename aligned_storage<sizeof(_Tp)>::type __value_;
+#endif
 
     typedef __list_node_base<_Tp, _VoidPtr> __base;
     typedef typename __base::__link_pointer __link_pointer;
 
     _LIBCPP_INLINE_VISIBILITY
+    __list_node() {}
+
+    _LIBCPP_INLINE_VISIBILITY
     __link_pointer __as_link() {
         return static_cast<__link_pointer>(__base::__self());
     }
 };
 
 template <class _Tp, class _Alloc = allocator<_Tp> > class _LIBCPP_TEMPLATE_VIS list;
 template <class _Tp, class _Alloc> class __list_imp;
 template <class _Tp, class _VoidPtr> class _LIBCPP_TEMPLATE_VIS __list_const_iterator;
#endif // _LIBCPP_DEBUG_LEVEL == 2
 
     _LIBCPP_INLINE_VISIBILITY
     reference operator*() const
     {
 #if _LIBCPP_DEBUG_LEVEL == 2
         _LIBCPP_ASSERT(__get_const_db()->__dereferenceable(this),
                        "Attempted to dereference a non-dereferenceable list::iterator");
 #endif
+#ifndef _LIBCPP_CXX03_LANG
         return __ptr_->__as_node()->__value_;
+#else
+        return *reinterpret_cast<value_type*>(&__ptr_->__as_node()->__value_);
+#endif
     }
     _LIBCPP_INLINE_VISIBILITY
     pointer operator->() const
     {
 #if _LIBCPP_DEBUG_LEVEL == 2
         _LIBCPP_ASSERT(__get_const_db()->__dereferenceable(this),
                        "Attempted to dereference a non-dereferenceable list::iterator");
 #endif

 #endif // _LIBCPP_DEBUG_LEVEL == 2
     _LIBCPP_INLINE_VISIBILITY
     reference operator*() const
     {
 #if _LIBCPP_DEBUG_LEVEL == 2
         _LIBCPP_ASSERT(__get_const_db()->__dereferenceable(this),
                        "Attempted to dereference a non-dereferenceable list::const_iterator");
 #endif
+
+#ifndef _LIBCPP_CXX03_LANG
         return __ptr_->__as_node()->__value_;
+#else
+        return *reinterpret_cast<value_type*>(&__ptr_->__as_node()->__value_);
+#endif
     }
     _LIBCPP_INLINE_VISIBILITY
     pointer operator->() const
     {
 #if _LIBCPP_DEBUG_LEVEL == 2
         _LIBCPP_ASSERT(__get_const_db()->__dereferenceable(this),
                        "Attempted to dereference a non-dereferenceable list::const_iterator");
 #endif
+
+#ifndef _LIBCPP_CXX03_LANG
         return pointer_traits<pointer>::pointer_to(__ptr_->__as_node()->__value_);
+#else
+        return pointer_traits<pointer>::pointer_to(
+            *reinterpret_cast<value_type*>(&__ptr_->__as_node()->__value_));
+#endif
     }
 
     _LIBCPP_INLINE_VISIBILITY
     __list_const_iterator& operator++()
     {
 #if _LIBCPP_DEBUG_LEVEL == 2
         _LIBCPP_ASSERT(__get_const_db()->__dereferenceable(this),
                        "Attempted to increment non-incrementable list::const_iterator");
if (!empty())
         __link_pointer __f = __end_.__next_;
         __link_pointer __l = __end_as_link();
         __unlink_nodes(__f, __l->__prev_);
         __sz() = 0;
         while (__f != __l)
         {
             __node_pointer __np = __f->__as_node();
             __f = __f->__next_;
+#ifndef _LIBCPP_CXX03_LANG
             __node_alloc_traits::destroy(__na, _VSTD::addressof(__np->__value_));
+#else
+            __node_alloc_traits::destroy(__na, _VSTD::addressof(
+                *reinterpret_cast<value_type*>(&__np->__value_)));
+#endif
             __node_alloc_traits::deallocate(__na, __np, 1);
         }
         __invalidate_all_iterators();
     }
 }
 
 template <class _Tp, class _Alloc>
 void
#endif // _LIBCPP_CXX03_LANG
     _LIBCPP_INLINE_VISIBILITY
     const_reverse_iterator crend()   const _NOEXCEPT
         {return const_reverse_iterator(begin());}
 
     _LIBCPP_INLINE_VISIBILITY
     reference front()
     {
         _LIBCPP_ASSERT(!empty(), "list::front called on empty list");
+#ifndef _LIBCPP_CXX03_LANG
         return base::__end_.__next_->__as_node()->__value_;
+#else
+        return *reinterpret_cast<value_type*>(&base::__end_.__next_->__as_node()->__value_);
+#endif
     }
     _LIBCPP_INLINE_VISIBILITY
     const_reference front() const
     {
         _LIBCPP_ASSERT(!empty(), "list::front called on empty list");
+#ifndef _LIBCPP_CXX03_LANG
         return base::__end_.__next_->__as_node()->__value_;
+#else
+        return *reinterpret_cast<value_type*>(&base::__end_.__next_->__as_node()->__value_);
+#endif
     }
     _LIBCPP_INLINE_VISIBILITY
     reference back()
     {
         _LIBCPP_ASSERT(!empty(), "list::back called on empty list");
+#ifndef _LIBCPP_CXX03_LANG
         return base::__end_.__prev_->__as_node()->__value_;
+#else
+        return *reinterpret_cast<value_type*>(&base::__end_.__prev_->__as_node()->__value_);
+#endif
     }
     _LIBCPP_INLINE_VISIBILITY
     const_reference back() const
     {
         _LIBCPP_ASSERT(!empty(), "list::back called on empty list");
+#ifndef _LIBCPP_CXX03_LANG
         return base::__end_.__prev_->__as_node()->__value_;
+#else
+        return *reinterpret_cast<value_type*>(&base::__end_.__prev_->__as_node()->__value_);
+#endif
     }
 
 #ifndef _LIBCPP_CXX03_LANG
     void push_front(value_type&& __x);
     void push_back(value_type&& __x);
 
     template <class... _Args>
 #if _LIBCPP_STD_VER > 14
#endif
     bool __invariants() const;
 
     typedef __allocator_destructor<__node_allocator> __node_destructor;
     typedef unique_ptr<__node, __node_destructor> __hold_pointer;
 
     _LIBCPP_INLINE_VISIBILITY
     __hold_pointer __allocate_node(__node_allocator& __na) {
       __node_pointer __p = __node_alloc_traits::allocate(__na, 1);
+      ::new ((void*)_VSTD::addressof(*__p)) __node();
       __p->__prev_ = nullptr;
       return __hold_pointer(__p, __node_destructor(__na, 1));
     }
 
 #if _LIBCPP_DEBUG_LEVEL == 2
 
     bool __dereferenceable(const const_iterator* __i) const;
     bool __decrementable(const const_iterator* __i) const;

 {
 #if _LIBCPP_DEBUG_LEVEL == 2
     _LIBCPP_ASSERT(__get_const_db()->__find_c_from_i(&__p) == this,
         "list::insert(iterator, x) called with an iterator not"
         " referring to this list");
 #endif
     __node_allocator& __na = base::__node_alloc();
     __hold_pointer __hold = __allocate_node(__na);
+#ifndef _LIBCPP_CXX03_LANG
     __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), __x);
+#else
+    __node_alloc_traits::construct(__na, _VSTD::addressof(
+        *reinterpret_cast<value_type*>(&__hold->__value_)), __x);
+#endif
     __link_nodes(__p.__ptr_, __hold->__as_link(), __hold->__as_link());
     ++base::__sz();
 #if _LIBCPP_DEBUG_LEVEL == 2
     return iterator(__hold.release()->__as_link(), this);
 #else
     return iterator(__hold.release()->__as_link());
 #endif
 }

 #else
     iterator __r(__p.__ptr_);
 #endif
     if (__n > 0)
     {
         size_type __ds = 0;
         __node_allocator& __na = base::__node_alloc();
         __hold_pointer __hold = __allocate_node(__na);
+#ifndef _LIBCPP_CXX03_LANG
         __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), __x);
+#else
+        __node_alloc_traits::construct(__na, _VSTD::addressof(
+            *reinterpret_cast<value_type*>(&__hold->__value_)), __x);
+#endif
+
         ++__ds;
 #if _LIBCPP_DEBUG_LEVEL == 2
         __r = iterator(__hold->__as_link(), this);
 #else
         __r = iterator(__hold->__as_link());
 #endif
         __hold.release();
         iterator __e = __r;
 #ifndef _LIBCPP_NO_EXCEPTIONS
         try
         {
 #endif // _LIBCPP_NO_EXCEPTIONS
             for (--__n; __n != 0; --__n, ++__e, ++__ds)
             {
                 __hold.reset(__node_alloc_traits::allocate(__na, 1));
+#ifndef _LIBCPP_CXX03_LANG
                 __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), __x);
+#else
+                __node_alloc_traits::construct(__na, _VSTD::addressof(
+                    *reinterpret_cast<value_type*>(&__hold->__value_)), __x);
+#endif
                 __e.__ptr_->__next_ = __hold->__as_link();
                 __hold->__prev_ = __e.__ptr_;
                 __hold.release();
             }
 #ifndef _LIBCPP_NO_EXCEPTIONS
         }
         catch (...)
         {

 #else
     iterator __r(__p.__ptr_);
 #endif
     if (__f != __l)
     {
         size_type __ds = 0;
         __node_allocator& __na = base::__node_alloc();
         __hold_pointer __hold = __allocate_node(__na);
+#ifndef _LIBCPP_CXX03_LANG
         __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), *__f);
+#else
+        __node_alloc_traits::construct(__na, _VSTD::addressof(
+            *reinterpret_cast<value_type*>(&__hold->__value_)), *__f);
+#endif
         ++__ds;
 #if _LIBCPP_DEBUG_LEVEL == 2
         __r = iterator(__hold.get()->__as_link(), this);
 #else
         __r = iterator(__hold.get()->__as_link());
 #endif
         __hold.release();
         iterator __e = __r;
 #ifndef _LIBCPP_NO_EXCEPTIONS
         try
         {
 #endif // _LIBCPP_NO_EXCEPTIONS
             for (++__f; __f != __l; ++__f, (void) ++__e, (void) ++__ds)
             {
                 __hold.reset(__node_alloc_traits::allocate(__na, 1));
+#ifndef _LIBCPP_CXX03_LANG
                 __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), *__f);
+#else
+                __node_alloc_traits::construct(__na, _VSTD::addressof(
+                    *reinterpret_cast<value_type*>(&__hold->__value_)), *__f);
+#endif
                 __e.__ptr_->__next_ = __hold.get()->__as_link();
                 __hold->__prev_ = __e.__ptr_;
                 __hold.release();
             }
 #ifndef _LIBCPP_NO_EXCEPTIONS
         }
         catch (...)
         {

 }
 
 template <class _Tp, class _Alloc>
 void
 list<_Tp, _Alloc>::push_front(const value_type& __x)
 {
     __node_allocator& __na = base::__node_alloc();
     __hold_pointer __hold = __allocate_node(__na);
+#ifndef _LIBCPP_CXX03_LANG
     __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), __x);
+#else
+    __node_alloc_traits::construct(__na, _VSTD::addressof(
+        *reinterpret_cast<value_type*>(&__hold->__value_)), __x);
+#endif
+
     __link_pointer __nl = __hold->__as_link();
     __link_nodes_at_front(__nl, __nl);
     ++base::__sz();
     __hold.release();
 }
 
 template <class _Tp, class _Alloc>
 void
 list<_Tp, _Alloc>::push_back(const value_type& __x)
 {
     __node_allocator& __na = base::__node_alloc();
     __hold_pointer __hold = __allocate_node(__na);
+#ifndef _LIBCPP_CXX03_LANG
     __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), __x);
+#else
+    __node_alloc_traits::construct(__na, _VSTD::addressof(
+        *reinterpret_cast<value_type*>(&__hold->__value_)), __x);
+#endif
     __link_nodes_at_back(__hold.get()->__as_link(), __hold.get()->__as_link());
     ++base::__sz();
     __hold.release();
 }
 
 #ifndef _LIBCPP_CXX03_LANG
 
 template <class _Tp, class _Alloc>
 void
 list<_Tp, _Alloc>::push_front(value_type&& __x)
 {
     __node_allocator& __na = base::__node_alloc();
     __hold_pointer __hold = __allocate_node(__na);
+#ifndef _LIBCPP_CXX03_LANG
     __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), _VSTD::move(__x));
+#else
+    __node_alloc_traits::construct(__na, _VSTD::addressof(
+        *reinterpret_cast<value_type*>(&__hold->__value_), _VSTD::move(__x));
+#endif
     __link_nodes_at_front(__hold.get()->__as_link(), __hold.get()->__as_link());
     ++base::__sz();
     __hold.release();
 }
 
 template <class _Tp, class _Alloc>
 void
 list<_Tp, _Alloc>::push_back(value_type&& __x)
 {
     __node_allocator& __na = base::__node_alloc();
     __hold_pointer __hold = __allocate_node(__na);
+#ifndef _LIBCPP_CXX03_LANG
     __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), _VSTD::move(__x));
+#else
+    __node_alloc_traits::construct(__na, _VSTD::addressof(
+        *reinterpret_cast<value_type*>(&__hold->__value_), __x);
+#endif
     __link_nodes_at_back(__hold.get()->__as_link(), __hold.get()->__as_link());
     ++base::__sz();
     __hold.release();
 }
 
 template <class _Tp, class _Alloc>
 template <class... _Args>
 #if _LIBCPP_STD_VER > 14
 typename list<_Tp, _Alloc>::reference
 #else
 void
 #endif
 list<_Tp, _Alloc>::emplace_front(_Args&&... __args)
 {
     __node_allocator& __na = base::__node_alloc();
     __hold_pointer __hold = __allocate_node(__na);
+#ifndef _LIBCPP_CXX03_LANG
     __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), _VSTD::forward<_Args>(__args)...);
+#else
+    __node_alloc_traits::construct(__na, _VSTD::addressof(
+        *reinterpret_cast<value_type*>(&__hold->__value_), _VSTD::forward<_Args>(__args)...);
+#endif
     __link_nodes_at_front(__hold.get()->__as_link(), __hold.get()->__as_link());
     ++base::__sz();
 #if _LIBCPP_STD_VER > 14
+#ifndef _LIBCPP_CXX03_LANG
     return __hold.release()->__value_;
 #else
+    return *reinterpret_cast<value_type*>(&__hold.release()->__value_);
+#endif
+
+#else
     __hold.release();
 #endif
 }
 
 template <class _Tp, class _Alloc>
 template <class... _Args>
 #if _LIBCPP_STD_VER > 14
 typename list<_Tp, _Alloc>::reference
 #else
 void
 #endif
 list<_Tp, _Alloc>::emplace_back(_Args&&... __args)
 {
     __node_allocator& __na = base::__node_alloc();
     __hold_pointer __hold = __allocate_node(__na);
+#ifndef _LIBCPP_CXX03_LANG
     __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), _VSTD::forward<_Args>(__args)...);
+#else
+    __node_alloc_traits::construct(__na, _VSTD::addressof(
+        *reinterpret_cast<value_type*>(&__hold->__value_), _VSTD::forward<_Args>(__args)...);
+#endif
     __link_pointer __nl = __hold->__as_link();
     __link_nodes_at_back(__nl, __nl);
     ++base::__sz();
 #if _LIBCPP_STD_VER > 14
+#ifndef _LIBCPP_CXX03_LANG
     return __hold.release()->__value_;
 #else
+    return *reinterpret_cast<value_type*>(&__hold.release()->__value_);
+#endif
+#else
     __hold.release();
 #endif
 }
 
 template <class _Tp, class _Alloc>
 template <class... _Args>
 typename list<_Tp, _Alloc>::iterator
 list<_Tp, _Alloc>::emplace(const_iterator __p, _Args&&... __args)
list<_Tp, _Alloc>::resize(size_type __n, const value_type& __x)
     if (__n < base::__sz())
         erase(__iterator(__n), end());
     else if (__n > base::__sz())
     {
         __n -= base::__sz();
         size_type __ds = 0;
         __node_allocator& __na = base::__node_alloc();
         __hold_pointer __hold = __allocate_node(__na);
+#ifndef _LIBCPP_CXX03_LANG
         __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), __x);
+#else
+        __node_alloc_traits::construct(__na, _VSTD::addressof(
+            *reinterpret_cast<value_type*>(&__hold->__value_)), __x);
+#endif
         ++__ds;
         __link_pointer __nl = __hold.release()->__as_link();
 #if _LIBCPP_DEBUG_LEVEL == 2
         iterator __r = iterator(__nl, this);
 #else
         iterator __r = iterator(__nl);
 #endif
         iterator __e = __r;
 #ifndef _LIBCPP_NO_EXCEPTIONS
         try
         {
 #endif // _LIBCPP_NO_EXCEPTIONS
             for (--__n; __n != 0; --__n, ++__e, ++__ds)
             {
                 __hold.reset(__node_alloc_traits::allocate(__na, 1));
+#ifndef _LIBCPP_CXX03_LANG
                 __node_alloc_traits::construct(__na, _VSTD::addressof(__hold->__value_), __x);
+#else
+                __node_alloc_traits::construct(__na, _VSTD::addressof(
+                    *reinterpret_cast<value_type*>(&__hold->__value_)), __x);
+#endif
                 __e.__ptr_->__next_ = __hold.get()->__as_link();
                 __hold->__prev_ = __e.__ptr_;
                 __hold.release();
             }
 #ifndef _LIBCPP_NO_EXCEPTIONS
         }
         catch (...)
         {

libcxx/include/optional

 template <class _Tp>
 struct __optional_destruct_base<_Tp, false>
 {
     typedef _Tp value_type;
     static_assert(is_object_v<value_type>,
         "instantiation of optional with a non-object type is undefined behavior");
     union
     {
-        char __null_state_;
+        // char __null_state_;
         value_type __val_;
     };
     bool __engaged_;
 
     _LIBCPP_INLINE_VISIBILITY
     ~__optional_destruct_base()
     {
         if (__engaged_)
             __val_.~value_type();
     }
 
     _LIBCPP_INLINE_VISIBILITY
     constexpr __optional_destruct_base() noexcept
-        :  __null_state_(),
+        :  /* __null_state_(), */
            __engaged_(false) {}
 
     template <class... _Args>
     _LIBCPP_INLINE_VISIBILITY
     constexpr explicit __optional_destruct_base(in_place_t, _Args&&... __args)
         :  __val_(_VSTD::forward<_Args>(__args)...),
            __engaged_(true) {}
 

 template <class _Tp>
 struct __optional_destruct_base<_Tp, true>
 {
     typedef _Tp value_type;
     static_assert(is_object_v<value_type>,
         "instantiation of optional with a non-object type is undefined behavior");
     union
     {
-        char __null_state_;
+        // char __null_state_;
         value_type __val_;
     };
     bool __engaged_;
 
     _LIBCPP_INLINE_VISIBILITY
     constexpr __optional_destruct_base() noexcept
-        :  __null_state_(),
+        :  /* __null_state_(), */
            __engaged_(false) {}
 
     template <class... _Args>
     _LIBCPP_INLINE_VISIBILITY
     constexpr explicit __optional_destruct_base(in_place_t, _Args&&... __args)
         :  __val_(_VSTD::forward<_Args>(__args)...),
            __engaged_(true) {}