[PATCH] [libcxx] Attempt to fix undefined behavior in list, forward_list and __tree.

[Andreas Jacobs]

Strange, I can't see much difference in include/list. Only `list::end()` (with and without `const`) and `__list_node_base::__self()` have been changed and use the new `_FancyPointerCaster<>`, but all this does is replace the old `static_cast`s with `reinterpret_cast`s, if the allocater defines `pointer` to be a built-in pointer. If it is a fancy pointer, your patch doesn't change anything at all! How is that supposed to fix the bug?
I've just checked your patch with my two example files. As I expected, main.cpp works, with or without your patch, and main2.cpp doesn't work, with or without your patch. I'm sorry.

By the way: what is the purpose of `_FancyPointerCaster<>`? I don't quite understand what you need it for, because all it does is choose whether to use a `static_cast` or a `reinterpret_cast`.

The class `__tree` seems more complicated and uses three instead of two node classes: `__tree_node` and `__tree_node_base` work like their counterparts in `list`, and then there is `__tree_end_node` for the end node that is the parent of the tree's root node and the target of the end iterator. As far as I understand it, a `__tree_node_base` is never created: all regular nodes are `__tree_node`s and the end node is a `__tree_end_node`. Therefore I don't expect any aliasing problems between `__tree_node` and `__tree_node_base`, only between `__tree_node` and `__tree_end_node`.
Your patch introduces a new function `__tree::__end_node_pointer()` that returns the end node as a pointer of that base class `__tree_end_node`, and it calls that function whenever it needs to access the end node. This looks good to me and should solve some strict aliasing problems.
Alas, not all of them! The end iterator is a `__tree_iterator` and thus points to the end node with a `__tree_node_pointer`. The `operator--()` casts these to `__node_base_pointer` first, but not to `__end_node_ptr`, so the very moment it accesses the `__left_` field, it breaks the strict aliasing rule again (`operator++()` works the same way, but this is fine, because it would be an error in the application to call `operator++()` on the end node). The same is true for `__tree_node_base::__parent_`, whose type points to a `__tree_node_base`, although the parent of the root node is the end node. Note, that it's safe for `__left_` and `__right_` to point to `__tree_node_base`, because due to the structure of the tree they can never point to the end node: only `__parent_` and the end iterator can, in addition to all functions that call `__end_node()` or `__end_node_pointer()`.
Unfortunately, I cannot present you a counterexample this time: that bug seems to be much harder to trigger in `__tree` than in `std::list`.

Finally, I've had a look into `std::forward_list`. Similar to `__tree`, you have replaced some calls to `__before_begin()` with calls to a new function `__before_begin_pointer()`, which returns the `__before_begin` node as a `__begin_node_pointer` instead of `__node_pointer`.
In my opinion, this is more appropriate and may solve some strict aliasing problems, but not all of them. The function `forward_list::before_begin()` still returns a `__forward_list_iterator<__node_pointer>` that contains a pointer to a `__forward_list_node`, although it actually points to a `__forward_begin_node` instead. Dereferencing that iterator violates strict aliasing, as can be seen from the fourth attachment to the bug entry: It fails, with or without your patch.


http://reviews.llvm.org/D6974

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/