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.... ...
(In reply to Tom de Vries from comment #0) > The buildbot for opensuse tumbleweed fails Same for rawhide.
https://sourceware.org/pipermail/gdb-patches/2023-May/199291.html
(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.
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 ...
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) ...
Thanks Tom, this analysis makes sense to me.
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>
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....
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).
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.
(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
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
(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.
Fixed.