[libcxx-commits] [PATCH] D101206: [libc++] Remove UB in list, forward_list and __hash_table

Wed Sep 27 12:36:15 PDT 2023

EricWF added a comment.

In D101206#4647500 <https://reviews.llvm.org/D101206#4647500>, @ldionne wrote:

> In D101206#4647492 <https://reviews.llvm.org/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.

================
Comment at: libcxx/include/__hash_table:1394
         __node_traits::destroy(__na, _NodeTypes::__get_ptr(__real_np->__value_));
+#ifdef _LIBCPP_HASH_TABLE_PROPER_NODE_LIFETIME
+        std::__destroy_at(std::addressof(*__real_np));
----------------
I don't know that we need to call the destructor here.
Do we?

================
Comment at: libcxx/include/__hash_table:1394
         __node_traits::destroy(__na, _NodeTypes::__get_ptr(__real_np->__value_));
+#ifdef _LIBCPP_HASH_TABLE_PROPER_NODE_LIFETIME
+        std::__destroy_at(std::addressof(*__real_np));
----------------
EricWF wrote:
> I don't know that we need to call the destructor here.
> Do we?
> 
> 
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. 

================
Comment at: libcxx/include/__hash_table:2235
+#ifdef _LIBCPP_HASH_TABLE_PROPER_NODE_LIFETIME
+    std::__construct_at(std::addressof(*__h), /* next = */nullptr, /* hash = */0);
+#else
----------------
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.

================
Comment at: libcxx/include/__hash_table:2235
+#ifdef _LIBCPP_HASH_TABLE_PROPER_NODE_LIFETIME
+    std::__construct_at(std::addressof(*__h), /* next = */nullptr, /* hash = */0);
+#else
----------------
EricWF wrote:
> 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.
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.

================
Comment at: libcxx/include/__hash_table:2235
+#ifdef _LIBCPP_HASH_TABLE_PROPER_NODE_LIFETIME
+    std::__construct_at(std::addressof(*__h), /* next = */nullptr, /* hash = */0);
+#else
----------------
EricWF wrote:
> EricWF wrote:
> > 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.
> 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.
Same note as above. Simply default construct the object, then initialize it the same way we were.

================
Comment at: libcxx/include/forward_list:330
+    // Allow starting the lifetime of nodes without initializing the value held by the node.
+    // Note that we can't do that in C++03 because unions can't contain non-trivially constructible types.
+#ifndef _LIBCPP_CXX03_LANG
----------------
Is there a correctness reason for using the union over some other form of aligned storage?

================
Comment at: libcxx/include/forward_list:331
+    // Note that we can't do that in C++03 because unions can't contain non-trivially constructible types.
+#ifndef _LIBCPP_CXX03_LANG
+#  define _LIBCPP_FORWARD_LIST_PROPER_NODE_LIFETIME
----------------
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.

================
Comment at: libcxx/include/forward_list:621
+
+#ifdef _LIBCPP_FORWARD_LIST_PROPER_NODE_LIFETIME
+        std::__destroy_at(std::addressof(*__node));
----------------
Same comment here.

================
Comment at: libcxx/include/list:349
+    _LIBCPP_HIDE_FROM_ABI explicit __list_node(__link_pointer __prev, __link_pointer __next) : __base(__prev, __next) {}
+    _LIBCPP_HIDE_FROM_ABI ~__list_node() {}
+
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101206