Bug 31693 - [gdb/exp] cast not handled correctly by indirection
Summary: [gdb/exp] cast not handled correctly by indirection
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: exp (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: 15.1
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-05-02 12:55 UTC by Tom de Vries
Modified: 2024-05-04 04:22 UTC (History)
2 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 2024-05-02 12:55:50 UTC
I was reviewing this patch ( https://sourceware.org/pipermail/gdb-patches/2024-May/208737.html ) and came across this description:
...
+	    # On Linux, using -g3, which causes macro information to
+	    # be included in the debuginfo, errno might be defined as
+	    # follows:
+	    #
+	    #   #define errno (*__errno_location ())
+	    #
+	    # So, when we do "ptype errno", due to macro expansion,
+	    # this ends up being "ptype (*__errno_location ())".  So
+	    # the call to __errno_location (or something similar on
+	    # other OSes) is the call mentioned in the error message.
+	    #
+	    # For the test "print (int) errno", we're casting the
+	    # result of the expression, which includes both the call
+	    # along with a dereferencing operation.
+	    #
+	    # This will sometimes produce the right answer, but it's
+	    # also just as likely to fail.  E.g.  on x86_64, if the
+	    # address being returned as a 32-bit int is the same as
+	    # that which would have been returned as a 64-bit pointer,
+	    # then the test might pass.  Otherwise, it will almost
+	    # certainly fail, which is why we XFAIL it here.  But do
+	    # expect to see the occasional XPASS for this case.
...

I tried to reproduce this (in a fedora rawhide container with the check-errno-macros exec) and got:
...
(gdb) p errno
'__errno_location' has unknown return type; cast the call to its declared return type
(gdb) ptype errno
'__errno_location' has unknown return type; cast the call to its declared return type
(gdb) p __errno_location
$9 = {<text variable, no debug info>} 0x7ffff7d10540 <__errno_location>
(gdb) ptype __errno_location
type = <unknown return type> ()
(gdb) p __errno_location ()
'__errno_location' has unknown return type; cast the call to its declared return type
(gdb) p *__errno_location ()
'__errno_location' has unknown return type; cast the call to its declared return type
(gdb) p (*__errno_location ())
'__errno_location' has unknown return type; cast the call to its declared return type
(gdb) p (int)(*__errno_location ())
Cannot access memory at address 0xfffffffff7ce36c8
(gdb) p (int)(*(int *)__errno_location ())
$10 = 42
(gdb) p /x &(int)*__errno_location ()
$11 = 0xfffffffff7ce36c8
(gdb) p /x &(int)*(int *)__errno_location ()
$12 = 0x7ffff7ce36c8
(gdb) p /x &(int)*(int)__errno_location ()
$3 = 0xfffffffff7ce36c8
...

We known that __errno_location has an unknown return type.  We ask for it to be cast before using it.  But then when using it nested in a expression we cast it to int.

Probably there's a bug in this code expop.h at unop_ind_base_operation:
...
  value *evaluate (struct type *expect_type,
                   struct expression *exp,
                   enum noside noside) override
  {
    if (expect_type != nullptr && expect_type->code () == TYPE_CODE_PTR)
      expect_type = check_typedef (expect_type)->target_type ();
    value *val = std::get<0> (m_storage)->evaluate (expect_type, exp, noside);
    return eval_op_ind (expect_type, exp, noside, val);
  }
...

We enter with expect_type "int", we should we execute the call with expect_type "int*", but instead we do so with "int".
Comment 1 Tom de Vries 2024-05-02 13:35:16 UTC
This seems to fix it:
...
diff --git a/gdb/expop.h b/gdb/expop.h
index b81e228c07e..043142f2f53 100644
--- a/gdb/expop.h
+++ b/gdb/expop.h
@@ -1513,9 +1513,10 @@ class unop_ind_base_operation
 		   struct expression *exp,
 		   enum noside noside) override
   {
-    if (expect_type != nullptr && expect_type->code () == TYPE_CODE_PTR)
-      expect_type = check_typedef (expect_type)->target_type ();
-    value *val = std::get<0> (m_storage)->evaluate (expect_type, exp, noside);
+    struct type *expect_type_2 = expect_type;
+    if (expect_type != nullptr)
+      expect_type_2 = lookup_pointer_type (expect_type);
+    value *val = std::get<0> (m_storage)->evaluate (expect_type_2, exp, noside);
     return eval_op_ind (expect_type, exp, noside, val);
   }
 
...
Comment 2 Tom de Vries 2024-05-02 14:32:56 UTC
To take it out of errno context, a simple reproducer.

Test-case:
...
$ cat test.c
char a = 'a';

char *
a_loc (void)
{
  return &a;
}

int
main (void)
{
  return 0;
}
...

Compile, no debug info:
...
$ gcc test.c
...

System gdb (13.2), not fixed:
...
$ gdb -q -batch a.out -ex start -ex "p (char)*a_loc ()"
Temporary breakpoint 1 at 0x4004b6

Temporary breakpoint 1, 0x00000000004004b6 in main ()
Cannot access memory at address 0x10
...

Fixed gdb (trunk):
...
$ gdb -q -batch a.out -ex start -ex "p (char)*a_loc ()"
Temporary breakpoint 1 at 0x4004b6

Temporary breakpoint 1, 0x00000000004004b6 in main ()
$1 = 97 'a'
....
Comment 3 Kevin Buettner 2024-05-02 15:34:44 UTC
The proposed fix in Comment 1 looks reasonable to me.
Comment 5 Tom Tromey 2024-05-02 15:49:17 UTC
The patch is probably fine, but the intent of the message
(IMO) is to say that the call itself should have the
cast, like:

print *(char*)a_loc()
Comment 6 Tom de Vries 2024-05-02 15:54:14 UTC
(In reply to Tom Tromey from comment #5)
> The patch is probably fine, but the intent of the message
> (IMO) is to say that the call itself should have the
> cast, like:
> 
> print *(char*)a_loc()

Agreed.  This is more about making errno work out of the box, when errno is a macro defined as *(__errno_location ()) and there's no debug info for __errno_location.
Comment 7 Tom de Vries 2024-05-02 15:57:11 UTC
(In reply to Tom de Vries from comment #6)
> (In reply to Tom Tromey from comment #5)
> > The patch is probably fine, but the intent of the message
> > (IMO) is to say that the call itself should have the
> > cast, like:
> > 
> > print *(char*)a_loc()
> 
> Agreed.  This is more about making errno work out of the box

Well, I mean (int)errno.
Comment 8 Sourceware Commits 2024-05-03 07:37:00 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=ed8fd0a342f6e832fee1a3fabc3e494977780dcf

commit ed8fd0a342f6e832fee1a3fabc3e494977780dcf
Author: Tom de Vries <tdevries@suse.de>
Date:   Fri May 3 09:37:19 2024 +0200

    [gdb/exp] Fix cast handling for indirection
    
    Consider a test-case compiled without debug info, containing:
    ...
    char a = 'a';
    
    char *
    a_loc (void)
    {
      return &a;
    }
    ...
    
    We get:
    ...
    (gdb) p (char)*a_loc ()
    Cannot access memory at address 0x10
    ...
    
    There's a bug in unop_ind_base_operation::evaluate that evaluates
    "(char)*a_loc ()" the same as:
    ...
    (gdb) p (char)*(char)a_loc ()
    Cannot access memory at address 0x10
    ...
    
    Fix this by instead evaluating it the same as:
    ...
    (gdb) p (char)*(char *)a_loc ()
    $1 = 97 'a'
    ...
    
    Tested on x86_64-linux.
    
    PR exp/31693
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31693
Comment 9 Tom de Vries 2024-05-03 07:38:12 UTC
Fixed.