Bug 30413 - [gdb/build] error: storing the address of local variable ‘<anonymous>’ in ‘frame_info_ptr::frame_list.intrusive_list<frame_info_ptr>::m_back’ [-Werror=dangling-pointer=]
Summary: [gdb/build] error: storing the address of local variable ‘<anonymous>’ in ‘fr...
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: build (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: 14.1
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-05-02 16:57 UTC by Tom de Vries
Modified: 2023-05-03 20:15 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom de Vries 2023-05-02 16:57:59 UTC
The buildbot for opensuse tumbleweed fails ( https://builder.sourceware.org/buildbot/#/builders/97/builds/3582/steps/4/logs/stdio ):
...
In file included from ../../binutils-gdb/gdb/frame.h:75,
                 from ../../binutils-gdb/gdb/symtab.h:40,
                 from ../../binutils-gdb/gdb/language.c:33:
In member function ‘void intrusive_list<T, AsNode>::push_empty(T&) [with T = frame_info_ptr; AsNode = intrusive_base_node<frame_info_ptr>]’,
    inlined from ‘void intrusive_list<T, AsNode>::push_back(reference) [with T = frame_info_ptr; AsNode = intrusive_base_node<frame_info_ptr>]’ at ../../binutils-gdb/gdb/../gdbsupport/intrusive_list.h:332:24,
    inlined from ‘frame_info_ptr::frame_info_ptr(const frame_info_ptr&)’ at ../../binutils-gdb/gdb/frame.h:241:26,
    inlined from ‘CORE_ADDR skip_language_trampoline(frame_info_ptr, CORE_ADDR)’ at ../../binutils-gdb/gdb/language.c:530:49:
../../binutils-gdb/gdb/../gdbsupport/intrusive_list.h:415:12: error: storing the address of local variable ‘<anonymous>’ in ‘frame_info_ptr::frame_list.intrusive_list<frame_info_ptr>::m_back’ [-Werror=dangling-pointer=]
  415 |     m_back = &elem;
      |     ~~~~~~~^~~~~~~
../../binutils-gdb/gdb/language.c: In function ‘CORE_ADDR skip_language_trampoline(frame_info_ptr, CORE_ADDR)’:
../../binutils-gdb/gdb/language.c:530:49: note: ‘<anonymous>’ declared here
  530 |       CORE_ADDR real_pc = lang->skip_trampoline (frame, pc);
      |                           ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
../../binutils-gdb/gdb/frame.h:359:41: note: ‘frame_info_ptr::frame_list’ declared here
  359 |   static intrusive_list<frame_info_ptr> frame_list;
      |                                         ^~~~~~~~~~
cc1plus: all warnings being treated as errors
make[1]: *** [Makefile:1922: language.o] Error 1
make[1]: *** Waiting for unfinished jobs....
...
Comment 1 Tom de Vries 2023-05-02 16:59:26 UTC
(In reply to Tom de Vries from comment #0)
> The buildbot for opensuse tumbleweed fails 

Same for rawhide.
Comment 3 Mark Wielaard 2023-05-02 20:54:31 UTC
(In reply to Tom de Vries from comment #1)
> (In reply to Tom de Vries from comment #0)
> > The buildbot for opensuse tumbleweed fails 
> 
> Same for rawhide.

and fedora-latest (that is f38 with an update to gcc 13.1.1)

The patch proposed in comment #2 should fix it.
Comment 4 Tom de Vries 2023-05-03 10:07:27 UTC
FWIW, I've managed to reproduce this outside of the gdb build, while using gdbsupport/intrusive_list.h:
...
$ cat test.C
#include <iterator>

#include <assert.h>
#define gdb_assert(A) assert (A)

#include "src/gdbsupport/intrusive_list.h"

class void_ptr;

class void_ptr : public intrusive_list_node<void_ptr>
{
public:
  void_ptr () : m_ptr (nullptr) {
    void_ptr_list.push_back (*this);
  }

  void_ptr (const void_ptr &other) : m_ptr (other.m_ptr)
  {
    void_ptr_list.push_back (*this);
  }

  ~void_ptr ()
  {
    if (is_linked ())
      void_ptr_list.erase (void_ptr_list.iterator_to (*this));
  }

private:
  mutable void *m_ptr = nullptr;
  static intrusive_list<void_ptr> void_ptr_list;
};

intrusive_list<void_ptr> void_ptr::void_ptr_list;

void 
bar (void_ptr arg)
{

}

void
foo (void_ptr arg)
{
  bar (arg);
}
  
int
main (void)
{
  void_ptr ptr_to_a;
  foo (ptr_to_a);

  return 0;
}
...

With -Wdangling-pointer=0:
...
$ g++ test.C -Wdangling-pointer=0 -pedantic -O1 -g -Werror
$
...

With -Wdangling-pointer=1:
...
$ g++ test.C -Wdangling-pointer=1 -pedantic -O1 -g -Werror
In file included from test.C:6:
In member function ‘void intrusive_list<T, AsNode>::push_empty(T&) [with T = void_ptr; AsNode = intrusive_base_node<void_ptr>]’,
    inlined from ‘void intrusive_list<T, AsNode>::push_back(reference) [with T = void_ptr; AsNode = intrusive_base_node<void_ptr>]’ at src/gdbsupport/intrusive_list.h:332:24,
    inlined from ‘void_ptr::void_ptr(const void_ptr&)’ at test.C:19:29,
    inlined from ‘void foo(void_ptr)’ at test.C:44:7:
src/gdbsupport/intrusive_list.h:415:12: error: storing the address of local variable ‘<anonymous>’ in ‘void_ptr::void_ptr_list.intrusive_list<void_ptr>::m_back’ [-Werror=dangling-pointer=]
  415 |     m_back = &elem;
      |     ~~~~~~~^~~~~~~
test.C: In function ‘void foo(void_ptr)’:
test.C:44:7: note: ‘<anonymous>’ declared here
   44 |   bar (arg);
      |   ~~~~^~~~~
test.C:33:26: note: ‘void_ptr::void_ptr_list’ declared here
   33 | intrusive_list<void_ptr> void_ptr::void_ptr_list;
      |                          ^~~~~~~~
cc1plus: all warnings being treated as errors
...
Comment 5 Tom de Vries 2023-05-03 10:49:39 UTC
I think the thing the warning complains about is that adding to the list is unconditional, and removing from the list is conditional (on is_linked), and the compiler can't figure out that for the temporary object is_linked () == true.

[ You could reason that the warning is too aggressive, but that may be intentional, I'm not sure. ]

The test on is_linked was introduced because of trying to accommodate the scenario mentioned in the comment:
...
  ~frame_info_ptr ()
  {
    /* If this node has static storage, it may be deleted after                      
       frame_list.  Attempting to erase ourselves would then trigger                 
       internal errors, so make sure we are still linked first.  */
    if (is_linked ())
      frame_list.erase (frame_list.iterator_to (*this));
  }
...

So perhaps we can fix this by removing the is_linked test.  The code was added because the static frame_list and the static frame_info_ptr where in different compilation units, but that's no longer the case.

So I'm thinking of:
...
diff --git a/gdb/frame.c b/gdb/frame.c
index 36fb02f3c8e..c47244b8cb2 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1733,6 +1733,11 @@ get_current_frame (void)
 static frame_id selected_frame_id = null_frame_id;
 static int selected_frame_level = -1;
 
+/* See frame-info-ptr.h.  This definition should come before any definition of
+   a static frame_info_ptr.  */
+
+intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
+
 /* The cached frame_info object pointing to the selected frame.
    Looked up on demand by get_selected_frame.  */
 static frame_info_ptr selected_frame;
@@ -3275,10 +3280,6 @@ maintenance_print_frame_id (const char *args, int from_tty)
 
 /* See frame-info-ptr.h.  */
 
-intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
-
-/* See frame-info-ptr.h.  */
-
 frame_info_ptr::frame_info_ptr (struct frame_info *ptr)
   : m_ptr (ptr)
 {
diff --git a/gdb/frame.h b/gdb/frame.h
index 6ed8db0af56..fff5c248070 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -257,8 +257,8 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
     /* If this node has static storage, it may be deleted after
        frame_list.  Attempting to erase ourselves would then trigger
        internal errors, so make sure we are still linked first.  */
-    if (is_linked ())
-      frame_list.erase (frame_list.iterator_to (*this));
+    gdb_assert (is_linked ());
+    frame_list.erase (frame_list.iterator_to (*this));
   }
 
   frame_info_ptr &operator= (const frame_info_ptr &other)


...
Comment 6 Simon Marchi 2023-05-03 13:56:38 UTC
Thanks Tom, this analysis makes sense to me.
Comment 7 Sourceware Commits 2023-05-03 15:11:24 UTC
The master branch has been updated by Mark Wielaard <mark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9b0ccb1ebaef7474d4f7242f778531ef042a55fc

commit 9b0ccb1ebaef7474d4f7242f778531ef042a55fc
Author: Mark Wielaard <mark@klomp.org>
Date:   Tue May 2 20:23:32 2023 +0200

    Pass const frame_info_ptr reference for skip_[language_]trampoline
    
    g++ 13.1.1 produces a -Werror=dangling-pointer=
    
    In file included from ../../binutils-gdb/gdb/frame.h:75,
                     from ../../binutils-gdb/gdb/symtab.h:40,
                     from ../../binutils-gdb/gdb/language.c:33:
    In member function âvoid intrusive_list<T, AsNode>::push_empty(T&) [with T = frame_info_ptr; AsNode = intrusive_base_node<frame_info_ptr>]â,
        inlined from âvoid intrusive_list<T, AsNode>::push_back(reference) [with T = frame_info_ptr; AsNode = intrusive_base_node<frame_info_ptr>]â at gdbsupport/intrusive_list.h:332:24,
        inlined from âframe_info_ptr::frame_info_ptr(const frame_info_ptr&)â at gdb/frame.h:241:26,
        inlined from âCORE_ADDR skip_language_trampoline(frame_info_ptr, CORE_ADDR)â at gdb/language.c:530:49:
    gdbsupport/intrusive_list.h:415:12: error: storing the address of local variable â<anonymous>â in âframe_info_ptr::frame_list.intrusive_list<frame_info_ptr>::m_backâ [-Werror=dangling-pointer=]
      415 |     m_back = &elem;
          |     ~~~~~~~^~~~~~~
    gdb/language.c: In function âCORE_ADDR skip_language_trampoline(frame_info_ptr, CORE_ADDR)â:
    gdb/language.c:530:49: note: â<anonymous>â declared here
      530 |       CORE_ADDR real_pc = lang->skip_trampoline (frame, pc);
          |                           ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
    gdb/frame.h:359:41: note: âframe_info_ptr::frame_listâ declared here
      359 |   static intrusive_list<frame_info_ptr> frame_list;
          |                                         ^~~~~~~~~~
    
    Each new frame_info_ptr is being pushed on a static frame list and g++
    cannot see why that is safe in case the frame_info_ptr is created and
    destroyed immediately when passed as value.
    
    It isn't clear why only in this one place g++ sees the issue (probably
    because it can inline enough code in this specific case).
    
    Since passing the frame_info_ptr as const reference is cheaper, use
    that as workaround for this warning.
    
    PR build/30413
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30413
    
    Tested-by: Kevin Buettner <kevinb@redhat.com>
    Reviewed-by: Kevin Buettner <kevinb@redhat.com>
    Reviewed-by: Tom Tromey <tom@tromey.com>
Comment 8 Mark Wielaard 2023-05-03 15:43:36 UTC
Note that the workaround commit in comment #7 fixed this issue for fedora-rawhide, fedora-latest and suse-tumbleweed on x86_64. But there is another instance of this issue on gentoo-sparc:
https://builder.sourceware.org/buildbot/#/builders/230/builds/1290

In file included from ../../binutils-gdb/gdb/frame.h:75,
                 from ../../binutils-gdb/gdb/frame.c:21:
In member function ‘void intrusive_list<T, AsNode>::push_empty(T&) [with T = frame_info_ptr; AsNode = intrusive_base_node<frame_info_ptr>]’,
    inlined from ‘void intrusive_list<T, AsNode>::push_back(reference) [with T = frame_info_ptr; AsNode = intrusive_base_node<frame_info_ptr>]’ at ../../binutils-gdb/gdb/../gdbsupport/intrusive_list.h:332:24,
    inlined from ‘frame_info_ptr::frame_info_ptr(std::nullptr_t)’ at ../../binutils-gdb/gdb/frame.h:233:26,
    inlined from ‘frame_info_ptr get_next_frame(frame_info_ptr)’ at ../../binutils-gdb/gdb/frame.c:2065:12,
    inlined from ‘void frame_register_unwind_location(frame_info_ptr, int, int*, lval_type*, CORE_ADDR*, int*)’ at ../../binutils-gdb/gdb/frame.c:2159:35:
../../binutils-gdb/gdb/../gdbsupport/intrusive_list.h:415:12: error: storing the address of local variable ‘<anonymous>’ in ‘frame_info_ptr::frame_list.intrusive_list<frame_info_ptr>::m_back’ [-Werror=dangling-pointer=]
  415 |     m_back = &elem;
      |     ~~~~~~~^~~~~~~
../../binutils-gdb/gdb/frame.c: In function ‘void frame_register_unwind_location(frame_info_ptr, int, int*, lval_type*, CORE_ADDR*, int*)’:
../../binutils-gdb/gdb/frame.c:2159:35: note: ‘<anonymous>’ declared here
 2159 |       this_frame = get_next_frame (this_frame);
      |                    ~~~~~~~~~~~~~~~^~~~~~~~~~~~
../../binutils-gdb/gdb/frame.c:3278:32: note: ‘frame_info_ptr::frame_list’ declared here
 3278 | intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
      |                                ^~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[1]: *** [Makefile:1922: frame.o] Error 1
make[1]: *** Waiting for unfinished jobs....
Comment 9 Tom de Vries 2023-05-03 16:40:55 UTC
I've also looked into the following.

The definition order in frame.c is:
...
static frame_info_ptr selected_frame;
  ...
intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
...

All the constructors of frame_info_ptr use frame_list, but selected_frame comes first, so the constructor for selected_frame is called first, in other words frame_list is used before its constructor is called.

But this doesn't cause problems because the constructor is trivial (i.e. performs no action).
Comment 10 Simon Marchi 2023-05-03 16:53:03 UTC
I don't think that intrusive_list is trivially constructible, since we assign the fields like this:

  T *m_front = nullptr;
  T *m_back = nullptr;

I verified using std::is_trivially_constructible.  I suppose that things still work because the object is put in .bss, so the fields are cleared from the start.  Still, I'd put the declarations in the right order, just to be extra safe.
Comment 11 Tom de Vries 2023-05-03 18:00:33 UTC
(In reply to Tom de Vries from comment #5)
> So I'm thinking of:
> ...
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 36fb02f3c8e..c47244b8cb2 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1733,6 +1733,11 @@ get_current_frame (void)
>  static frame_id selected_frame_id = null_frame_id;
>  static int selected_frame_level = -1;
>  
> +/* See frame-info-ptr.h.  This definition should come before any definition
> of
> +   a static frame_info_ptr.  */
> +
> +intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
> +
>  /* The cached frame_info object pointing to the selected frame.
>     Looked up on demand by get_selected_frame.  */
>  static frame_info_ptr selected_frame;
> @@ -3275,10 +3280,6 @@ maintenance_print_frame_id (const char *args, int
> from_tty)
>  
>  /* See frame-info-ptr.h.  */
>  
> -intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
> -
> -/* See frame-info-ptr.h.  */
> -
>  frame_info_ptr::frame_info_ptr (struct frame_info *ptr)
>    : m_ptr (ptr)
>  {
> diff --git a/gdb/frame.h b/gdb/frame.h
> index 6ed8db0af56..fff5c248070 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -257,8 +257,8 @@ class frame_info_ptr : public
> intrusive_list_node<frame_info_ptr>
>      /* If this node has static storage, it may be deleted after
>         frame_list.  Attempting to erase ourselves would then trigger
>         internal errors, so make sure we are still linked first.  */
> -    if (is_linked ())
> -      frame_list.erase (frame_list.iterator_to (*this));
> +    gdb_assert (is_linked ());
> +    frame_list.erase (frame_list.iterator_to (*this));
>    }
>  
>    frame_info_ptr &operator= (const frame_info_ptr &other)
> 
> 
> ...

Submitted here: https://sourceware.org/pipermail/gdb-patches/2023-May/199342.html
Comment 12 Sourceware Commits 2023-05-03 19:43:09 UTC
The master branch has been updated by Tom de Vries <vries@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=751c7c72c0105c7d55e58bba3c069c36a74c8937

commit 751c7c72c0105c7d55e58bba3c069c36a74c8937
Author: Tom de Vries <tdevries@suse.de>
Date:   Wed May 3 21:43:03 2023 +0200

    [gdb/build] Fix frame_list position in frame.c
    
    In commit 995a34b1772 ("Guard against frame.c destructors running before
    frame-info.c's") the following problem was addressed.
    
    The frame_info_ptr destructor:
    ...
      ~frame_info_ptr ()
      {
        frame_list.erase (frame_list.iterator_to (*this));
      }
    ...
    uses frame_list, which is a static member of class frame_info_ptr,
    instantiated in frame-info.c:
    ...
    intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
    ...
    
    Then there's a static frame_info_pointer variable named selected_frame in
    frame.c:
    ...
    static frame_info_ptr selected_frame;
    ...
    
    Because the destructor of selected_frame uses frame_list, its destructor needs
    to be called before the destructor of frame_list.
    
    But because they're in different compilation units, the initialization order and
    consequently destruction order is not guarantueed.
    
    The commit fixed this by handling the case that the destructor of frame_list
    is called first, adding a check on is_linked ():
    ...
       ~frame_info_ptr ()
       {
    -    frame_list.erase (frame_list.iterator_to (*this));
    +    /* If this node has static storage, it may be deleted after
    +       frame_list.  Attempting to erase ourselves would then trigger
    +       internal errors, so make sure we are still linked first.  */
    +    if (is_linked ())
    +      frame_list.erase (frame_list.iterator_to (*this));
       }
    ...
    
    However, since then frame_list has been moved into frame.c, and
    initialization/destruction order is guarantueed inside a compilation unit.
    
    Revert aforementioned commit, and fix the destruction order problem by moving
    frame_list before selected_frame.
    
    Reverting the commit is another way of fixing the already fixed
    Wdangling-pointer warning reported in PR build/30413, in a different way than
    commit 9b0ccb1ebae ("Pass const frame_info_ptr reference for
    skip_[language_]trampoline").
    
    Approved-By: Simon Marchi <simon.marchi@efficios.com>
    Tested on x86_64-linux.
    PR build/30413
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30413
Comment 13 Tom de Vries 2023-05-03 19:56:50 UTC
(In reply to Simon Marchi from comment #10)
> I don't think that intrusive_list is trivially constructible, since we
> assign the fields like this:
> 
>   T *m_front = nullptr;
>   T *m_back = nullptr;
> 
> I verified using std::is_trivially_constructible.  I suppose that things
> still work because the object is put in .bss, so the fields are cleared from
> the start.

Yes, I've now found the additional info I needed here ( https://en.cppreference.com/w/cpp/language/initialization#Non-local_variables ).

So what happens with frame_list is static initialization, which happens before dynamic initialization, and the ordering within a compilation unit applies to the dynamic initialization.

> Still, I'd put the declarations in the right order, just to be
> extra safe.

Sure, done as part of the by now committed cleanup patch.
Comment 14 Tom de Vries 2023-05-03 20:15:00 UTC
Fixed.