Pass const frame_info_ptr reference for skip_[language_]trampoline
Commit Message
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 inlined enough code in this specific case).
Since passing the frame_info_ptr as const reference is cheaper, use
that as workaround for this warning.
---
gdb/c-lang.c | 2 +-
gdb/language.c | 2 +-
gdb/language.h | 4 ++--
gdb/objc-lang.c | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
Comments
Hi Mark,
On Tue, 2 May 2023 20:34:44 +0200
Mark Wielaard <mark@klomp.org> wrote:
> 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 inlined enough code in this specific case).
>
> Since passing the frame_info_ptr as const reference is cheaper, use
> that as workaround for this warning.
I've tested your patch on Fedora 38 w/ gcc (GCC) 13.1.1 20230426 (Red
Hat 13.1.1-1) and find that GDB builds again. Currently, it doesn't
build without your patch.
Your explanation and fix looks reasonable to me too.
Tested-by: Kevin Buettner <kevinb@redhat.com>
Reviewed-by: Kevin Buettner <kevinb@redhat.com>
>>>>> "Mark" == Mark Wielaard <mark@klomp.org> writes:
Mark> It isn't clear why only in this one place g++ sees the issue (probably
Mark> because it can inlined enough code in this specific case).
s/inlined/inline/
Could you add a "Bug:" trailer with a link to the bugzilla entry?
Anyway, other than that nit, this looks ok to me. It's a little
unfortunate we don't know why exactly this particular spot warns, but on
the other hand, the fix is harmless (actually an improvement) and we've
worked around plenty of compiler oddities before.
Reviewed-By: Tom Tromey <tom@tromey.com>
Tom
On Wed, May 03, 2023 at 08:50:07AM -0600, Tom Tromey wrote:
> >>>>> "Mark" == Mark Wielaard <mark@klomp.org> writes:
>
> Mark> It isn't clear why only in this one place g++ sees the issue (probably
> Mark> because it can inlined enough code in this specific case).
>
> s/inlined/inline/
>
> Could you add a "Bug:" trailer with a link to the bugzilla entry?
Fixed typo and added Bug link.
PR build/30413
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30413
> Anyway, other than that nit, this looks ok to me. It's a little
> unfortunate we don't know why exactly this particular spot warns, but on
> the other hand, the fix is harmless (actually an improvement) and we've
> worked around plenty of compiler oddities before.
>
> Reviewed-By: Tom Tromey <tom@tromey.com>
Thanks, pushed with that added. So that the code compiles again.
In the bug Tom de Vries has some ideas to "fix" this more generically
by changing the frame_info_ptr destructor to give the compiler even
more visibility into what is happening, which might help prevent
similar issues in the future if you don't want to change a pass by
value by a pass by reference.
Cheers,
Mark
On 5/3/23 17:11, Mark Wielaard wrote:
> On Wed, May 03, 2023 at 08:50:07AM -0600, Tom Tromey wrote:
>>>>>>> "Mark" == Mark Wielaard <mark@klomp.org> writes:
>>
>> Mark> It isn't clear why only in this one place g++ sees the issue (probably
>> Mark> because it can inlined enough code in this specific case).
>>
>> s/inlined/inline/
>>
>> Could you add a "Bug:" trailer with a link to the bugzilla entry?
>
> Fixed typo and added Bug link.
>
> PR build/30413
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30413
>
>> Anyway, other than that nit, this looks ok to me. It's a little
>> unfortunate we don't know why exactly this particular spot warns, but on
>> the other hand, the fix is harmless (actually an improvement) and we've
>> worked around plenty of compiler oddities before.
>>
>> Reviewed-By: Tom Tromey <tom@tromey.com>
>
> Thanks, pushed with that added. So that the code compiles again.
>
> In the bug Tom de Vries has some ideas to "fix" this more generically
> by changing the frame_info_ptr destructor to give the compiler even
> more visibility into what is happening, which might help prevent
> similar issues in the future if you don't want to change a pass by
> value by a pass by reference.
>
Hi Mark,
thanks for fixing this.
I've submitted a patch at
https://sourceware.org/pipermail/gdb-patches/2023-May/199342.html .
Indeed it's not a "fix" for the warning as such.
It's more a follow-up on the observation that the code that triggers the
warning was introduced for a reason that's no longer valid, so we can
just remove it and simplify the code, which then also "fixes" the warning.
Thanks,
- Tom
Hi Tom,
On Wed, May 03, 2023 at 08:07:50PM +0200, Tom de Vries wrote:
> I've submitted a patch at
> https://sourceware.org/pipermail/gdb-patches/2023-May/199342.html .
Thanks for figuring out why the compiler couldn't see through the
destructor. I must admit I didn't really understood that part.
> Indeed it's not a "fix" for the warning as such.
>
> It's more a follow-up on the observation that the code that triggers
> the warning was introduced for a reason that's no longer valid, so
> we can just remove it and simplify the code, which then also "fixes"
> the warning.
It is an interesting warning because it caused a bit of code
cleanup. But it seems to warn about something, the correct lifetime of
an object, that the compiler doesn't seem able to proof consistently.
Hopefully your simplification of the destructor will make the compile
able to "proof" the correct lifetime and fix the remaining sparc
issue.
Cheers,
Mark
@@ -1002,7 +1002,7 @@ class cplus_language : public language_defn
/* See language.h. */
- CORE_ADDR skip_trampoline (frame_info_ptr fi,
+ CORE_ADDR skip_trampoline (const frame_info_ptr &fi,
CORE_ADDR pc) const override
{
return cplus_skip_trampoline (fi, pc);
@@ -523,7 +523,7 @@ add_set_language_command ()
Return the result from the first that returns non-zero, or 0 if all
`fail'. */
CORE_ADDR
-skip_language_trampoline (frame_info_ptr frame, CORE_ADDR pc)
+skip_language_trampoline (const frame_info_ptr &frame, CORE_ADDR pc)
{
for (const auto &lang : language_defn::languages)
{
@@ -471,7 +471,7 @@ struct language_defn
If that PC falls in a trampoline belonging to this language, return
the address of the first pc in the real function, or 0 if it isn't a
language tramp for this language. */
- virtual CORE_ADDR skip_trampoline (frame_info_ptr fi, CORE_ADDR pc) const
+ virtual CORE_ADDR skip_trampoline (const frame_info_ptr &fi, CORE_ADDR pc) const
{
return (CORE_ADDR) 0;
}
@@ -790,7 +790,7 @@ extern const char *language_str (enum language);
/* Check for a language-specific trampoline. */
-extern CORE_ADDR skip_language_trampoline (frame_info_ptr, CORE_ADDR pc);
+extern CORE_ADDR skip_language_trampoline (const frame_info_ptr &, CORE_ADDR pc);
/* Return information about whether TYPE should be passed
(and returned) by reference at the language level. */
@@ -282,7 +282,7 @@ class objc_language : public language_defn
/* See language.h. */
- CORE_ADDR skip_trampoline (frame_info_ptr frame,
+ CORE_ADDR skip_trampoline (const frame_info_ptr &frame,
CORE_ADDR stop_pc) const override
{
struct gdbarch *gdbarch = get_frame_arch (frame);