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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D46845: [... Richard Smith - zygoloid via Phabricator via cfe-commits

Reply via email to