[PATCH] Fix PR65549, avoid force_decl_die in late compilation

[PATCH] Fix PR65549, avoid force_decl_die in late compilation

[Richard Biener]

For PR65549 the issue is that the force_decl_die DW_TAG_GNU_call_site
resolve_addr does can end up creating DIEs for types we won't emit
(it re-populates the limbo DIE list for the testcase).  For the
particular testcase this happens because the context of the function
called (a lambda type) wasn't needed (it's created/used in another
LTRANS unit).

The DW_TAG_GNU_call_site support doesn't actually need a full DIE
tree for the callee decl (which is external in the case in question):

static void
resolve_addr (dw_die_ref die)
{
...
  FOR_EACH_VEC_SAFE_ELT (die->die_attr, ix, a)
    switch (AT_class (a))
      {
...
      case dw_val_class_addr:
...
        if (die->die_tag == DW_TAG_GNU_call_site
            && a->dw_attr == DW_AT_abstract_origin)
          {
            tree tdecl = SYMBOL_REF_DECL (a->dw_attr_val.v.val_addr);
            dw_die_ref tdie = lookup_decl_die (tdecl);
            if (tdie == NULL
                && DECL_EXTERNAL (tdecl)
                && DECL_ABSTRACT_ORIGIN (tdecl) == NULL_TREE)
              {
                force_decl_die (tdecl);
                tdie = lookup_decl_die (tdecl);

instead it is enough to create a DIE with enough information for
the debugger to associate it with the callee - namely a declaration
with just a name.  This is what the patch does.

Bootstrap and regtest in progress on x86_64-unknown-linux-gnu
(I'm also double-checking LTO bootstrap).

I've manually verified gdb is still happy with the DW_TAG_GNU_call_site
info (not sure if we have a guality test for the feature).

Ok for trunk and affected branches?  (I've also run into this when
doing the LTO early debug work this week which is where the fix
originates).

Thanks,
Richard.

2015-04-17  Richard Biener  <rguenther@suse.de>

	PR debug/65549
	* dwarf2out.c (resolve_addr): Avoid forcing a full DIE for the
	target of a DW_TAG_GNU_call_site during late compilation.
	Instead create a stub DIE without a type.

	* g++.dg/lto/pr65549_0.C: New testcase.

Index: gcc/dwarf2out.c
===================================================================
*** gcc/dwarf2out.c	(revision 222165)
--- gcc/dwarf2out.c	(working copy)
*************** resolve_addr (dw_die_ref die)
*** 23950,23957 ****
  		&& DECL_EXTERNAL (tdecl)
  		&& DECL_ABSTRACT_ORIGIN (tdecl) == NULL_TREE)
  	      {
! 		force_decl_die (tdecl);
! 		tdie = lookup_decl_die (tdecl);
  	      }
  	    if (tdie)
  	      {
--- 23950,23966 ----
  		&& DECL_EXTERNAL (tdecl)
  		&& DECL_ABSTRACT_ORIGIN (tdecl) == NULL_TREE)
  	      {
! 		/* Creating a full DIE for tdecl is overly expensive and
! 		   at this point even wrong when in the LTO phase
! 		   as it can end up generating new type DIEs we didn't
! 		   output and thus optimize_external_refs will crash.  */
! 		tdie = new_die (DW_TAG_subprogram, comp_unit_die (), NULL_TREE);
! 		add_AT_flag (tdie, DW_AT_external, 1);
! 		add_AT_flag (tdie, DW_AT_declaration, 1);
! 		/* GDB is happy with either of the following but add both.  */
! 		add_linkage_attr (tdie, tdecl);
! 		add_name_and_src_coords_attributes (tdie, tdecl);
! 		equate_decl_number_to_die (tdecl, tdie);
  	      }
  	    if (tdie)
  	      {
Index: gcc/testsuite/g++.dg/lto/pr65549_0.C
===================================================================
*** gcc/testsuite/g++.dg/lto/pr65549_0.C	(revision 0)
--- gcc/testsuite/g++.dg/lto/pr65549_0.C	(working copy)
***************
*** 0 ****
--- 1,144 ----
+ // { dg-lto-do link }
+ // { dg-lto-options { { -std=gnu++14 -flto -g } { -std=gnu++14 -flto -g -O2 -fno-inline -flto-partition=max } } }
+ // { dg-extra-ld-options "-r -nostdlib" }
+ 
+ namespace std {
+ inline namespace __cxx11 {}
+ template <typename _Tp, _Tp> struct integral_constant {
+   static constexpr _Tp value = 0;
+ };
+ template <typename> struct __and_;
+ struct is_member_object_pointer : integral_constant<bool, false> {};
+ template <typename>
+ struct is_member_function_pointer : integral_constant<bool, false> {};
+ template <typename> struct remove_reference { typedef int type; };
+ template <typename> class C;
+ template <bool, int, typename...> struct __result_of_impl;
+ template <typename _Functor, typename... _ArgTypes>
+ struct __result_of_impl<false, 0, _Functor, _ArgTypes...> {
+   typedef decltype(0) type;
+ };
+ template <typename _Functor, typename... _ArgTypes>
+ struct C<_Functor(_ArgTypes...)>
+     : __result_of_impl<is_member_object_pointer::value,
+                        is_member_function_pointer<
+                            typename remove_reference<_Functor>::type>::value,
+                        _Functor> {};
+ template <typename _Tp> using result_of_t = typename C<_Tp>::type;
+ template <typename> void forward();
+ template <typename _Tp> _Tp move(_Tp) {}
+ namespace __cxx11 {
+ class basic_string typedef string;
+ }
+ template <typename> struct allocator_traits { typedef decltype(0) pointer; };
+ }
+ struct F : std::allocator_traits<int> {};
+ namespace std {
+ namespace __cxx11 {
+ class basic_string {
+ public:
+   struct _Alloc_hider : F {
+     _Alloc_hider(pointer);
+   } _M_dataplus;
+   basic_string(int) : _M_dataplus(0) {}
+   ~basic_string();
+ };
+ }
+ template <typename> class function;
+ template <typename _Functor> class _Base_manager {
+ protected:
+   static _Functor *_M_get_pointer(int) {}
+ };
+ template <typename, typename> class _Function_handler;
+ template <typename _Res, typename _Functor, typename... _ArgTypes>
+ class _Function_handler<_Res(_ArgTypes...), _Functor>
+     : _Base_manager<_Functor> {
+ public:
+   static _Res _M_invoke(const int &) {
+     (*_Base_manager<_Functor>::_M_get_pointer(0))();
+   }
+ };
+ template <typename, typename> using __check_func_return_type = int;
+ template <typename _Res, typename... _ArgTypes>
+ class function<_Res(_ArgTypes...)> {
+   template <typename> using _Invoke = decltype(0);
+   template <typename _Functor>
+   using _Callable = __and_<__check_func_return_type<_Invoke<_Functor>, _Res>>;
+   template <typename, typename> using _Requires = int;
+ 
+ public:
+   template <typename _Functor, typename = _Requires<_Callable<_Functor>, void>>
+   function(_Functor);
+   using _Invoker_type = _Res (*)(const int &);
+   _Invoker_type _M_invoker;
+ };
+ template <typename _Res, typename... _ArgTypes>
+ template <typename _Functor, typename>
+ function<_Res(_ArgTypes...)>::function(_Functor) {
+   _M_invoker = _Function_handler<_Res(), _Functor>::_M_invoke;
+ }
+ class unique_ptr {
+ public:
+   ~unique_ptr();
+ };
+ template <typename _Tp, typename... _Args> _Tp make_unique(_Args... __args) {
+   _Tp(__args...);
+ }
+ }
+ class A {
+ public:
+   template <class T> T as();
+ };
+ class variables_map {
+ public:
+   A operator[](std::basic_string);
+ };
+ class B {
+ public:
+   variables_map configuration();
+   void run(int, int, std::function<void()>);
+ };
+ class H;
+ struct G {
+   enum {} _state;
+ };
+ class D {
+   G _local_state;
+   std::unique_ptr _task;
+   template <typename Func> void schedule(Func func) {
+     struct task_with_state {
+       task_with_state(Func func) : _func(func) {}
+       Func _func;
+     } tws = std::make_unique<task_with_state>(std::move(func));
+   }
+   friend H;
+ };
+ template <typename> using futurize_t = H;
+ class H {
+   D *_promise;
+   template <typename Func> void schedule(Func func) {
+     G __trans_tmp_1;
+     struct task_with_ready_state {
+       task_with_ready_state(Func, G);
+     };
+     std::make_unique<task_with_ready_state>(std::move(func), __trans_tmp_1);
+     _promise->schedule(std::move(func));
+   }
+   template <typename Func, typename Param> void then(Func func, Param) {
+     using P = D;
+     P pr;
+     schedule([ pr = std::move(pr), func, param = std::forward<Param> ]{});
+   }
+ 
+ public:
+   template <typename Func> futurize_t<std::result_of_t<Func()>> then(Func) {
+     then(0, [] {});
+   }
+ } clients;
+ main() {
+   B app;
+   app.run(0, 0, [&] {
+     auto config = app.configuration()[0].as<std::string>();
+     clients.then([] {});
+   });
+ }

Re: [PATCH] Fix PR65549, avoid force_decl_die in late compilation

[Richard Biener]

On Fri, 17 Apr 2015, Richard Biener wrote:

> 
> For PR65549 the issue is that the force_decl_die DW_TAG_GNU_call_site
> resolve_addr does can end up creating DIEs for types we won't emit
> (it re-populates the limbo DIE list for the testcase).  For the
> particular testcase this happens because the context of the function
> called (a lambda type) wasn't needed (it's created/used in another
> LTRANS unit).
> 
> The DW_TAG_GNU_call_site support doesn't actually need a full DIE
> tree for the callee decl (which is external in the case in question):
> 
> static void
> resolve_addr (dw_die_ref die)
> {
> ...
>   FOR_EACH_VEC_SAFE_ELT (die->die_attr, ix, a)
>     switch (AT_class (a))
>       {
> ...
>       case dw_val_class_addr:
> ...
>         if (die->die_tag == DW_TAG_GNU_call_site
>             && a->dw_attr == DW_AT_abstract_origin)
>           {
>             tree tdecl = SYMBOL_REF_DECL (a->dw_attr_val.v.val_addr);
>             dw_die_ref tdie = lookup_decl_die (tdecl);
>             if (tdie == NULL
>                 && DECL_EXTERNAL (tdecl)
>                 && DECL_ABSTRACT_ORIGIN (tdecl) == NULL_TREE)
>               {
>                 force_decl_die (tdecl);
>                 tdie = lookup_decl_die (tdecl);
> 
> instead it is enough to create a DIE with enough information for
> the debugger to associate it with the callee - namely a declaration
> with just a name.  This is what the patch does.
> 
> Bootstrap and regtest in progress on x86_64-unknown-linux-gnu
> (I'm also double-checking LTO bootstrap).
> 
> I've manually verified gdb is still happy with the DW_TAG_GNU_call_site
> info (not sure if we have a guality test for the feature).
> 
> Ok for trunk and affected branches?  (I've also run into this when
> doing the LTO early debug work this week which is where the fix
> originates).

So Jakub says that using comp_unit_die () for the context of the stub
DIE is wrong and he is of course right.  The following adjusted patch
uses the correct context, but only if we already have a DIE for it,
otherwise we drop the DW_TAG_GNU_call_site ref on the floor.

With DINFO_LEVEL_TERSE we already don't emit a DW_AT_type attribute
for the subprogram DIE so we should be safe not doing that here
either (it's type is another source of the same issue).

Re-testing in progress.

Ok?

Thanks,
Richard.

2015-04-17  Richard Biener  <rguenther@suse.de>

	PR debug/65549
	* dwarf2out.c (lookup_context_die): New function.
	(resolve_addr): Avoid forcing a full DIE for the
	target of a DW_TAG_GNU_call_site during late compilation.
	Instead create a stub DIE without a type if we have a
	context DIE present.

	* g++.dg/lto/pr65549_0.C: New testcase.

Index: gcc/dwarf2out.c
===================================================================
*** gcc/dwarf2out.c	(revision 222165)
--- gcc/dwarf2out.c	(working copy)
*************** is_naming_typedef_decl (const_tree decl)
*** 20617,20622 ****
--- 20617,20644 ----
  	      != TYPE_NAME (TREE_TYPE (decl))));
  }
  
+ /* Looks up the DIE for a context.  */
+ 
+ static inline dw_die_ref
+ lookup_context_die (tree context)
+ {
+   if (context)
+     {
+       /* Find die that represents this context.  */
+       if (TYPE_P (context))
+ 	{
+ 	  context = TYPE_MAIN_VARIANT (context);
+ 	  dw_die_ref ctx = lookup_type_die (context);
+ 	  if (!ctx)
+ 	    return NULL;
+ 	  return strip_naming_typedef (context, ctx);
+ 	}
+       else
+ 	return lookup_decl_die (context);
+     }
+   return comp_unit_die ();
+ }
+ 
  /* Returns the DIE for a context.  */
  
  static inline dw_die_ref
*************** resolve_addr (dw_die_ref die)
*** 23946,23957 ****
  	  {
  	    tree tdecl = SYMBOL_REF_DECL (a->dw_attr_val.v.val_addr);
  	    dw_die_ref tdie = lookup_decl_die (tdecl);
  	    if (tdie == NULL
  		&& DECL_EXTERNAL (tdecl)
! 		&& DECL_ABSTRACT_ORIGIN (tdecl) == NULL_TREE)
  	      {
! 		force_decl_die (tdecl);
! 		tdie = lookup_decl_die (tdecl);
  	      }
  	    if (tdie)
  	      {
--- 23968,23989 ----
  	  {
  	    tree tdecl = SYMBOL_REF_DECL (a->dw_attr_val.v.val_addr);
  	    dw_die_ref tdie = lookup_decl_die (tdecl);
+ 	    dw_die_ref cdie;
  	    if (tdie == NULL
  		&& DECL_EXTERNAL (tdecl)
! 		&& DECL_ABSTRACT_ORIGIN (tdecl) == NULL_TREE
! 		&& (cdie = lookup_context_die (DECL_CONTEXT (tdecl))))
  	      {
! 		/* Creating a full DIE for tdecl is overly expensive and
! 		   at this point even wrong when in the LTO phase
! 		   as it can end up generating new type DIEs we didn't
! 		   output and thus optimize_external_refs will crash.  */
! 		tdie = new_die (DW_TAG_subprogram, cdie, NULL_TREE);
! 		add_AT_flag (tdie, DW_AT_external, 1);
! 		add_AT_flag (tdie, DW_AT_declaration, 1);
! 		add_linkage_attr (tdie, tdecl);
! 		add_name_and_src_coords_attributes (tdie, tdecl);
! 		equate_decl_number_to_die (tdecl, tdie);
  	      }
  	    if (tdie)
  	      {
Index: gcc/testsuite/g++.dg/lto/pr65549_0.C
===================================================================
*** gcc/testsuite/g++.dg/lto/pr65549_0.C	(revision 0)
--- gcc/testsuite/g++.dg/lto/pr65549_0.C	(working copy)
***************
*** 0 ****
--- 1,144 ----
+ // { dg-lto-do link }
+ // { dg-lto-options { { -std=gnu++14 -flto -g } { -std=gnu++14 -flto -g -O2 -fno-inline -flto-partition=max } } }
+ // { dg-extra-ld-options "-r -nostdlib" }
+ 
+ namespace std {
+ inline namespace __cxx11 {}
+ template <typename _Tp, _Tp> struct integral_constant {
+   static constexpr _Tp value = 0;
+ };
+ template <typename> struct __and_;
+ struct is_member_object_pointer : integral_constant<bool, false> {};
+ template <typename>
+ struct is_member_function_pointer : integral_constant<bool, false> {};
+ template <typename> struct remove_reference { typedef int type; };
+ template <typename> class C;
+ template <bool, int, typename...> struct __result_of_impl;
+ template <typename _Functor, typename... _ArgTypes>
+ struct __result_of_impl<false, 0, _Functor, _ArgTypes...> {
+   typedef decltype(0) type;
+ };
+ template <typename _Functor, typename... _ArgTypes>
+ struct C<_Functor(_ArgTypes...)>
+     : __result_of_impl<is_member_object_pointer::value,
+                        is_member_function_pointer<
+                            typename remove_reference<_Functor>::type>::value,
+                        _Functor> {};
+ template <typename _Tp> using result_of_t = typename C<_Tp>::type;
+ template <typename> void forward();
+ template <typename _Tp> _Tp move(_Tp) {}
+ namespace __cxx11 {
+ class basic_string typedef string;
+ }
+ template <typename> struct allocator_traits { typedef decltype(0) pointer; };
+ }
+ struct F : std::allocator_traits<int> {};
+ namespace std {
+ namespace __cxx11 {
+ class basic_string {
+ public:
+   struct _Alloc_hider : F {
+     _Alloc_hider(pointer);
+   } _M_dataplus;
+   basic_string(int) : _M_dataplus(0) {}
+   ~basic_string();
+ };
+ }
+ template <typename> class function;
+ template <typename _Functor> class _Base_manager {
+ protected:
+   static _Functor *_M_get_pointer(int) {}
+ };
+ template <typename, typename> class _Function_handler;
+ template <typename _Res, typename _Functor, typename... _ArgTypes>
+ class _Function_handler<_Res(_ArgTypes...), _Functor>
+     : _Base_manager<_Functor> {
+ public:
+   static _Res _M_invoke(const int &) {
+     (*_Base_manager<_Functor>::_M_get_pointer(0))();
+   }
+ };
+ template <typename, typename> using __check_func_return_type = int;
+ template <typename _Res, typename... _ArgTypes>
+ class function<_Res(_ArgTypes...)> {
+   template <typename> using _Invoke = decltype(0);
+   template <typename _Functor>
+   using _Callable = __and_<__check_func_return_type<_Invoke<_Functor>, _Res>>;
+   template <typename, typename> using _Requires = int;
+ 
+ public:
+   template <typename _Functor, typename = _Requires<_Callable<_Functor>, void>>
+   function(_Functor);
+   using _Invoker_type = _Res (*)(const int &);
+   _Invoker_type _M_invoker;
+ };
+ template <typename _Res, typename... _ArgTypes>
+ template <typename _Functor, typename>
+ function<_Res(_ArgTypes...)>::function(_Functor) {
+   _M_invoker = _Function_handler<_Res(), _Functor>::_M_invoke;
+ }
+ class unique_ptr {
+ public:
+   ~unique_ptr();
+ };
+ template <typename _Tp, typename... _Args> _Tp make_unique(_Args... __args) {
+   _Tp(__args...);
+ }
+ }
+ class A {
+ public:
+   template <class T> T as();
+ };
+ class variables_map {
+ public:
+   A operator[](std::basic_string);
+ };
+ class B {
+ public:
+   variables_map configuration();
+   void run(int, int, std::function<void()>);
+ };
+ class H;
+ struct G {
+   enum {} _state;
+ };
+ class D {
+   G _local_state;
+   std::unique_ptr _task;
+   template <typename Func> void schedule(Func func) {
+     struct task_with_state {
+       task_with_state(Func func) : _func(func) {}
+       Func _func;
+     } tws = std::make_unique<task_with_state>(std::move(func));
+   }
+   friend H;
+ };
+ template <typename> using futurize_t = H;
+ class H {
+   D *_promise;
+   template <typename Func> void schedule(Func func) {
+     G __trans_tmp_1;
+     struct task_with_ready_state {
+       task_with_ready_state(Func, G);
+     };
+     std::make_unique<task_with_ready_state>(std::move(func), __trans_tmp_1);
+     _promise->schedule(std::move(func));
+   }
+   template <typename Func, typename Param> void then(Func func, Param) {
+     using P = D;
+     P pr;
+     schedule([ pr = std::move(pr), func, param = std::forward<Param> ]{});
+   }
+ 
+ public:
+   template <typename Func> futurize_t<std::result_of_t<Func()>> then(Func) {
+     then(0, [] {});
+   }
+ } clients;
+ main() {
+   B app;
+   app.run(0, 0, [&] {
+     auto config = app.configuration()[0].as<std::string>();
+     clients.then([] {});
+   });
+ }

Re: [PATCH] Fix PR65549, avoid force_decl_die in late compilation

[Jakub Jelinek]

On Fri, Apr 17, 2015 at 12:32:03PM +0200, Richard Biener wrote:
> So Jakub says that using comp_unit_die () for the context of the stub
> DIE is wrong and he is of course right.  The following adjusted patch
> uses the correct context, but only if we already have a DIE for it,
> otherwise we drop the DW_TAG_GNU_call_site ref on the floor.

I'd mention that this last line doesn't match what the code does,
because in reality it just means that we drop the DW_AT_abstract_origin
of DW_TAG_GNU_call_site.  That really means just that the debugger won't
know what function is called at that point, as if it were e.g.
an indirect jump with unknown target that is not preserved in any register
across the call), but e.g. all the guarantees that we cover all the calls
still holds.

	Jakub

Re: [PATCH] Fix PR65549, avoid force_decl_die in late compilation

[Richard Biener]

On Fri, 17 Apr 2015, Jakub Jelinek wrote:

> On Fri, Apr 17, 2015 at 12:32:03PM +0200, Richard Biener wrote:
> > So Jakub says that using comp_unit_die () for the context of the stub
> > DIE is wrong and he is of course right.  The following adjusted patch
> > uses the correct context, but only if we already have a DIE for it,
> > otherwise we drop the DW_TAG_GNU_call_site ref on the floor.
> 
> I'd mention that this last line doesn't match what the code does,
> because in reality it just means that we drop the DW_AT_abstract_origin
> of DW_TAG_GNU_call_site.  That really means just that the debugger won't
> know what function is called at that point, as if it were e.g.
> an indirect jump with unknown target that is not preserved in any register
> across the call), but e.g. all the guarantees that we cover all the calls
> still holds.

Ping.

Richard.

Re: [PATCH] Fix PR65549, avoid force_decl_die in late compilation

[Richard Biener]

On Tue, Apr 28, 2015 at 10:37 AM, Richard Biener <rguenther@suse.de> wrote:
> On Fri, 17 Apr 2015, Jakub Jelinek wrote:
>
>> On Fri, Apr 17, 2015 at 12:32:03PM +0200, Richard Biener wrote:
>> > So Jakub says that using comp_unit_die () for the context of the stub
>> > DIE is wrong and he is of course right.  The following adjusted patch
>> > uses the correct context, but only if we already have a DIE for it,
>> > otherwise we drop the DW_TAG_GNU_call_site ref on the floor.
>>
>> I'd mention that this last line doesn't match what the code does,
>> because in reality it just means that we drop the DW_AT_abstract_origin
>> of DW_TAG_GNU_call_site.  That really means just that the debugger won't
>> know what function is called at that point, as if it were e.g.
>> an indirect jump with unknown target that is not preserved in any register
>> across the call), but e.g. all the guarantees that we cover all the calls
>> still holds.
>
> Ping.

Ping again - I'd really like to fix this most-common LTO ICE on the 4.8 branch.
I'm fine with guarding all the code with in_lto_p if you require me to do that.

So - any further guidance please?

For reference, the last patch was at

https://gcc.gnu.org/ml/gcc-patches/2015-04/msg00862.html

Thanks,
Richard.

> Richard.

Re: [PATCH] Fix PR65549, avoid force_decl_die in late compilation

[Jason Merrill]

OK.

Jason