Patchwork Oh dear. I regret to inform you that commit de52b9607d2623f18b7a7dbee3e1123d8d63f5da might be unfortunate

login
register
mail settings
Submitter Sergio Durigan Junior
Date June 29, 2018, 11:22 p.m.
Message ID <87vaa15gbn.fsf@redhat.com>
Download mbox | patch
Permalink /patch/28167/
State New
Headers show

Comments

Sergio Durigan Junior - June 29, 2018, 11:22 p.m.
On Friday, June 29 2018, Joel Brobecker wrote:

>> That must be me, so I will take a look now. I think it's because
>> I didn't build with --enable-targets=all.
>
> Attached is the patch I just pushed to master.
>
> gdb/ChangeLog:
>
>         * amd64-darwin-tdep.c (x86_darwin_init_abi_64): Add missing
>         parameter in call to amd64_target_description.
>         * amd64-dicos-tdep.c (amd64_dicos_init_abi): Likewise.
>         * amd64-fbsd-tdep.c (amd64fbsd_core_read_description)
>         (amd64fbsd_init_abi): Likewise.
>         * amd64-nbsd-tdep.c (amd64nbsd_init_abi): Likewise.
>         * amd64-obsd-tdep.c (amd64obsd_init_abi): Likewise.
>         * amd64-sol2-tdep.c (amd64_sol2_init_abi): Likewise.
>         * amd64-fbsd-nat.c (amd64_fbsd_nat_target): Likewise.
>
> The change to amd64-fbsd-nat.c was done "blind" (no access to system),
> but is reasonably straightforward. The changes to the -tdep.c files
> were verify by rebuilding GDB on x86_64-linux when configured with
> --enable-targets=all.

Hey Joel,

I'm still seeing a breakage here on x86-64 Fedora GNU/Linux, when
building gdbserver:

  ../../../binutils-gdb/gdb/gdbserver/linux-x86-tdesc.c: In function ‘const target_desc* amd64_linux_read_description(uint64_t, bool)’:
  ../../../binutils-gdb/gdb/gdbserver/linux-x86-tdesc.c:121:67: error: too few arguments to function ‘target_desc* amd64_create_target_description(uint64_t, bool, bool, bool)’
         *tdesc = amd64_create_target_description (xcr0, is_x32, true);
                                                                     ^
  In file included from ../../../binutils-gdb/gdb/gdbserver/linux-x86-tdesc.c:26:0:
  ../../../binutils-gdb/gdb/gdbserver/../arch/amd64.h:21:14: note: declared here
   target_desc *amd64_create_target_description (uint64_t xcr0, bool is_x32,
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Does the patch below make sense?  It fixes the breakage for me.

Thanks,
Joel Brobecker - June 29, 2018, 11:35 p.m.
> I'm still seeing a breakage here on x86-64 Fedora GNU/Linux, when
> building gdbserver:
> 
>   ../../../binutils-gdb/gdb/gdbserver/linux-x86-tdesc.c: In function ‘const target_desc* amd64_linux_read_description(uint64_t, bool)’:
>   ../../../binutils-gdb/gdb/gdbserver/linux-x86-tdesc.c:121:67: error: too few arguments to function ‘target_desc* amd64_create_target_description(uint64_t, bool, bool, bool)’
>          *tdesc = amd64_create_target_description (xcr0, is_x32, true);
>                                                                      ^
>   In file included from ../../../binutils-gdb/gdb/gdbserver/linux-x86-tdesc.c:26:0:
>   ../../../binutils-gdb/gdb/gdbserver/../arch/amd64.h:21:14: note: declared here
>    target_desc *amd64_create_target_description (uint64_t xcr0, bool is_x32,
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Does the patch below make sense?  It fixes the breakage for me.

Thanks for looking into it and the patch!

I think the parameter should be set to "true". Otherwise, it will
not include the fs_base and gs_base register in the list of registers.
Although the name of the source file says x86, the code itself is
protected by...

    #ifdef __x86_64__

... and is inside a function called amd64_linux_read_description.
I also verified that this file gets compiled on amd64-linux platforms.
See gdb/gdbserver/configure.srv:

  x86_64-*-linux*)      srv_regobj="$srv_amd64_linux_regobj $srv_i386_linux_regobj"

The last piece of confirmation is that setting the parameter to "true"
provides the behavior before the parameter was added; and the reason
for adding the parameter was to remove the {fs,gs}_base registers
from the list for Windows only.

Otherwise the rest (basically the ChangeLog) looks good.

Thanks again!

> diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
> index 2fd2d90a49..743d465d81 100644
> --- a/gdb/gdbserver/ChangeLog
> +++ b/gdb/gdbserver/ChangeLog
> @@ -1,3 +1,8 @@
> +2018-06-29  Sergio Durigan Junior  <sergiodj@redhat.com>
> +
> +	* linux-x86-tdesc.c (amd64_linux_read_description): Add missing
> +	parameter in call to 'amd64_create_target_description'.
> +
>  2018-06-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
>  
>  	* x86-tdesc.h: Remove executable permission flag.
> diff --git a/gdb/gdbserver/linux-x86-tdesc.c b/gdb/gdbserver/linux-x86-tdesc.c
> index 358659b295..7a45953dc9 100644
> --- a/gdb/gdbserver/linux-x86-tdesc.c
> +++ b/gdb/gdbserver/linux-x86-tdesc.c
> @@ -118,7 +118,7 @@ amd64_linux_read_description (uint64_t xcr0, bool is_x32)
>  
>    if (*tdesc == NULL)
>      {
> -      *tdesc = amd64_create_target_description (xcr0, is_x32, true);
> +      *tdesc = amd64_create_target_description (xcr0, is_x32, true, false);
>  
>        init_target_desc (*tdesc, amd64_expedite_regs);
>      }
Sergio Durigan Junior - June 30, 2018, 1:39 a.m.
On Friday, June 29 2018, Joel Brobecker wrote:

>> I'm still seeing a breakage here on x86-64 Fedora GNU/Linux, when
>> building gdbserver:
>> 
>>   ../../../binutils-gdb/gdb/gdbserver/linux-x86-tdesc.c: In function ‘const target_desc* amd64_linux_read_description(uint64_t, bool)’:
>>   ../../../binutils-gdb/gdb/gdbserver/linux-x86-tdesc.c:121:67: error: too few arguments to function ‘target_desc* amd64_create_target_description(uint64_t, bool, bool, bool)’
>>          *tdesc = amd64_create_target_description (xcr0, is_x32, true);
>>                                                                      ^
>>   In file included from ../../../binutils-gdb/gdb/gdbserver/linux-x86-tdesc.c:26:0:
>>   ../../../binutils-gdb/gdb/gdbserver/../arch/amd64.h:21:14: note: declared here
>>    target_desc *amd64_create_target_description (uint64_t xcr0, bool is_x32,
>>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 
>> Does the patch below make sense?  It fixes the breakage for me.
>
> Thanks for looking into it and the patch!
>
> I think the parameter should be set to "true". Otherwise, it will
> not include the fs_base and gs_base register in the list of registers.
> Although the name of the source file says x86, the code itself is
> protected by...
>
>     #ifdef __x86_64__
>
> ... and is inside a function called amd64_linux_read_description.
> I also verified that this file gets compiled on amd64-linux platforms.
> See gdb/gdbserver/configure.srv:
>
>   x86_64-*-linux*)      srv_regobj="$srv_amd64_linux_regobj $srv_i386_linux_regobj"
>
> The last piece of confirmation is that setting the parameter to "true"
> provides the behavior before the parameter was added; and the reason
> for adding the parameter was to remove the {fs,gs}_base registers
> from the list for Windows only.
>
> Otherwise the rest (basically the ChangeLog) looks good.

Ah, thanks for the explanation.  I should have investigated more before
sending the patch; I was doing something else and just wanted to make
GDB compile again.  Glad I sent it here for your review!

> Thanks again!

No, I thank you!

I pushed the patch with s/false/true/, and your name in the ChangeLog.

>> diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
>> index 2fd2d90a49..743d465d81 100644
>> --- a/gdb/gdbserver/ChangeLog
>> +++ b/gdb/gdbserver/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2018-06-29  Sergio Durigan Junior  <sergiodj@redhat.com>
>> +
>> +	* linux-x86-tdesc.c (amd64_linux_read_description): Add missing
>> +	parameter in call to 'amd64_create_target_description'.
>> +
>>  2018-06-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
>>  
>>  	* x86-tdesc.h: Remove executable permission flag.
>> diff --git a/gdb/gdbserver/linux-x86-tdesc.c b/gdb/gdbserver/linux-x86-tdesc.c
>> index 358659b295..7a45953dc9 100644
>> --- a/gdb/gdbserver/linux-x86-tdesc.c
>> +++ b/gdb/gdbserver/linux-x86-tdesc.c
>> @@ -118,7 +118,7 @@ amd64_linux_read_description (uint64_t xcr0, bool is_x32)
>>  
>>    if (*tdesc == NULL)
>>      {
>> -      *tdesc = amd64_create_target_description (xcr0, is_x32, true);
>> +      *tdesc = amd64_create_target_description (xcr0, is_x32, true, false);
>>  
>>        init_target_desc (*tdesc, amd64_expedite_regs);
>>      }
>
> -- 
> Joel

Patch

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 2fd2d90a49..743d465d81 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,8 @@ 
+2018-06-29  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	* linux-x86-tdesc.c (amd64_linux_read_description): Add missing
+	parameter in call to 'amd64_create_target_description'.
+
 2018-06-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* x86-tdesc.h: Remove executable permission flag.
diff --git a/gdb/gdbserver/linux-x86-tdesc.c b/gdb/gdbserver/linux-x86-tdesc.c
index 358659b295..7a45953dc9 100644
--- a/gdb/gdbserver/linux-x86-tdesc.c
+++ b/gdb/gdbserver/linux-x86-tdesc.c
@@ -118,7 +118,7 @@  amd64_linux_read_description (uint64_t xcr0, bool is_x32)
 
   if (*tdesc == NULL)
     {
-      *tdesc = amd64_create_target_description (xcr0, is_x32, true);
+      *tdesc = amd64_create_target_description (xcr0, is_x32, true, false);
 
       init_target_desc (*tdesc, amd64_expedite_regs);
     }