[PATCH] D46845: [libcxx][c++17] P0083R5: Splicing Maps and Sets

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

How do you deal with the fact that node handles for map-like types need to be able to mutate a const member of a `pair` in-place? Are you assuming/hoping the compiler won't do bad things to the program as a result, or is there some reason why it would be unlikely to do so / disallowed from doing so? If you're just casting `pair<const K, V>` to / from `pair<K, V>` (and I think that's what's happening), there is a very real risk that struct-path-sensitive TBAA will decide that `pair<const K, V>::first` and `pair<K, V>::first` cannot alias (and likewise for `::second`) and then miscompile code using libc++'s node pointers.

One way we could deal with this is by adding an attribute to the compiler to indicate "the const is a lie", that we can apply to `std::pair::first`, with the semantics being that a top-level `const` is ignored when determining the "real" type of the member (and so mutating the member after a `const_cast` has defined behavior).  This would apply to //all// `std::pair`s, not just the `node_handle` case, but in practice we don't optimize on the basis of a member being declared `const` *anyway*, so this isn't such a big deal right now.

Another possibility would be a compiler intrinsic to change the type of the pair, in-place, from a `std::pair<const K, V>` to a `std::pair<K, V>`, without changing anything about the members -- except that the `first` member changes type.

Yet another possibility would be to only ever access the `key` field through a type annotated with `__attribute__((may_alias))`, and then launder the node pointer when we're ready to insert it back into the container (to "pick up" the new value of the const member). That's formally breaking the rules (a `launder` isn't enough, because we didn't actually create a new object, and in any case we still mutated the value of a `const` subobject), but it'll probably work fine across compilers that support the attribute.



================
Comment at: libcxx/include/__node_handle:148
+    typedef __node_handle_base<_NodeType, _Alloc> __base;
+    using __base::__node_handle_base;
+
----------------
`using __base::__base;` would be a much more reasonable way to write an inheriting constructor declaration here.


Repository:
  rCXX libc++

https://reviews.llvm.org/D46845