Apply [[standalone_debug]] to some types in the STL.

[akhuang]

Summary

Add this attribute to some types to ensure that they have
debug info.
The debug info for these classes are required for debuggers to display
some STL types. With constructor homing (a new debug info optimization)
their debug info isn't emitted because their constructors are never
called.

The list of types with the attribute added are hash_value_type,
value_type, tree_node_base, tree_node, hash_node, list_node,
and __forward_list_node.

[Mordante]

I'm not familiar with this attribute so leave approval to other more versed with this attribute.

libcxx/include/__config

1354

I would prefer _LIBCPP_STANDALONE_DEBUG to improve readability.

[akhuang]

Change macro name to LIBCPP_STANDALONE_DEBUG

[akhuang]

ping?

[ldionne]

It feels like I've done that before (if so please refresh my memory), but here's a patch trying to remove the UB we have in std::list: https://reviews.llvm.org/D99624. Could you please try it out and see if it fixes the issues you were seeing with __list_node? If it does, then I believe we can apply similar fixes to the other types and fix the underlying problem instead of applying this attribute which will only hide the issue.

[akhuang]

In D98750#2659984, @ldionne wrote:

It feels like I've done that before (if so please refresh my memory), but here's a patch trying to remove the UB we have in std::list: https://reviews.llvm.org/D99624. Could you please try it out and see if it fixes the issues you were seeing with __list_node? If it does, then I believe we can apply similar fixes to the other types and fix the underlying problem instead of applying this attribute which will only hide the issue.

Thanks! I tried out the other patch and it does fix the issues with __list_node.

[ldionne]

In D98750#2660132, @akhuang wrote:

In D98750#2659984, @ldionne wrote:

It feels like I've done that before (if so please refresh my memory), but here's a patch trying to remove the UB we have in std::list: https://reviews.llvm.org/D99624. Could you please try it out and see if it fixes the issues you were seeing with __list_node? If it does, then I believe we can apply similar fixes to the other types and fix the underlying problem instead of applying this attribute which will only hide the issue.

Thanks! I tried out the other patch and it does fix the issues with __list_node.

Thanks for trying it out. Depending on the outcome of the discussion currently happening on D99624, I would suggest that we follow a similar path for the other types instead of "hiding" the issue with the attribute.

If it is instead the case that D99624 isn't actually fixing any UB (see discussion over there), then I believe it would be OK to move forward with this patch.

Also, how do you reproduce the issue you're fixing here? Is there a reasonably easy way to do that so we could add something to the test suite?

[akhuang]

In D98750#2661803, @ldionne wrote:

In D98750#2660132, @akhuang wrote:

In D98750#2659984, @ldionne wrote:

It feels like I've done that before (if so please refresh my memory), but here's a patch trying to remove the UB we have in std::list: https://reviews.llvm.org/D99624. Could you please try it out and see if it fixes the issues you were seeing with __list_node? If it does, then I believe we can apply similar fixes to the other types and fix the underlying problem instead of applying this attribute which will only hide the issue.

Thanks! I tried out the other patch and it does fix the issues with __list_node.

Thanks for trying it out. Depending on the outcome of the discussion currently happening on D99624, I would suggest that we follow a similar path for the other types instead of "hiding" the issue with the attribute.

If it is instead the case that D99624 isn't actually fixing any UB (see discussion over there), then I believe it would be OK to move forward with this patch.

Also, how do you reproduce the issue you're fixing here? Is there a reasonably easy way to do that so we could add something to the test suite?

Sounds good -

So, to reproduce, I've basically been building a cpp file with stuff like

#include <list>
std::list<int> L {1, 2, 3};
# ..and all the other types

int main() { return 0; }

and using gdb to pretty print the types. (or, build the obj file and look in the dwarf dump to see if the __list_node type is a declaration or not)

It looks like there's already a pretty printer test in libc++ (./libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp). It currently fails on my machine for some reason before running the actual tests, but in theory if it worked, it would catch the issues when -fuse-ctor-homing is enabled.

[akhuang]

In D98750#2662041, @akhuang wrote:

It looks like there's already a pretty printer test in libc++ (./libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp). It currently fails on my machine for some reason before running the actual tests, but in theory if it worked, it would catch the issues when -fuse-ctor-homing is enabled.

Actually, it looks like the ./libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp test is working; it's just that when it's run with lit, it doesn't show any details about which parts pass or fail.

[EricWF]

I like this patch. It's targeted and fixes the problem it's aiming at well.

Debug information provides real value to our users. And while it might be ideal to fix the actual implementations so they contain less magic and generate better debug info, that's unrealistic given the nature of the standard library (which needs magic from time to time).

This patch LGTM. I don't think it precludes other improvements to the types as @ldionne was mentioning.

[akhuang]

Update a test case to test the use of this attribute.

[akhuang]

I've updated the ./libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp test so that it runs with constructor homing enabled. One of the cases in the test was already failing, but everything else seems to work. (It only passes if it's run with a version clang that includes my __attribute__((standalone_debug)) change, though).

[akhuang]

Update to only apply attribute to types not covered in
https://reviews.llvm.org/D101206.

Also remove test case changes, which don't work because the test could be using
an older version of clang.

[akhuang]

put attribute back on the other types

[akhuang]

@EricWF @ldionne
I'm thinking it'd be better to commit this patch first, since it seems like fixing the UB otherwise is nontrivial and might take some more time.

If we also want to fix any UB in libc++ we can do that too and then revert this change once it's fixed, but since this solves the issue for the constructor homing debug info optimization it'd be nice to get this in for now.

thoughts?

[rnk]

Yes. I think Amy has demonstrated that we need this attribute, at least for some complex cases. If libc++ is going to use the attribute in at least one place, I don't think there is much cost to adding it in the other places that we think we could address with unions. The attribute can always be removed later if the alleged UB is resolved.

Our motivation is to reduce object file size by 50%, not to address the UB, and this has been the blocking issue for us since October. @EricWF already indicated that he prefers the attribute. @ldionne, you indicated that you'd prefer to avoid the attribute, but if it is unavoidable in some cases, do you have any other objections, or is this OK?

[rnk]

In the absence of more input, I think we should go ahead and land the patch. We have one approval already.

[• Quuxplusone]

FWIW, I think it would be appropriate to wait for @ldionne's OK.
(But I also think it would be appropriate for @ldionne to give his OK. :) This PR seems unobjectionable to me.)

[akhuang]

Hm, ok, I'll wait a few days to see if @ldionne has any thoughts on this, and if not, then I think I'll just commit it.

To summarize the reasoning for going with using this attribute rather than fixing libc++: based on comments from the other patch, it seems like changing libc++ could cause more issues. Also, I couldn't find a way to apply the fix to value_type and hash_value_type, and if we have to use the attribute for those types then we might as well use it on all of them. This patch addresses the issues we have with debug info; while fixing any potential undefined behavior would be nice, it looks like it's not as straightforward to fix as we'd like.

[akhuang]

Ok, I think I'll commit this now since people generally think it's reasonable. I agree it'd be better to wait for @ldionne but it's been waiting for a while and I kind of want to move forward with a fix for this.

libcxx/include/__config

 #if __has_attribute(__nodebug__) && \
     (defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER >= 900)
 #define _LIBCPP_NODEBUG_TYPE __attribute__((nodebug))
 #else
 #define _LIBCPP_NODEBUG_TYPE
 #endif
 #endif // !defined(_LIBCPP_NODEBUG_TYPE)
 
+#if __has_attribute(__standalone_debug__)
+#define _LIBCPP_STANDALONEDEBUG __attribute__((__standalone_debug__))
+#else
+#define _LIBCPP_STANDALONEDEBUG
+#endif
+
 #if __has_attribute(__preferred_name__)
 #define _LIBCPP_PREFERRED_NAME(x) __attribute__((__preferred_name__(x)))
 #else
 #define _LIBCPP_PREFERRED_NAME(x)
 #endif
 
 #if defined(_LIBCPP_ABI_MICROSOFT) && \
     (defined(_LIBCPP_COMPILER_MSVC) || __has_declspec_attribute(empty_bases))

libcxx/include/__hash_table

#endif
     size_t __hash() const _NOEXCEPT {
         return static_cast<__node_type const&>(*this).__hash_;
     }
 
     _LIBCPP_INLINE_VISIBILITY __hash_node_base() _NOEXCEPT : __next_(nullptr) {}
 };
 
 template <class _Tp, class _VoidPtr>
-struct __hash_node
+struct _LIBCPP_STANDALONEDEBUG __hash_node
     : public __hash_node_base
              <
                  typename __rebind_pointer<_VoidPtr, __hash_node<_Tp, _VoidPtr> >::type
              >
 {
     typedef _Tp __node_value_type;
 
     size_t            __hash_;

libcxx/include/__tree

public:
     typedef _Pointer pointer;
     pointer __left_;
 
     _LIBCPP_INLINE_VISIBILITY
     __tree_end_node() _NOEXCEPT : __left_() {}
 };
 
 template <class _VoidPtr>
-class __tree_node_base
+class _LIBCPP_STANDALONEDEBUG __tree_node_base
     : public __tree_node_base_types<_VoidPtr>::__end_node_type
 {
     typedef __tree_node_base_types<_VoidPtr> _NodeBaseTypes;
 
 public:
     typedef typename _NodeBaseTypes::__node_base_pointer pointer;
     typedef typename _NodeBaseTypes::__parent_pointer __parent_pointer;
 

 
 private:
   ~__tree_node_base() _LIBCPP_EQUAL_DELETE;
   __tree_node_base(__tree_node_base const&) _LIBCPP_EQUAL_DELETE;
   __tree_node_base& operator=(__tree_node_base const&) _LIBCPP_EQUAL_DELETE;
 };
 
 template <class _Tp, class _VoidPtr>
-class __tree_node
+class _LIBCPP_STANDALONEDEBUG __tree_node
     : public __tree_node_base<_VoidPtr>
 {
 public:
     typedef _Tp __node_value_type;
 
     __node_value_type __value_;
 
 private:

libcxx/include/forward_list

 struct _LIBCPP_HIDDEN __begin_node_of
 {
     typedef __forward_begin_node<
         typename __rebind_pointer<_VoidPtr, __forward_list_node<_Tp, _VoidPtr> >::type
     > type;
 };
 
 template <class _Tp, class _VoidPtr>
-struct __forward_list_node
+struct _LIBCPP_STANDALONEDEBUG __forward_list_node
     : public __begin_node_of<_Tp, _VoidPtr>::type
 {
     typedef _Tp value_type;
 
     value_type __value_;
 };
 
 

libcxx/include/list

struct __list_node_base
 
     _LIBCPP_INLINE_VISIBILITY
     __node_pointer __as_node() {
         return static_cast<__node_pointer>(__self());
     }
 };
 
 template <class _Tp, class _VoidPtr>
-struct __list_node
+struct _LIBCPP_STANDALONEDEBUG __list_node
     : public __list_node_base<_Tp, _VoidPtr>
 {
     _Tp __value_;
 
     typedef __list_node_base<_Tp, _VoidPtr> __base;
     typedef typename __base::__link_pointer __link_pointer;
 
     _LIBCPP_INLINE_VISIBILITY

libcxx/include/map

template <class _Key, class _Tp, class _Compare, class _Allocator>
     class map;
 template <class _Key, class _Tp, class _Compare, class _Allocator>
     class multimap;
 template <class _TreeIterator> class __map_const_iterator;
 
 #ifndef _LIBCPP_CXX03_LANG
 
 template <class _Key, class _Tp>
-struct __value_type
+struct _LIBCPP_STANDALONEDEBUG __value_type
 {
     typedef _Key                                     key_type;
     typedef _Tp                                      mapped_type;
     typedef pair<const key_type, mapped_type>        value_type;
     typedef pair<key_type&, mapped_type&>            __nc_ref_pair_type;
     typedef pair<key_type&&, mapped_type&&>          __nc_rref_pair_type;
 
 private:

libcxx/include/unordered_map

void operator()(pointer __p) _NOEXCEPT
             __alloc_traits::destroy(__na_, _VSTD::addressof(__p->__value_.__get_value().first));
         if (__p)
             __alloc_traits::deallocate(__na_, __p, 1);
     }
 };
 
 #ifndef _LIBCPP_CXX03_LANG
 template <class _Key, class _Tp>
-struct __hash_value_type
+struct _LIBCPP_STANDALONEDEBUG __hash_value_type
 {
     typedef _Key                                     key_type;
     typedef _Tp                                      mapped_type;
     typedef pair<const key_type, mapped_type>        value_type;
     typedef pair<key_type&, mapped_type&>            __nc_ref_pair_type;
     typedef pair<key_type&&, mapped_type&&>          __nc_rref_pair_type;
 
 private: