[PATCH] Fix cygwin build error with i386-linux-tdep.c

[PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

Hi guys,

When I try to build gdb in cygwin with "--enable-targets=all
--enable-64-bits-bfd".  I got some error.

gcc -g -O2   -I. -I../../src/gdb -I../../src/gdb/common
-I../../src/gdb/config -DLOCALEDIR="\"/usr/local/share/locale\""
-DHAVE_CONFIG_H -I../
. -I../bfd -I../../src/gdb/../bfd -I../../src/gdb/../include
-I../libdecnumber -I../../src/gdb/../libdecnumber
-I../../src/gdb/gnulib -Ignuli
tatement -Wpointer-arith -Wformat-nonliteral -Wno-unused -Wno-switch
-Wno-char-subscripts -Werror -c -o i386-linux-tdep.o -MT
i386-linux-tdep.
6-linux-tdep.c
../../src/gdb/i386-linux-tdep.c: In function
`i386_linux_intx80_sysenter_record':
../../src/gdb/i386-linux-tdep.c:376: warning: unsigned int format,
uint32_t arg (arg 2)
../../src/gdb/i386-linux-tdep.c:376: warning: unsigned int format,
uint32_t arg (arg 2)
make[2]: *** [i386-linux-tdep.o] Error 1
make[2]: Leaving directory `/home/hzhu/gdb/bgdb/gdb'
make[1]: *** [all-gdb] Error 2
make[1]: Leaving directory `/home/hzhu/gdb/bgdb'
make: *** [all] Error 2

I make a patch to fix it.

Please help me review it.

Thanks,
Hui


2009-08-29  Hui Zhu  <teawater@gmail.com>

	* i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Add
	(unsigned) before tmpu32.

Index: gdb/i386-linux-tdep.c
===================================================================
--- gdb.orig/i386-linux-tdep.c	2009-08-23 21:17:37.000000000 +0800
+++ gdb/i386-linux-tdep.c	2009-08-30 20:19:53.828125000 +0800
@@ -374,7 +374,7 @@
   if (tmpu32 > 499)
     {
       printf_unfiltered (_("Process record and replay target doesn't "
-                           "support syscall number %u\n"), tmpu32);
+                           "support syscall number %u\n"), (unsigned) tmpu32);
       return -1;
     }

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Mark Kettenis]

> From: Hui Zhu <teawater@gmail.com>
> Date: Sun, 30 Aug 2009 21:15:22 +0800
> 
> 2009-08-29  Hui Zhu  <teawater@gmail.com>
> 
> 	* i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Add
> 	(unsigned) before tmpu32.

Ugh!  Casts like that are ugly.

This made me look at the code again and realize that what you're doing
in that function is wrong.  You should be using
regcache_{raw|cooked}_read_unsigned() instead of regcache_raw_read().
Then the whole issue of printing an uint32_t goes away.  When you do
change the code like that please use a more meaningful variable name
instead of 'tmpu32'.  My suggestion would be 'syscall'.

Cheers,

Mark

> Index: gdb/i386-linux-tdep.c
> ===================================================================
> --- gdb.orig/i386-linux-tdep.c	2009-08-23 21:17:37.000000000 +0800
> +++ gdb/i386-linux-tdep.c	2009-08-30 20:19:53.828125000 +0800
> @@ -374,7 +374,7 @@
>    if (tmpu32 > 499)
>      {
>        printf_unfiltered (_("Process record and replay target doesn't "
> -                           "support syscall number %u\n"), tmpu32);
> +                           "support syscall number %u\n"), (unsigned) tmpu32);
>        return -1;
>      }
> 

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

On Sun, Aug 30, 2009 at 21:56, Mark Kettenis<mark.kettenis@xs4all.nl> wrote:
>> From: Hui Zhu <teawater@gmail.com>
>> Date: Sun, 30 Aug 2009 21:15:22 +0800
>>
>> 2009-08-29  Hui Zhu  <teawater@gmail.com>
>>
>>       * i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Add
>>       (unsigned) before tmpu32.
>
> Ugh!  Casts like that are ugly.
>
> This made me look at the code again and realize that what you're doing
> in that function is wrong.  You should be using
> regcache_{raw|cooked}_read_unsigned() instead of regcache_raw_read().
> Then the whole issue of printing an uint32_t goes away.  When you do
> change the code like that please use a more meaningful variable name
> instead of 'tmpu32'.  My suggestion would be 'syscall'.

For the regcache_raw_read_unsigned, I am not agres with it.

void
regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
			    ULONGEST *val)
{
  gdb_byte *buf;
  gdb_assert (regcache != NULL);
  gdb_assert (regnum >= 0 && regnum < regcache->descr->nr_raw_registers);
  buf = alloca (regcache->descr->sizeof_register[regnum]);
  regcache_raw_read (regcache, regnum, buf);
  (*val) = extract_unsigned_integer
	     (buf, regcache->descr->sizeof_register[regnum],
	      gdbarch_byte_order (regcache->descr->gdbarch));
}

It just add a "extract_unsigned_integer".  For this code, it in
i386-linux-tdep.c.  We know that I386_EAX_REGNUM is 32 bits.  So we
don't need extract_unsigned_integer to set anything.

Thanks,
Hui

>
> Cheers,
>
> Mark
>
>> Index: gdb/i386-linux-tdep.c
>> ===================================================================
>> --- gdb.orig/i386-linux-tdep.c        2009-08-23 21:17:37.000000000 +0800
>> +++ gdb/i386-linux-tdep.c     2009-08-30 20:19:53.828125000 +0800
>> @@ -374,7 +374,7 @@
>>    if (tmpu32 > 499)
>>      {
>>        printf_unfiltered (_("Process record and replay target doesn't "
>> -                           "support syscall number %u\n"), tmpu32);
>> +                           "support syscall number %u\n"), (unsigned) tmpu32);
>>        return -1;
>>      }
>>
>

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Andreas Schwab]

Mark Kettenis <mark.kettenis@xs4all.nl> writes:

>> From: Hui Zhu <teawater@gmail.com>
>> Date: Sun, 30 Aug 2009 21:15:22 +0800
>> 
>> Index: gdb/i386-linux-tdep.c
>> ===================================================================
>> --- gdb.orig/i386-linux-tdep.c	2009-08-23 21:17:37.000000000 +0800
>> +++ gdb/i386-linux-tdep.c	2009-08-30 20:19:53.828125000 +0800
>> @@ -374,7 +374,7 @@
>>    if (tmpu32 > 499)

Also, magic numbers like this are bad as well.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

On Sun, Aug 30, 2009 at 22:09, Hui Zhu<teawater@gmail.com> wrote:
> On Sun, Aug 30, 2009 at 21:56, Mark Kettenis<mark.kettenis@xs4all.nl> wrote:
>>> From: Hui Zhu <teawater@gmail.com>
>>> Date: Sun, 30 Aug 2009 21:15:22 +0800
>>>
>>> 2009-08-29  Hui Zhu  <teawater@gmail.com>
>>>
>>>       * i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Add
>>>       (unsigned) before tmpu32.
>>
>> Ugh!  Casts like that are ugly.
>>
>> This made me look at the code again and realize that what you're doing
>> in that function is wrong.  You should be using
>> regcache_{raw|cooked}_read_unsigned() instead of regcache_raw_read().
>> Then the whole issue of printing an uint32_t goes away.  When you do
>> change the code like that please use a more meaningful variable name
>> instead of 'tmpu32'.  My suggestion would be 'syscall'.
>
> For the regcache_raw_read_unsigned, I am not agres with it.
>
> void
> regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
>                            ULONGEST *val)
> {
>  gdb_byte *buf;
>  gdb_assert (regcache != NULL);
>  gdb_assert (regnum >= 0 && regnum < regcache->descr->nr_raw_registers);
>  buf = alloca (regcache->descr->sizeof_register[regnum]);
>  regcache_raw_read (regcache, regnum, buf);
>  (*val) = extract_unsigned_integer
>             (buf, regcache->descr->sizeof_register[regnum],
>              gdbarch_byte_order (regcache->descr->gdbarch));
> }
>
> It just add a "extract_unsigned_integer".  For this code, it in
> i386-linux-tdep.c.  We know that I386_EAX_REGNUM is 32 bits.  So we
> don't need extract_unsigned_integer to set anything.
>
> Thanks,
> Hui

Oops, I forget that we have the byte order trouble.  Thanks for remind
me.  I will fix it.

Thanks,
Hui

>
>>
>> Cheers,
>>
>> Mark
>>
>>> Index: gdb/i386-linux-tdep.c
>>> ===================================================================
>>> --- gdb.orig/i386-linux-tdep.c        2009-08-23 21:17:37.000000000 +0800
>>> +++ gdb/i386-linux-tdep.c     2009-08-30 20:19:53.828125000 +0800
>>> @@ -374,7 +374,7 @@
>>>    if (tmpu32 > 499)
>>>      {
>>>        printf_unfiltered (_("Process record and replay target doesn't "
>>> -                           "support syscall number %u\n"), tmpu32);
>>> +                           "support syscall number %u\n"), (unsigned) tmpu32);
>>>        return -1;
>>>      }
>>>
>>
>

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

On Sun, Aug 30, 2009 at 22:19, Andreas Schwab<schwab@linux-m68k.org> wrote:
> Mark Kettenis <mark.kettenis@xs4all.nl> writes:
>
>>> From: Hui Zhu <teawater@gmail.com>
>>> Date: Sun, 30 Aug 2009 21:15:22 +0800
>>>
>>> Index: gdb/i386-linux-tdep.c
>>> ===================================================================
>>> --- gdb.orig/i386-linux-tdep.c       2009-08-23 21:17:37.000000000 +0800
>>> +++ gdb/i386-linux-tdep.c    2009-08-30 20:19:53.828125000 +0800
>>> @@ -374,7 +374,7 @@
>>>    if (tmpu32 > 499)
>
> Also, magic numbers like this are bad as well.
>
> Andreas.

If you don't mind, please let me fix it step by step.  Let me fix the
reg value issue firest.  :)

Thanks,
Hui

>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."
>

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Jiang Jilin]

On Sun, Aug 30, 2009 at 9:15 PM, Hui Zhu<teawater@gmail.com> wrote:
> Hi guys,
>
>   if (tmpu32 > 499)
>     {
>       printf_unfiltered (_("Process record and replay target doesn't "
> -                           "support syscall number %u\n"), tmpu32);
> +                           "support syscall number %u\n"), (unsigned) tmpu32);
>       return -1;
>     }
>

Sorry, it's not clear to me.

when looking into the code, I found the type of tmpu32 is uint32_t,
why did the gcc complain "warning: unsigned int format, uint32_t arg
(arg 2)"?

%u doesn't mean unsigned int?


-- 
Thanks

Jiang

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

On Sun, Aug 30, 2009 at 23:13, Jiang Jilin<freephp@gmail.com> wrote:
> On Sun, Aug 30, 2009 at 9:15 PM, Hui Zhu<teawater@gmail.com> wrote:
>> Hi guys,
>>
>>   if (tmpu32 > 499)
>>     {
>>       printf_unfiltered (_("Process record and replay target doesn't "
>> -                           "support syscall number %u\n"), tmpu32);
>> +                           "support syscall number %u\n"), (unsigned) tmpu32);
>>       return -1;
>>     }
>>
>
> Sorry, it's not clear to me.
>
> when looking into the code, I found the type of tmpu32 is uint32_t,
> why did the gcc complain "warning: unsigned int format, uint32_t arg
> (arg 2)"?
>
> %u doesn't mean unsigned int?
>

Really?  what does it mean?

Hui

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Jiang Jilin]

On Sun, Aug 30, 2009 at 11:22 PM, Hui Zhu<teawater@gmail.com> wrote:
> On Sun, Aug 30, 2009 at 23:13, Jiang Jilin<freephp@gmail.com> wrote:
>> On Sun, Aug 30, 2009 at 9:15 PM, Hui Zhu<teawater@gmail.com> wrote:
>>> Hi guys,
>>>
>>>   if (tmpu32 > 499)
>>>     {
>>>       printf_unfiltered (_("Process record and replay target doesn't "
>>> -                           "support syscall number %u\n"), tmpu32);
>>> +                           "support syscall number %u\n"), (unsigned) tmpu32);
>>>       return -1;
>>>     }
>>>
>>
>> Sorry, it's not clear to me.
>>
>> when looking into the code, I found the type of tmpu32 is uint32_t,
>> why did the gcc complain "warning: unsigned int format, uint32_t arg
>> (arg 2)"?
>>
>> %u doesn't mean unsigned int?
>>
>
> Really?  what does it mean?

I mean without your patch, the type of tmpu32 _is_ uint32_t, why did
gcc complain? and when you cast it to unsigned, the warning
disappeared?



-- 
Thanks

Jiang

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

On Mon, Aug 31, 2009 at 07:39, Jiang Jilin<freephp@gmail.com> wrote:
> On Sun, Aug 30, 2009 at 11:22 PM, Hui Zhu<teawater@gmail.com> wrote:
>> On Sun, Aug 30, 2009 at 23:13, Jiang Jilin<freephp@gmail.com> wrote:
>>> On Sun, Aug 30, 2009 at 9:15 PM, Hui Zhu<teawater@gmail.com> wrote:
>>>> Hi guys,
>>>>
>>>>   if (tmpu32 > 499)
>>>>     {
>>>>       printf_unfiltered (_("Process record and replay target doesn't "
>>>> -                           "support syscall number %u\n"), tmpu32);
>>>> +                           "support syscall number %u\n"), (unsigned) tmpu32);
>>>>       return -1;
>>>>     }
>>>>
>>>
>>> Sorry, it's not clear to me.
>>>
>>> when looking into the code, I found the type of tmpu32 is uint32_t,
>>> why did the gcc complain "warning: unsigned int format, uint32_t arg
>>> (arg 2)"?
>>>
>>> %u doesn't mean unsigned int?
>>>
>>
>> Really?  what does it mean?
>
> I mean without your patch, the type of tmpu32 _is_ uint32_t, why did
> gcc complain? and when you cast it to unsigned, the warning
> disappeared?
>

I am not sure the unit32_t in cygwin.  But it looks like uint32_t is
not unsigned int in cygwin.
And "unsigned" is seems "unsigned int".

Thanks,
Hui

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Jiang Jilin]

On Mon, Aug 31, 2009 at 7:43 AM, Hui Zhu<teawater@gmail.com> wrote:
> On Mon, Aug 31, 2009 at 07:39, Jiang Jilin<freephp@gmail.com> wrote:
>> On Sun, Aug 30, 2009 at 11:22 PM, Hui Zhu<teawater@gmail.com> wrote:
>>> On Sun, Aug 30, 2009 at 23:13, Jiang Jilin<freephp@gmail.com> wrote:
>>>> On Sun, Aug 30, 2009 at 9:15 PM, Hui Zhu<teawater@gmail.com> wrote:
>>>>> Hi guys,
>>>>>
>>>>>   if (tmpu32 > 499)
>>>>>     {
>>>>>       printf_unfiltered (_("Process record and replay target doesn't "
>>>>> -                           "support syscall number %u\n"), tmpu32);
>>>>> +                           "support syscall number %u\n"), (unsigned) tmpu32);
>>>>>       return -1;
>>>>>     }
>>>>>
>>>>
>>>> Sorry, it's not clear to me.
>>>>
>>>> when looking into the code, I found the type of tmpu32 is uint32_t,
>>>> why did the gcc complain "warning: unsigned int format, uint32_t arg
>>>> (arg 2)"?
>>>>
>>>> %u doesn't mean unsigned int?
>>>>
>>>
>>> Really?  what does it mean?
>>
>> I mean without your patch, the type of tmpu32 _is_ uint32_t, why did
>> gcc complain? and when you cast it to unsigned, the warning
>> disappeared?
>>
>
> I am not sure the unit32_t in cygwin.  But it looks like uint32_t is
> not unsigned int in cygwin.
> And "unsigned" is seems "unsigned int".

If it's true, that is, uint32_t is not unsigned int in cygwin,  I
think there is a  bug in cygwin, we should redefine the uint32_t to
real unsigned int, shouldn't we?



-- 
Thanks

Jiang

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Jiang Jilin]

On Mon, Aug 31, 2009 at 7:52 AM, Jiang Jilin<freephp@gmail.com> wrote:
> On Mon, Aug 31, 2009 at 7:43 AM, Hui Zhu<teawater@gmail.com> wrote:
>> On Mon, Aug 31, 2009 at 07:39, Jiang Jilin<freephp@gmail.com> wrote:
>>> On Sun, Aug 30, 2009 at 11:22 PM, Hui Zhu<teawater@gmail.com> wrote:
>>>> On Sun, Aug 30, 2009 at 23:13, Jiang Jilin<freephp@gmail.com> wrote:
>>>>> On Sun, Aug 30, 2009 at 9:15 PM, Hui Zhu<teawater@gmail.com> wrote:
>>>>>> Hi guys,
>>>>>>
>>>>>>   if (tmpu32 > 499)
>>>>>>     {
>>>>>>       printf_unfiltered (_("Process record and replay target doesn't "
>>>>>> -                           "support syscall number %u\n"), tmpu32);
>>>>>> +                           "support syscall number %u\n"), (unsigned) tmpu32);
>>>>>>       return -1;
>>>>>>     }
>>>>>>
>>>>>
>>>>> Sorry, it's not clear to me.
>>>>>
>>>>> when looking into the code, I found the type of tmpu32 is uint32_t,
>>>>> why did the gcc complain "warning: unsigned int format, uint32_t arg
>>>>> (arg 2)"?
>>>>>
>>>>> %u doesn't mean unsigned int?
>>>>>
>>>>
>>>> Really?  what does it mean?
>>>
>>> I mean without your patch, the type of tmpu32 _is_ uint32_t, why did
>>> gcc complain? and when you cast it to unsigned, the warning
>>> disappeared?
>>>
>>
>> I am not sure the unit32_t in cygwin.  But it looks like uint32_t is
>> not unsigned int in cygwin.
>> And "unsigned" is seems "unsigned int".
>
> If it's true, that is, uint32_t is not unsigned int in cygwin,  I
> think there is a  bug in cygwin, we should redefine the uint32_t to
> real unsigned int, shouldn't we?

ok, I got it.  uint32_t is typedefed to  long unsigned int in cygwin .

Thank!

Jiang

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

On Sun, Aug 30, 2009 at 22:37, Hui Zhu<teawater@gmail.com> wrote:
> On Sun, Aug 30, 2009 at 22:09, Hui Zhu<teawater@gmail.com> wrote:
>> On Sun, Aug 30, 2009 at 21:56, Mark Kettenis<mark.kettenis@xs4all.nl> wrote:
>>>> From: Hui Zhu <teawater@gmail.com>
>>>> Date: Sun, 30 Aug 2009 21:15:22 +0800
>>>>
>>>> 2009-08-29  Hui Zhu  <teawater@gmail.com>
>>>>
>>>>       * i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Add
>>>>       (unsigned) before tmpu32.
>>>
>>> Ugh!  Casts like that are ugly.
>>>
>>> This made me look at the code again and realize that what you're doing
>>> in that function is wrong.  You should be using
>>> regcache_{raw|cooked}_read_unsigned() instead of regcache_raw_read().
>>> Then the whole issue of printing an uint32_t goes away.  When you do
>>> change the code like that please use a more meaningful variable name
>>> instead of 'tmpu32'.  My suggestion would be 'syscall'.
>>
>> For the regcache_raw_read_unsigned, I am not agres with it.
>>
>> void
>> regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
>>                            ULONGEST *val)
>> {
>>  gdb_byte *buf;
>>  gdb_assert (regcache != NULL);
>>  gdb_assert (regnum >= 0 && regnum < regcache->descr->nr_raw_registers);
>>  buf = alloca (regcache->descr->sizeof_register[regnum]);
>>  regcache_raw_read (regcache, regnum, buf);
>>  (*val) = extract_unsigned_integer
>>             (buf, regcache->descr->sizeof_register[regnum],
>>              gdbarch_byte_order (regcache->descr->gdbarch));
>> }
>>
>> It just add a "extract_unsigned_integer".  For this code, it in
>> i386-linux-tdep.c.  We know that I386_EAX_REGNUM is 32 bits.  So we
>> don't need extract_unsigned_integer to set anything.
>>
>> Thanks,
>> Hui
>
> Oops, I forget that we have the byte order trouble.  Thanks for remind
> me.  I will fix it.
>
> Thanks,
> Hui
>
>>
>>>
>>> Cheers,
>>>
>>> Mark
>>>
>>>> Index: gdb/i386-linux-tdep.c
>>>> ===================================================================
>>>> --- gdb.orig/i386-linux-tdep.c        2009-08-23 21:17:37.000000000 +0800
>>>> +++ gdb/i386-linux-tdep.c     2009-08-30 20:19:53.828125000 +0800
>>>> @@ -374,7 +374,7 @@
>>>>    if (tmpu32 > 499)
>>>>      {
>>>>        printf_unfiltered (_("Process record and replay target doesn't "
>>>> -                           "support syscall number %u\n"), tmpu32);
>>>> +                           "support syscall number %u\n"), (unsigned) tmpu32);
>>>>        return -1;
>>>>      }
>>>>
>>>
>>
>

Hi guys,

I make a new patch that change regcache_raw_read to regcache_raw_read_unsigned.
Please help me review it.

Thanks,
Hui

2009-08-31  Hui Zhu  <teawater@gmail.com>

	* i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Change
	regcache_raw_read to regcache_raw_read_unsigned.

---
 i386-linux-tdep.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

--- a/i386-linux-tdep.c
+++ b/i386-linux-tdep.c
@@ -367,18 +367,18 @@ static int
 i386_linux_intx80_sysenter_record (struct regcache *regcache)
 {
   int ret;
-  uint32_t tmpu32;
+  ULONGEST num;

-  regcache_raw_read (regcache, I386_EAX_REGNUM, (gdb_byte *) &tmpu32);
+  regcache_raw_read_unsigned (regcache, I386_EAX_REGNUM, &num);

-  if (tmpu32 > 499)
+  if (num > 499)
     {
       printf_unfiltered (_("Process record and replay target doesn't "
-                           "support syscall number %u\n"), tmpu32);
+                           "support syscall number %d\n"), (int) num);
       return -1;
     }

-  ret = record_linux_system_call (tmpu32, regcache,
+  ret = record_linux_system_call ((int) num, regcache,
 				  &i386_linux_record_tdep);
   if (ret)
     return ret;

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Jiang Jilin]

On Mon, Aug 31, 2009 at 4:22 PM, Hui Zhu<teawater@gmail.com> wrote:

> Hi guys,
>
> I make a new patch that change regcache_raw_read to regcache_raw_read_unsigned.
> Please help me review it.
>
> Thanks,
> Hui
>
> 2009-08-31  Hui Zhu  <teawater@gmail.com>
>
>        * i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Change
>        regcache_raw_read to regcache_raw_read_unsigned.
>
> ---
>  i386-linux-tdep.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> --- a/i386-linux-tdep.c
> +++ b/i386-linux-tdep.c
> @@ -367,18 +367,18 @@ static int
>  i386_linux_intx80_sysenter_record (struct regcache *regcache)
>  {
>   int ret;
> -  uint32_t tmpu32;
> +  ULONGEST num;
>
> -  regcache_raw_read (regcache, I386_EAX_REGNUM, (gdb_byte *) &tmpu32);
> +  regcache_raw_read_unsigned (regcache, I386_EAX_REGNUM, &num);
>
> -  if (tmpu32 > 499)
> +  if (num > 499)
>     {
>       printf_unfiltered (_("Process record and replay target doesn't "
> -                           "support syscall number %u\n"), tmpu32);
> +                           "support syscall number %d\n"), (int) num);

if you want an integer read, why not use regcache_raw_read_signed() instead?

>       return -1;
>     }
>
> -  ret = record_linux_system_call (tmpu32, regcache,
> +  ret = record_linux_system_call ((int) num, regcache,
>                                  &i386_linux_record_tdep);
>   if (ret)
>     return ret;
>

I think it's not very sensible to cast unsigned to signed, think about
if the unsigned value is _very_ big.


-- 
Thanks

Jiang

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

On Mon, Aug 31, 2009 at 16:45, Jiang Jilin<freephp@gmail.com> wrote:
> On Mon, Aug 31, 2009 at 4:22 PM, Hui Zhu<teawater@gmail.com> wrote:
>
>> Hi guys,
>>
>> I make a new patch that change regcache_raw_read to regcache_raw_read_unsigned.
>> Please help me review it.
>>
>> Thanks,
>> Hui
>>
>> 2009-08-31  Hui Zhu  <teawater@gmail.com>
>>
>>        * i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Change
>>        regcache_raw_read to regcache_raw_read_unsigned.
>>
>> ---
>>  i386-linux-tdep.c |   10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> --- a/i386-linux-tdep.c
>> +++ b/i386-linux-tdep.c
>> @@ -367,18 +367,18 @@ static int
>>  i386_linux_intx80_sysenter_record (struct regcache *regcache)
>>  {
>>   int ret;
>> -  uint32_t tmpu32;
>> +  ULONGEST num;
>>
>> -  regcache_raw_read (regcache, I386_EAX_REGNUM, (gdb_byte *) &tmpu32);
>> +  regcache_raw_read_unsigned (regcache, I386_EAX_REGNUM, &num);
>>
>> -  if (tmpu32 > 499)
>> +  if (num > 499)
>>     {
>>       printf_unfiltered (_("Process record and replay target doesn't "
>> -                           "support syscall number %u\n"), tmpu32);
>> +                           "support syscall number %d\n"), (int) num);
>
> if you want an integer read, why not use regcache_raw_read_signed() instead?
>
>>       return -1;
>>     }
>>
>> -  ret = record_linux_system_call (tmpu32, regcache,
>> +  ret = record_linux_system_call ((int) num, regcache,
>>                                  &i386_linux_record_tdep);
>>   if (ret)
>>     return ret;
>>
>
> I think it's not very sensible to cast unsigned to signed, think about
> if the unsigned value is _very_ big.
>

This is syscall id, it will not _very_ big.

Hui

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Mark Kettenis]

> From: Hui Zhu <teawater@gmail.com>
> Date: Mon, 31 Aug 2009 16:48:33 +0800
> 
> On Mon, Aug 31, 2009 at 16:45, Jiang Jilin<freephp@gmail.com> wrote:
> >
> > I think it's not very sensible to cast unsigned to signed, think about
> > if the unsigned value is _very_ big.
> >
> 
> This is syscall id, it will not _very_ big.

Jiang has a point here though.  The cast is weird, and only there
because it seems you can't make up your mind whether syscall numbers
are signed or unsigned.  Some bits of code use signed integers and
some use unsigned integers.  Once that inconsistency is fixed, all
these problems will disappear.

Mark

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

On Mon, Aug 31, 2009 at 23:46, Mark Kettenis<mark.kettenis@xs4all.nl> wrote:
>> From: Hui Zhu <teawater@gmail.com>
>> Date: Mon, 31 Aug 2009 16:48:33 +0800
>>
>> On Mon, Aug 31, 2009 at 16:45, Jiang Jilin<freephp@gmail.com> wrote:
>> >
>> > I think it's not very sensible to cast unsigned to signed, think about
>> > if the unsigned value is _very_ big.
>> >
>>
>> This is syscall id, it will not _very_ big.
>
> Jiang has a point here though.  The cast is weird, and only there
> because it seems you can't make up your mind whether syscall numbers
> are signed or unsigned.  Some bits of code use signed integers and
> some use unsigned integers.  Once that inconsistency is fixed, all
> these problems will disappear.
>
> Mark
>
>

  regcache_raw_read_unsigned (regcache, I386_EAX_REGNUM, &num);

  if (num > 499)
    {
      printf_unfiltered (_("Process record and replay target doesn't "
                           "support syscall number %d\n"), (int) num);
      return -1;
    }

  ret = record_linux_system_call ((int) num, regcache,

This num that will send to "record_linux_system_call" is 0 - 499.
So it don't have big or small trouble, right?


Hui

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Michael Snyder]

Hui Zhu wrote:

> Hi guys,
> 
> I make a new patch that change regcache_raw_read to regcache_raw_read_unsigned.
> Please help me review it.
> 
> Thanks,
> Hui
> 
> 2009-08-31  Hui Zhu  <teawater@gmail.com>
> 
> 	* i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Change
> 	regcache_raw_read to regcache_raw_read_unsigned.
> 
> ---
>  i386-linux-tdep.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> --- a/i386-linux-tdep.c
> +++ b/i386-linux-tdep.c
> @@ -367,18 +367,18 @@ static int
>  i386_linux_intx80_sysenter_record (struct regcache *regcache)
>  {
>    int ret;
> -  uint32_t tmpu32;
> +  ULONGEST num;

I like Mark's suggestion of calling it "syscall".
Calling it "num" is not much better than calling it "tmp".   ;-)

Now, talking about signed vs. unsigned vs. casts...

You declared it a ULONGEST because that's what
regcache_raw_read_unsigned expects, right?  -- fair enough.
See below.

> -  regcache_raw_read (regcache, I386_EAX_REGNUM, (gdb_byte *) &tmpu32);
> +  regcache_raw_read_unsigned (regcache, I386_EAX_REGNUM, &num);
> 
> -  if (tmpu32 > 499)
> +  if (num > 499)
>      {
>        printf_unfiltered (_("Process record and replay target doesn't "
> -                           "support syscall number %u\n"), tmpu32);
> +                           "support syscall number %d\n"), (int) num);
>        return -1;
>      }
> 
> -  ret = record_linux_system_call (tmpu32, regcache,
> +  ret = record_linux_system_call ((int) num, regcache,
>  				  &i386_linux_record_tdep);

But here you cast it to int because that's what
record_linux_system_call expects, right?

And since we know at this point that the value is between
0 and 499, the cast can't really do any harm, so it seems
fair.

I could suggest casting it to (unsigned int), but it wouldn't
really make any difference, would it?  Mark -- Jiang -- would
that make you guys more comfortable?

Or would it be more correct to change the prototype of
linux-record-system-call so that it expects an unsigned int?


>    if (ret)
>      return ret;
> 

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Joel Brobecker]

> I could suggest casting it to (unsigned int), but it wouldn't
> really make any difference, would it?  Mark -- Jiang -- would
> that make you guys more comfortable?

Short term, I'd rather see us read the syscall number as a signed
number since this is what record_linux_system_call expects. We can
decide whether to rationalize as a signed or unsigned as a separate
patch. I think Mark was OK with the patch I sent yesterday, except
that he said we should add a check against negative values.

There is also a cast that is unnecessary in the error message.
Hui can use %s/paddress rather than %d/cast.

-- 
Joel

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]

On Sun, Sep 6, 2009 at 05:15, Joel Brobecker<brobecker@adacore.com> wrote:
>> I could suggest casting it to (unsigned int), but it wouldn't
>> really make any difference, would it?  Mark -- Jiang -- would
>> that make you guys more comfortable?
>
> Short term, I'd rather see us read the syscall number as a signed
> number since this is what record_linux_system_call expects. We can
> decide whether to rationalize as a signed or unsigned as a separate
> patch. I think Mark was OK with the patch I sent yesterday, except
> that he said we should add a check against negative values.
>
> There is also a cast that is unnecessary in the error message.
> Hui can use %s/paddress rather than %d/cast.
>
> --
> Joel
>

Hi guys,

I make a new patch according to your comment.

Thanks,
Hui


2009-09-06  Michael Snyder  <msnyder@vmware.com>
	    Joel Brobecker  <brobecker@adacore.com>
	    Hui Zhu  <teawater@gmail.com>

	* i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Change
	regcache_raw_read to regcache_raw_read_signed.

Index: i386-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-linux-tdep.c,v
retrieving revision 1.66
diff -u -p -r1.66 i386-linux-tdep.c
--- i386-linux-tdep.c	10 Aug 2009 03:04:44 -0000	1.66
+++ i386-linux-tdep.c	6 Sep 2009 02:12:15 -0000
@@ -367,18 +367,19 @@ static int
 i386_linux_intx80_sysenter_record (struct regcache *regcache)
 {
   int ret;
-  uint32_t tmpu32;
+  LONGEST syscall;

-  regcache_raw_read (regcache, I386_EAX_REGNUM, (gdb_byte *) &tmpu32);
+  regcache_raw_read_signed (regcache, I386_EAX_REGNUM, &syscall);

-  if (tmpu32 > 499)
+  if (syscall < 0 || syscall > 499)
     {
       printf_unfiltered (_("Process record and replay target doesn't "
-                           "support syscall number %u\n"), tmpu32);
+                           "support syscall number %s\n"),
+			 plongest (syscall));
       return -1;
     }

-  ret = record_linux_system_call (tmpu32, regcache,
+  ret = record_linux_system_call (syscall, regcache,
 				  &i386_linux_record_tdep);
   if (ret)
     return ret;

[-- Attachment #2: longest.txt --]
[-- Type: text/plain, Size: 1086 bytes --]

Index: i386-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-linux-tdep.c,v
retrieving revision 1.66
diff -u -p -r1.66 i386-linux-tdep.c
--- i386-linux-tdep.c	10 Aug 2009 03:04:44 -0000	1.66
+++ i386-linux-tdep.c	6 Sep 2009 02:12:15 -0000
@@ -367,18 +367,19 @@ static int
 i386_linux_intx80_sysenter_record (struct regcache *regcache)
 {
   int ret;
-  uint32_t tmpu32;
+  LONGEST syscall;
 
-  regcache_raw_read (regcache, I386_EAX_REGNUM, (gdb_byte *) &tmpu32);
+  regcache_raw_read_signed (regcache, I386_EAX_REGNUM, &syscall);
 
-  if (tmpu32 > 499)
+  if (syscall < 0 || syscall > 499)
     {
       printf_unfiltered (_("Process record and replay target doesn't "
-                           "support syscall number %u\n"), tmpu32);
+                           "support syscall number %s\n"), 
+			 plongest (syscall));
       return -1;
     }
 
-  ret = record_linux_system_call (tmpu32, regcache,
+  ret = record_linux_system_call (syscall, regcache,
 				  &i386_linux_record_tdep);
   if (ret)
     return ret;

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

Hi guys,

What do you think about this patch?

Hui

On Sun, Sep 6, 2009 at 12:15, Hui Zhu<teawater@gmail.com> wrote:
> On Sun, Sep 6, 2009 at 05:15, Joel Brobecker<brobecker@adacore.com> wrote:
>>> I could suggest casting it to (unsigned int), but it wouldn't
>>> really make any difference, would it?  Mark -- Jiang -- would
>>> that make you guys more comfortable?
>>
>> Short term, I'd rather see us read the syscall number as a signed
>> number since this is what record_linux_system_call expects. We can
>> decide whether to rationalize as a signed or unsigned as a separate
>> patch. I think Mark was OK with the patch I sent yesterday, except
>> that he said we should add a check against negative values.
>>
>> There is also a cast that is unnecessary in the error message.
>> Hui can use %s/paddress rather than %d/cast.
>>
>> --
>> Joel
>>
>
> Hi guys,
>
> I make a new patch according to your comment.
>
> Thanks,
> Hui
>
>
> 2009-09-06  Michael Snyder  <msnyder@vmware.com>
>            Joel Brobecker  <brobecker@adacore.com>
>            Hui Zhu  <teawater@gmail.com>
>
>        * i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Change
>        regcache_raw_read to regcache_raw_read_signed.
>
> Index: i386-linux-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-linux-tdep.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 i386-linux-tdep.c
> --- i386-linux-tdep.c   10 Aug 2009 03:04:44 -0000      1.66
> +++ i386-linux-tdep.c   6 Sep 2009 02:12:15 -0000
> @@ -367,18 +367,19 @@ static int
>  i386_linux_intx80_sysenter_record (struct regcache *regcache)
>  {
>   int ret;
> -  uint32_t tmpu32;
> +  LONGEST syscall;
>
> -  regcache_raw_read (regcache, I386_EAX_REGNUM, (gdb_byte *) &tmpu32);
> +  regcache_raw_read_signed (regcache, I386_EAX_REGNUM, &syscall);
>
> -  if (tmpu32 > 499)
> +  if (syscall < 0 || syscall > 499)
>     {
>       printf_unfiltered (_("Process record and replay target doesn't "
> -                           "support syscall number %u\n"), tmpu32);
> +                           "support syscall number %s\n"),
> +                        plongest (syscall));
>       return -1;
>     }
>
> -  ret = record_linux_system_call (tmpu32, regcache,
> +  ret = record_linux_system_call (syscall, regcache,
>                                  &i386_linux_record_tdep);
>   if (ret)
>     return ret;
>

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Michael Snyder]

Hui Zhu wrote:
> Hi guys,
> 
> What do you think about this patch?

Hey, Hui,

Appologies -- this patch of yours sort of got subsumed into my
bigger syscall patch.  If you look at CVS head, I think everything
that this patch changes is already in.

Didn't mean to "cut you out"...   ;-)

Michael


> On Sun, Sep 6, 2009 at 12:15, Hui Zhu<teawater@gmail.com> wrote:
>> On Sun, Sep 6, 2009 at 05:15, Joel Brobecker<brobecker@adacore.com> wrote:
>>>> I could suggest casting it to (unsigned int), but it wouldn't
>>>> really make any difference, would it?  Mark -- Jiang -- would
>>>> that make you guys more comfortable?
>>> Short term, I'd rather see us read the syscall number as a signed
>>> number since this is what record_linux_system_call expects. We can
>>> decide whether to rationalize as a signed or unsigned as a separate
>>> patch. I think Mark was OK with the patch I sent yesterday, except
>>> that he said we should add a check against negative values.
>>>
>>> There is also a cast that is unnecessary in the error message.
>>> Hui can use %s/paddress rather than %d/cast.
>>>
>>> --
>>> Joel
>>>
>> Hi guys,
>>
>> I make a new patch according to your comment.
>>
>> Thanks,
>> Hui
>>
>>
>> 2009-09-06  Michael Snyder  <msnyder@vmware.com>
>>            Joel Brobecker  <brobecker@adacore.com>
>>            Hui Zhu  <teawater@gmail.com>
>>
>>        * i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Change
>>        regcache_raw_read to regcache_raw_read_signed.
>>
>> Index: i386-linux-tdep.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/i386-linux-tdep.c,v
>> retrieving revision 1.66
>> diff -u -p -r1.66 i386-linux-tdep.c
>> --- i386-linux-tdep.c   10 Aug 2009 03:04:44 -0000      1.66
>> +++ i386-linux-tdep.c   6 Sep 2009 02:12:15 -0000
>> @@ -367,18 +367,19 @@ static int
>>  i386_linux_intx80_sysenter_record (struct regcache *regcache)
>>  {
>>   int ret;
>> -  uint32_t tmpu32;
>> +  LONGEST syscall;
>>
>> -  regcache_raw_read (regcache, I386_EAX_REGNUM, (gdb_byte *) &tmpu32);
>> +  regcache_raw_read_signed (regcache, I386_EAX_REGNUM, &syscall);
>>
>> -  if (tmpu32 > 499)
>> +  if (syscall < 0 || syscall > 499)
>>     {
>>       printf_unfiltered (_("Process record and replay target doesn't "
>> -                           "support syscall number %u\n"), tmpu32);
>> +                           "support syscall number %s\n"),
>> +                        plongest (syscall));
>>       return -1;
>>     }
>>
>> -  ret = record_linux_system_call (tmpu32, regcache,
>> +  ret = record_linux_system_call (syscall, regcache,
>>                                  &i386_linux_record_tdep);
>>   if (ret)
>>     return ret;
>>

Re: [PATCH] Fix cygwin build error with i386-linux-tdep.c

[Hui Zhu]

On Wed, Sep 9, 2009 at 11:29, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> Hi guys,
>>
>> What do you think about this patch?
>
> Hey, Hui,
>
> Appologies -- this patch of yours sort of got subsumed into my
> bigger syscall patch.  If you look at CVS head, I think everything
> that this patch changes is already in.

Great.
Thanks a lot.

Hui

>
> Didn't mean to "cut you out"...   ;-)
>
> Michael
>
>
>> On Sun, Sep 6, 2009 at 12:15, Hui Zhu<teawater@gmail.com> wrote:
>>>
>>> On Sun, Sep 6, 2009 at 05:15, Joel Brobecker<brobecker@adacore.com>
>>> wrote:
>>>>>
>>>>> I could suggest casting it to (unsigned int), but it wouldn't
>>>>> really make any difference, would it?  Mark -- Jiang -- would
>>>>> that make you guys more comfortable?
>>>>
>>>> Short term, I'd rather see us read the syscall number as a signed
>>>> number since this is what record_linux_system_call expects. We can
>>>> decide whether to rationalize as a signed or unsigned as a separate
>>>> patch. I think Mark was OK with the patch I sent yesterday, except
>>>> that he said we should add a check against negative values.
>>>>
>>>> There is also a cast that is unnecessary in the error message.
>>>> Hui can use %s/paddress rather than %d/cast.
>>>>
>>>> --
>>>> Joel
>>>>
>>> Hi guys,
>>>
>>> I make a new patch according to your comment.
>>>
>>> Thanks,
>>> Hui
>>>
>>>
>>> 2009-09-06  Michael Snyder  <msnyder@vmware.com>
>>>           Joel Brobecker  <brobecker@adacore.com>
>>>           Hui Zhu  <teawater@gmail.com>
>>>
>>>       * i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Change
>>>       regcache_raw_read to regcache_raw_read_signed.
>>>
>>> Index: i386-linux-tdep.c
>>> ===================================================================
>>> RCS file: /cvs/src/src/gdb/i386-linux-tdep.c,v
>>> retrieving revision 1.66
>>> diff -u -p -r1.66 i386-linux-tdep.c
>>> --- i386-linux-tdep.c   10 Aug 2009 03:04:44 -0000      1.66
>>> +++ i386-linux-tdep.c   6 Sep 2009 02:12:15 -0000
>>> @@ -367,18 +367,19 @@ static int
>>>  i386_linux_intx80_sysenter_record (struct regcache *regcache)
>>>  {
>>>  int ret;
>>> -  uint32_t tmpu32;
>>> +  LONGEST syscall;
>>>
>>> -  regcache_raw_read (regcache, I386_EAX_REGNUM, (gdb_byte *) &tmpu32);
>>> +  regcache_raw_read_signed (regcache, I386_EAX_REGNUM, &syscall);
>>>
>>> -  if (tmpu32 > 499)
>>> +  if (syscall < 0 || syscall > 499)
>>>    {
>>>      printf_unfiltered (_("Process record and replay target doesn't "
>>> -                           "support syscall number %u\n"), tmpu32);
>>> +                           "support syscall number %s\n"),
>>> +                        plongest (syscall));
>>>      return -1;
>>>    }
>>>
>>> -  ret = record_linux_system_call (tmpu32, regcache,
>>> +  ret = record_linux_system_call (syscall, regcache,
>>>                                 &i386_linux_record_tdep);
>>>  if (ret)
>>>    return ret;
>>>
>
>