diff mbox series

Pass const frame_info_ptr reference for skip_[language_]trampoline

Message ID 20230502183444.1445634-1-mark@klomp.org
State New
Headers
Series Pass const frame_info_ptr reference for skip_[language_]trampoline |

Commit Message

Mark Wielaard May 2, 2023, 6:34 p.m. UTC 
  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

Kevin Buettner May 3, 2023, 12:33 a.m. UTC | #1 
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>
Tom Tromey May 3, 2023, 2:50 p.m. UTC | #2 
>>>>> "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
Mark Wielaard May 3, 2023, 3:11 p.m. UTC | #3 
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
Tom de Vries May 3, 2023, 6:07 p.m. UTC | #4 
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
Mark Wielaard May 3, 2023, 7:18 p.m. UTC | #5 
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
diff mbox series

Patch

diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index 6535ab93498..484f81e455b 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -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);
diff --git a/gdb/language.c b/gdb/language.c
index 0b4202ff839..f82c5b173b3 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -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)
     {
diff --git a/gdb/language.h b/gdb/language.h
index 4c91776d94d..0ebd4c1d957 100644
--- a/gdb/language.h
+++ b/gdb/language.h
@@ -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.  */
diff --git a/gdb/objc-lang.c b/gdb/objc-lang.c
index ccbe7c19729..6af25d9c988 100644
--- a/gdb/objc-lang.c
+++ b/gdb/objc-lang.c
@@ -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);