[PATCHv3,4/4] gdb: fix invalid arg coercion when calling static member functions

Message ID cdcdf467df69bfc7205d220b9072e2855c30d0bf.1624486326.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • gdb: fix regression in evaluate_funcall for non C++ like cases
Related show

Commit Message

Andrew Burgess June 23, 2021, 10:44 p.m.
In this commit:

  commit 7022349d5c86bae74b49225515f42d2e221bd368
  Date:   Mon Sep 4 20:21:13 2017 +0100

      Stop assuming no-debug-info functions return int

A new if case was added to call_function_by_hand_dummy to decide if a
function should be considered prototyped or not.  Previously the code
was structured like this:

  if (COND_1)
    ACTION_1
  else if (COND_2)
    ACTION_2
  else
    ACTION_3

With the new block the code now looks like this:

  if (COND_1)
    ACTION_1
  if (NEW_COND)
    NEW_ACTION
  else if (COND_2)
    ACTION_2
  else
    ACTION_3

Notice the new block was added as and 'if' not 'else if'.  I'm running
into a case where GDB executes ACTION_1 and then ACTION_2.  Prior to
the above commit GDB would only have executed ACTION_1.

The actions in the code in question are trying to figure out if a
function should be considered prototyped or not.  When a function is
not prototyped some arguments will be coerced, e.g. floats to doubles.

The COND_1 / ACTION_1 are a very broad, any member function should be
considered prototyped, however, after the above patch GDB is now
executing the later ACTION_2 which checks to see if the function's
type has the 'prototyped' flag set - this is not the case for the
member functions I'm testing, and so GDB treats the function as
unprototyped and casts the float argument to a double.

I believe that adding the new check as 'if' rather than 'else if' was
a mistake, and so in this commit I add in the missing 'else'.

gdb/ChangeLog:

	* infcall.c (call_function_by_hand_dummy): Add missing 'else' when
	setting prototyped flag.

gdb/testsuite/ChangeLog:

	* gdb.cp/method-call-in-c.cc (struct foo_type): Add static member
	function static_method.
	(global_var): New global.
	(main): Use new static_method to ensure it is compiled in.
	* gdb.cp/method-call-in-c.exp: Test calls to static member
	function.
---
 gdb/ChangeLog                             | 5 +++++
 gdb/infcall.c                             | 4 ++--
 gdb/testsuite/ChangeLog                   | 9 +++++++++
 gdb/testsuite/gdb.cp/method-call-in-c.cc  | 9 +++++++++
 gdb/testsuite/gdb.cp/method-call-in-c.exp | 3 +++
 5 files changed, 28 insertions(+), 2 deletions(-)

-- 
2.25.4

Comments

Eli Zaretskii via Gdb-patches June 25, 2021, 2:53 p.m. | #1
On 2021-06-23 6:44 p.m., Andrew Burgess wrote:
> In this commit:

> 

>   commit 7022349d5c86bae74b49225515f42d2e221bd368

>   Date:   Mon Sep 4 20:21:13 2017 +0100

> 

>       Stop assuming no-debug-info functions return int

> 

> A new if case was added to call_function_by_hand_dummy to decide if a

> function should be considered prototyped or not.  Previously the code

> was structured like this:

> 

>   if (COND_1)

>     ACTION_1

>   else if (COND_2)

>     ACTION_2

>   else

>     ACTION_3

> 

> With the new block the code now looks like this:

> 

>   if (COND_1)

>     ACTION_1

>   if (NEW_COND)

>     NEW_ACTION

>   else if (COND_2)

>     ACTION_2

>   else

>     ACTION_3

> 

> Notice the new block was added as and 'if' not 'else if'.  I'm running

> into a case where GDB executes ACTION_1 and then ACTION_2.  Prior to

> the above commit GDB would only have executed ACTION_1.

> 

> The actions in the code in question are trying to figure out if a

> function should be considered prototyped or not.  When a function is

> not prototyped some arguments will be coerced, e.g. floats to doubles.

> 

> The COND_1 / ACTION_1 are a very broad, any member function should be

> considered prototyped, however, after the above patch GDB is now

> executing the later ACTION_2 which checks to see if the function's

> type has the 'prototyped' flag set - this is not the case for the

> member functions I'm testing, and so GDB treats the function as

> unprototyped and casts the float argument to a double.

> 

> I believe that adding the new check as 'if' rather than 'else if' was

> a mistake, and so in this commit I add in the missing 'else'.


Wow, good investigation work, thanks for fixing this.  It all LGTM.

Simon
Andrew Burgess June 25, 2021, 7:53 p.m. | #2
* Simon Marchi <simon.marchi@polymtl.ca> [2021-06-25 10:53:16 -0400]:

> On 2021-06-23 6:44 p.m., Andrew Burgess wrote:

> > In this commit:

> > 

> >   commit 7022349d5c86bae74b49225515f42d2e221bd368

> >   Date:   Mon Sep 4 20:21:13 2017 +0100

> > 

> >       Stop assuming no-debug-info functions return int

> > 

> > A new if case was added to call_function_by_hand_dummy to decide if a

> > function should be considered prototyped or not.  Previously the code

> > was structured like this:

> > 

> >   if (COND_1)

> >     ACTION_1

> >   else if (COND_2)

> >     ACTION_2

> >   else

> >     ACTION_3

> > 

> > With the new block the code now looks like this:

> > 

> >   if (COND_1)

> >     ACTION_1

> >   if (NEW_COND)

> >     NEW_ACTION

> >   else if (COND_2)

> >     ACTION_2

> >   else

> >     ACTION_3

> > 

> > Notice the new block was added as and 'if' not 'else if'.  I'm running

> > into a case where GDB executes ACTION_1 and then ACTION_2.  Prior to

> > the above commit GDB would only have executed ACTION_1.

> > 

> > The actions in the code in question are trying to figure out if a

> > function should be considered prototyped or not.  When a function is

> > not prototyped some arguments will be coerced, e.g. floats to doubles.

> > 

> > The COND_1 / ACTION_1 are a very broad, any member function should be

> > considered prototyped, however, after the above patch GDB is now

> > executing the later ACTION_2 which checks to see if the function's

> > type has the 'prototyped' flag set - this is not the case for the

> > member functions I'm testing, and so GDB treats the function as

> > unprototyped and casts the float argument to a double.

> > 

> > I believe that adding the new check as 'if' rather than 'else if' was

> > a mistake, and so in this commit I add in the missing 'else'.

> 

> Wow, good investigation work, thanks for fixing this.  It all LGTM.


I've pushed this series.

Thanks for all the feedback,

Andrew

Patch

diff --git a/gdb/infcall.c b/gdb/infcall.c
index ca3347fbb9d..40298fb1318 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -1026,8 +1026,8 @@  call_function_by_hand_dummy (struct value *function,
 	 prototyped.  Can we respect TYPE_VARARGS?  Probably not.  */
       if (ftype->code () == TYPE_CODE_METHOD)
 	prototyped = 1;
-      if (TYPE_TARGET_TYPE (ftype) == NULL && ftype->num_fields () == 0
-	  && default_return_type != NULL)
+      else if (TYPE_TARGET_TYPE (ftype) == NULL && ftype->num_fields () == 0
+	       && default_return_type != NULL)
 	{
 	  /* Calling a no-debug function with the return type
 	     explicitly cast.  Assume the function is prototyped,
diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.cc b/gdb/testsuite/gdb.cp/method-call-in-c.cc
index 95f3f3c22de..846a9111ef9 100644
--- a/gdb/testsuite/gdb.cp/method-call-in-c.cc
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.cc
@@ -35,9 +35,16 @@  struct foo_type
     return *this;
   }
 
+  static int static_method (float f, baz_type b)
+  {
+    return b.a + b.b + b.c + (int) f;
+  }
+
   int var = 120;
 };
 
+volatile int global_var;
+
 int
 main (void)
 {
@@ -48,5 +55,7 @@  main (void)
 
   foo += b;
 
+  global_var = foo.static_method (f, b);
+
   return foo.func (b, f);	/* Break here.  */
 }
diff --git a/gdb/testsuite/gdb.cp/method-call-in-c.exp b/gdb/testsuite/gdb.cp/method-call-in-c.exp
index 411ba6790b1..5debc0e9a7a 100644
--- a/gdb/testsuite/gdb.cp/method-call-in-c.exp
+++ b/gdb/testsuite/gdb.cp/method-call-in-c.exp
@@ -43,5 +43,8 @@  foreach_with_prefix lang { c++ c } {
 	set result [expr $result + 3]
 	gdb_test "print foo += b" \
 	    " = \\((?:struct )?foo_type &\\) @${hex}: \\\{var = ${result}\\\}"
+
+	gdb_test "print foo.static_method (f, b)" " = 4"
+	gdb_test "print foo_type::static_method (f, b)" " = 4"
     }
 }