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

[Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>]

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