[PATCH][gdb/testsuite] Use prototype to call libc functions

[PATCH][gdb/testsuite] Use prototype to call libc functions

[Tom de Vries]

Hi,

On openSUSE Tumbleweed (using glibc 2.36), I run into:
...
(gdb) print /d (int) munmap (4198400, 4096)^M
Invalid cast.^M
(gdb) FAIL: gdb.base/break-main-file-remove-fail.exp: cmdline: \
  get integer valueof "(int) munmap (4198400, 4096)"
...

The problem is that after starting the executable, the symbol has type
"void (*) (void)":
...
(gdb) p munmap
$1 = {<text variable, no debug info>} 0x401030 <munmap@plt>
(gdb) start
  ...
(gdb) p munmap
$2 = {void (void)} 0x7ffff7feb9a0 <__GI_munmap>
...
which causes the "Invalid cast" error.

Looking at the debug info for glibc for symbol __GI_munmap:
...
 <0><189683>: Abbrev Number: 1 (DW_TAG_compile_unit)
    <189691>   DW_AT_name        : ../sysdeps/unix/syscall-template.S
    <189699>   DW_AT_producer    : GNU AS 2.39.0
<1><1896ae>: Abbrev Number: 2 (DW_TAG_subprogram)
    <1896af>   DW_AT_name        : __GI___munmap
    <1896b3>   DW_AT_external    : 1
    <1896b4>   DW_AT_low_pc      : 0x10cad0
    <1896bc>   DW_AT_high_pc     : 37
...
that's probably caused by this bit (or similar bits for other munmap aliases).

This is fixed in gas on trunk by commit 5578fbf672e ("GAS: Add a return type
tag to DWARF DIEs generated for function symbols").

Work around this (for say gas 2.39) by explicitly specifying the prototype for
munmap.

Likewise for getpid in a couple of other test-cases.

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb/testsuite] Use prototype to call libc functions

---
 gdb/testsuite/gdb.base/break-main-file-remove-fail.exp | 4 +++-
 gdb/testsuite/gdb.base/dprintf-detach.exp              | 2 +-
 gdb/testsuite/gdb.base/info-os.exp                     | 2 +-
 gdb/testsuite/gdb.threads/siginfo-threads.exp          | 2 +-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp
index 41c3114aa05..444b1d33ace 100644
--- a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp
+++ b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp
@@ -87,7 +87,9 @@ proc test_remove_bp { initial_load } {
 	# should warn the user about it.
 	set pagesize [get_integer_valueof "pg_size" 0]
 	set align_addr [expr $bp_addr - $bp_addr % $pagesize]
-	set munmap [get_integer_valueof "(int) munmap ($align_addr, $pagesize)" -1]
+	set munmap_prototype "int (*) (void *, size_t)"
+	set munmap_expr "(($munmap_prototype) munmap) ($align_addr, $pagesize)"
+	set munmap [get_integer_valueof $munmap_expr -1]
 
 	if {$munmap != 0} {
 	    unsupported "can't munmap foo's page"
diff --git a/gdb/testsuite/gdb.base/dprintf-detach.exp b/gdb/testsuite/gdb.base/dprintf-detach.exp
index 692048f82a3..b6da01a2845 100644
--- a/gdb/testsuite/gdb.base/dprintf-detach.exp
+++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
@@ -52,7 +52,7 @@ proc dprintf_detach_test { breakpoint_always_inserted dprintf_style disconnected
 	# Get PID of test program.
 	set inferior_pid -1
 	set test "get inferior process ID"
-	gdb_test_multiple "call (int) getpid ()" $test {
+	gdb_test_multiple "call ((int (*) (void)) getpid) ()" $test {
 	    -re ".* = ($decimal).*$gdb_prompt $" {
 		set inferior_pid $expect_out(1,string)
 		pass $test
diff --git a/gdb/testsuite/gdb.base/info-os.exp b/gdb/testsuite/gdb.base/info-os.exp
index 7967e2e43df..d12f8750a9c 100644
--- a/gdb/testsuite/gdb.base/info-os.exp
+++ b/gdb/testsuite/gdb.base/info-os.exp
@@ -39,7 +39,7 @@ if ![runto_main] then {
 # Get PID of test program.
 set inferior_pid ""
 set test "get inferior process ID"
-gdb_test_multiple "call (int) getpid()" $test {
+gdb_test_multiple "call ((int (*) (void)) getpid) ()" $test {
     -re ".* = ($decimal).*$gdb_prompt $" {
 	set inferior_pid $expect_out(1,string)
 	pass $test
diff --git a/gdb/testsuite/gdb.threads/siginfo-threads.exp b/gdb/testsuite/gdb.threads/siginfo-threads.exp
index f10c7fdcd25..8dd34b753a2 100644
--- a/gdb/testsuite/gdb.threads/siginfo-threads.exp
+++ b/gdb/testsuite/gdb.threads/siginfo-threads.exp
@@ -41,7 +41,7 @@ gdb_breakpoint [gdb_get_line_number "break-at-exit"]
 
 set test "get pid"
 set pid ""
-gdb_test_multiple "p (int) getpid ()" $test {
+gdb_test_multiple "p ((int (*) (void))getpid) ()" $test {
     -re " = (\[0-9\]+)\r\n$gdb_prompt $" {
 	set pid $expect_out(1,string)
 	pass $test

Re: [PATCH][gdb/testsuite] Use prototype to call libc functions

[Tom de Vries]

On 9/5/22 14:27, Tom de Vries via Gdb-patches wrote:
> Hi,
> 
> On openSUSE Tumbleweed (using glibc 2.36), I run into:
> ...
> (gdb) print /d (int) munmap (4198400, 4096)^M
> Invalid cast.^M
> (gdb) FAIL: gdb.base/break-main-file-remove-fail.exp: cmdline: \
>    get integer valueof "(int) munmap (4198400, 4096)"
> ...
> 
> The problem is that after starting the executable, the symbol has type
> "void (*) (void)":
> ...
> (gdb) p munmap
> $1 = {<text variable, no debug info>} 0x401030 <munmap@plt>
> (gdb) start
>    ...
> (gdb) p munmap
> $2 = {void (void)} 0x7ffff7feb9a0 <__GI_munmap>
> ...
> which causes the "Invalid cast" error.
> 
> Looking at the debug info for glibc for symbol __GI_munmap:
> ...
>   <0><189683>: Abbrev Number: 1 (DW_TAG_compile_unit)
>      <189691>   DW_AT_name        : ../sysdeps/unix/syscall-template.S
>      <189699>   DW_AT_producer    : GNU AS 2.39.0
> <1><1896ae>: Abbrev Number: 2 (DW_TAG_subprogram)
>      <1896af>   DW_AT_name        : __GI___munmap
>      <1896b3>   DW_AT_external    : 1
>      <1896b4>   DW_AT_low_pc      : 0x10cad0
>      <1896bc>   DW_AT_high_pc     : 37
> ...
> that's probably caused by this bit (or similar bits for other munmap aliases).
> 
> This is fixed in gas on trunk by commit 5578fbf672e ("GAS: Add a return type
> tag to DWARF DIEs generated for function symbols").
> 
> Work around this (for say gas 2.39) by explicitly specifying the prototype for
> munmap.
> 
> Likewise for getpid in a couple of other test-cases.
> 
> Tested on x86_64-linux.
> 
> Any comments?
> 

I've pushed this, given that it fixes gdb-rawhide-x86_64 buildbot.

Thanks,
- Tom

> [gdb/testsuite] Use prototype to call libc functions
> 
> ---
>   gdb/testsuite/gdb.base/break-main-file-remove-fail.exp | 4 +++-
>   gdb/testsuite/gdb.base/dprintf-detach.exp              | 2 +-
>   gdb/testsuite/gdb.base/info-os.exp                     | 2 +-
>   gdb/testsuite/gdb.threads/siginfo-threads.exp          | 2 +-
>   4 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp
> index 41c3114aa05..444b1d33ace 100644
> --- a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp
> +++ b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp
> @@ -87,7 +87,9 @@ proc test_remove_bp { initial_load } {
>   	# should warn the user about it.
>   	set pagesize [get_integer_valueof "pg_size" 0]
>   	set align_addr [expr $bp_addr - $bp_addr % $pagesize]
> -	set munmap [get_integer_valueof "(int) munmap ($align_addr, $pagesize)" -1]
> +	set munmap_prototype "int (*) (void *, size_t)"
> +	set munmap_expr "(($munmap_prototype) munmap) ($align_addr, $pagesize)"
> +	set munmap [get_integer_valueof $munmap_expr -1]
>   
>   	if {$munmap != 0} {
>   	    unsupported "can't munmap foo's page"
> diff --git a/gdb/testsuite/gdb.base/dprintf-detach.exp b/gdb/testsuite/gdb.base/dprintf-detach.exp
> index 692048f82a3..b6da01a2845 100644
> --- a/gdb/testsuite/gdb.base/dprintf-detach.exp
> +++ b/gdb/testsuite/gdb.base/dprintf-detach.exp
> @@ -52,7 +52,7 @@ proc dprintf_detach_test { breakpoint_always_inserted dprintf_style disconnected
>   	# Get PID of test program.
>   	set inferior_pid -1
>   	set test "get inferior process ID"
> -	gdb_test_multiple "call (int) getpid ()" $test {
> +	gdb_test_multiple "call ((int (*) (void)) getpid) ()" $test {
>   	    -re ".* = ($decimal).*$gdb_prompt $" {
>   		set inferior_pid $expect_out(1,string)
>   		pass $test
> diff --git a/gdb/testsuite/gdb.base/info-os.exp b/gdb/testsuite/gdb.base/info-os.exp
> index 7967e2e43df..d12f8750a9c 100644
> --- a/gdb/testsuite/gdb.base/info-os.exp
> +++ b/gdb/testsuite/gdb.base/info-os.exp
> @@ -39,7 +39,7 @@ if ![runto_main] then {
>   # Get PID of test program.
>   set inferior_pid ""
>   set test "get inferior process ID"
> -gdb_test_multiple "call (int) getpid()" $test {
> +gdb_test_multiple "call ((int (*) (void)) getpid) ()" $test {
>       -re ".* = ($decimal).*$gdb_prompt $" {
>   	set inferior_pid $expect_out(1,string)
>   	pass $test
> diff --git a/gdb/testsuite/gdb.threads/siginfo-threads.exp b/gdb/testsuite/gdb.threads/siginfo-threads.exp
> index f10c7fdcd25..8dd34b753a2 100644
> --- a/gdb/testsuite/gdb.threads/siginfo-threads.exp
> +++ b/gdb/testsuite/gdb.threads/siginfo-threads.exp
> @@ -41,7 +41,7 @@ gdb_breakpoint [gdb_get_line_number "break-at-exit"]
>   
>   set test "get pid"
>   set pid ""
> -gdb_test_multiple "p (int) getpid ()" $test {
> +gdb_test_multiple "p ((int (*) (void))getpid) ()" $test {
>       -re " = (\[0-9\]+)\r\n$gdb_prompt $" {
>   	set pid $expect_out(1,string)
>   	pass $test

Re: [PATCH][gdb/testsuite] Use prototype to call libc functions

[Kevin Buettner]

On Mon, 5 Sep 2022 14:27:07 +0200
Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> wrote:

> Hi,
> 
> On openSUSE Tumbleweed (using glibc 2.36), I run into:
> ...
> (gdb) print /d (int) munmap (4198400, 4096)^M
> Invalid cast.^M
> (gdb) FAIL: gdb.base/break-main-file-remove-fail.exp: cmdline: \
>   get integer valueof "(int) munmap (4198400, 4096)"
> ...
> 
> The problem is that after starting the executable, the symbol has type
> "void (*) (void)":
> ...
> (gdb) p munmap
> $1 = {<text variable, no debug info>} 0x401030 <munmap@plt>
> (gdb) start
>   ...
> (gdb) p munmap
> $2 = {void (void)} 0x7ffff7feb9a0 <__GI_munmap>
> ...
> which causes the "Invalid cast" error.
> 
> Looking at the debug info for glibc for symbol __GI_munmap:
> ...
>  <0><189683>: Abbrev Number: 1 (DW_TAG_compile_unit)
>     <189691>   DW_AT_name        : ../sysdeps/unix/syscall-template.S
>     <189699>   DW_AT_producer    : GNU AS 2.39.0
> <1><1896ae>: Abbrev Number: 2 (DW_TAG_subprogram)
>     <1896af>   DW_AT_name        : __GI___munmap
>     <1896b3>   DW_AT_external    : 1
>     <1896b4>   DW_AT_low_pc      : 0x10cad0
>     <1896bc>   DW_AT_high_pc     : 37
> ...
> that's probably caused by this bit (or similar bits for other munmap aliases).
> 
> This is fixed in gas on trunk by commit 5578fbf672e ("GAS: Add a return type
> tag to DWARF DIEs generated for function symbols").
> 
> Work around this (for say gas 2.39) by explicitly specifying the prototype for
> munmap.
> 
> Likewise for getpid in a couple of other test-cases.
> 
> Tested on x86_64-linux.
> 
> Any comments?

I have mixed feelings about patches like this.

One the one hand, it's nice to have the test "fixed" so that it doesn't
fail.

But, on the other hand, this failure found a problem with the debug info
in glibc, so fixing it as you did here will remove that test.

Perhaps you could add a new test which will still fail when the debug info
incorrectly specifies that munmap has a void return type?

Kevin

Re: [PATCH][gdb/testsuite] Use prototype to call libc functions

[Tom de Vries]

On 9/7/22 19:41, Kevin Buettner wrote:
> On Mon, 5 Sep 2022 14:27:07 +0200
> Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
>> Hi,
>>
>> On openSUSE Tumbleweed (using glibc 2.36), I run into:
>> ...
>> (gdb) print /d (int) munmap (4198400, 4096)^M
>> Invalid cast.^M
>> (gdb) FAIL: gdb.base/break-main-file-remove-fail.exp: cmdline: \
>>    get integer valueof "(int) munmap (4198400, 4096)"
>> ...
>>
>> The problem is that after starting the executable, the symbol has type
>> "void (*) (void)":
>> ...
>> (gdb) p munmap
>> $1 = {<text variable, no debug info>} 0x401030 <munmap@plt>
>> (gdb) start
>>    ...
>> (gdb) p munmap
>> $2 = {void (void)} 0x7ffff7feb9a0 <__GI_munmap>
>> ...
>> which causes the "Invalid cast" error.
>>
>> Looking at the debug info for glibc for symbol __GI_munmap:
>> ...
>>   <0><189683>: Abbrev Number: 1 (DW_TAG_compile_unit)
>>      <189691>   DW_AT_name        : ../sysdeps/unix/syscall-template.S
>>      <189699>   DW_AT_producer    : GNU AS 2.39.0
>> <1><1896ae>: Abbrev Number: 2 (DW_TAG_subprogram)
>>      <1896af>   DW_AT_name        : __GI___munmap
>>      <1896b3>   DW_AT_external    : 1
>>      <1896b4>   DW_AT_low_pc      : 0x10cad0
>>      <1896bc>   DW_AT_high_pc     : 37
>> ...
>> that's probably caused by this bit (or similar bits for other munmap aliases).
>>
>> This is fixed in gas on trunk by commit 5578fbf672e ("GAS: Add a return type
>> tag to DWARF DIEs generated for function symbols").
>>
>> Work around this (for say gas 2.39) by explicitly specifying the prototype for
>> munmap.
>>
>> Likewise for getpid in a couple of other test-cases.
>>
>> Tested on x86_64-linux.
>>
>> Any comments?
> 
> I have mixed feelings about patches like this.
> 
> One the one hand, it's nice to have the test "fixed" so that it doesn't
> fail.
> 
> But, on the other hand, this failure found a problem with the debug info
> in glibc, so fixing it as you did here will remove that test.
> 
> Perhaps you could add a new test which will still fail when the debug info
> incorrectly specifies that munmap has a void return type?

Hi Kevin,

thanks for the review.

Alternatively, I could have marked these xfails, because they represent 
a failure in the environment, not in gdb.  But I think that if we can 
avoid xfails by working around them, we should, because it simplifies 
testsuite maintenance.

The fact that we no longer use that libc debuginfo is fine from the 
point of view of the gdb testsuite being a regression test suite for gdb.

That does beg the question, where else would libc debuginfo be tested, 
and I don't know where, so perhaps we could have some dedicated gdb.libc 
testsuite subdir for that.  I would put the test you propose in there.

What is useful about the proposed test is that it eventually (once the 
new binutils release makes it into the various distros) will test the 
munmap debuginfo as generated by the fixed gas, that is, using 
DW_TAG_unspecified_type.

We don't seem to have a dedicated test for this tag.  There's some 
ada-specific bit in the code in gdb/dwarf/read.c handling this, which is 
supposed to be tested by gdb.ada/taft_type.exp, but how it will behave 
with the fixed gas, I have no idea.

So, can we already test this now?  Let's try: 
https://sourceware.org/bugzilla/show_bug.cgi?id=29558 .  AFAIU, the 
generated dwarf is incorrect, so I've filed a gas PR for that.

We can rewrite this into a gdb test-case, which would test gas debuginfo 
(so gdb.gas subdir?), but I guess we'd probably also want a dwarf 
assembly test-case.

Thanks,
- Tom

Re: [PATCH][gdb/testsuite] Use prototype to call libc functions

[Luis Machado]

On 9/7/22 18:41, Kevin Buettner via Gdb-patches wrote:
> On Mon, 5 Sep 2022 14:27:07 +0200
> Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
>> Hi,
>>
>> On openSUSE Tumbleweed (using glibc 2.36), I run into:
>> ...
>> (gdb) print /d (int) munmap (4198400, 4096)^M
>> Invalid cast.^M
>> (gdb) FAIL: gdb.base/break-main-file-remove-fail.exp: cmdline: \
>>    get integer valueof "(int) munmap (4198400, 4096)"
>> ...
>>
>> The problem is that after starting the executable, the symbol has type
>> "void (*) (void)":
>> ...
>> (gdb) p munmap
>> $1 = {<text variable, no debug info>} 0x401030 <munmap@plt>
>> (gdb) start
>>    ...
>> (gdb) p munmap
>> $2 = {void (void)} 0x7ffff7feb9a0 <__GI_munmap>
>> ...
>> which causes the "Invalid cast" error.
>>
>> Looking at the debug info for glibc for symbol __GI_munmap:
>> ...
>>   <0><189683>: Abbrev Number: 1 (DW_TAG_compile_unit)
>>      <189691>   DW_AT_name        : ../sysdeps/unix/syscall-template.S
>>      <189699>   DW_AT_producer    : GNU AS 2.39.0
>> <1><1896ae>: Abbrev Number: 2 (DW_TAG_subprogram)
>>      <1896af>   DW_AT_name        : __GI___munmap
>>      <1896b3>   DW_AT_external    : 1
>>      <1896b4>   DW_AT_low_pc      : 0x10cad0
>>      <1896bc>   DW_AT_high_pc     : 37
>> ...
>> that's probably caused by this bit (or similar bits for other munmap aliases).
>>
>> This is fixed in gas on trunk by commit 5578fbf672e ("GAS: Add a return type
>> tag to DWARF DIEs generated for function symbols").
>>
>> Work around this (for say gas 2.39) by explicitly specifying the prototype for
>> munmap.
>>
>> Likewise for getpid in a couple of other test-cases.
>>
>> Tested on x86_64-linux.
>>
>> Any comments?
> 
> I have mixed feelings about patches like this.
> 
> One the one hand, it's nice to have the test "fixed" so that it doesn't
> fail.
> 
> But, on the other hand, this failure found a problem with the debug info
> in glibc, so fixing it as you did here will remove that test.
> 
> Perhaps you could add a new test which will still fail when the debug info
> incorrectly specifies that munmap has a void return type?
> 
> Kevin
> 

My $0.02 is that the GDB testsuite is already very often affected by bugs in other components (compiler, debug info, linker, gas etc), which make it
even harder to have a somewhat clean GDB testsuite run.

I'd say it is the responsibility of whatever component contains the bug to have a test for it and fix it. In this case, gas.

The XFAIL idea is also OK in my opinion.