* [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression
@ 2014-07-20 14:03 Chen Gang
2014-07-23 20:04 ` Michael Eager
0 siblings, 1 reply; 10+ messages in thread
From: Chen Gang @ 2014-07-20 14:03 UTC (permalink / raw)
To: eager; +Cc: binutils, gdb-patches
Use typecast 'size_t' on 'reg', not only avoid the related warning, but
also check whether less than zero -- for 'reg' is type 'int', and sizeof
(dwarf2_to_reg_map) is less than 0x7fff.
It is quoted in gdb_assert(), so need check 'reg' whether less than zero.
And the related warning (with '-W'):
../../binutils-gdb/gdb/microblaze-tdep.c:667:3: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
ChangeLog:
* microblaze-tdep.c (microblaze_dwarf2_reg_to_regnum): Check whether
less tha zero in conditional expression.
Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
gdb/microblaze-tdep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
index 7e89241..9bec260 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -664,7 +664,7 @@ static int dwarf2_to_reg_map[78] =
static int
microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
{
- gdb_assert (reg < sizeof (dwarf2_to_reg_map));
+ gdb_assert ((size_t) reg < sizeof (dwarf2_to_reg_map));
return dwarf2_to_reg_map[reg];
}
--
1.7.11.7
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression
2014-07-20 14:03 [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression Chen Gang
@ 2014-07-23 20:04 ` Michael Eager
2014-07-23 22:06 ` Chen Gang
0 siblings, 1 reply; 10+ messages in thread
From: Michael Eager @ 2014-07-23 20:04 UTC (permalink / raw)
To: Chen Gang, eager; +Cc: binutils, gdb-patches
On 07/20/14 07:03, Chen Gang wrote:
> Use typecast 'size_t' on 'reg', not only avoid the related warning, but
> also check whether less than zero -- for 'reg' is type 'int', and sizeof
> (dwarf2_to_reg_map) is less than 0x7fff.
>
> It is quoted in gdb_assert(), so need check 'reg' whether less than zero.
> And the related warning (with '-W'):
>
> ../../binutils-gdb/gdb/microblaze-tdep.c:667:3: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>
> ChangeLog:
>
> * microblaze-tdep.c (microblaze_dwarf2_reg_to_regnum): Check whether
> less tha zero in conditional expression.
>
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> gdb/microblaze-tdep.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
> index 7e89241..9bec260 100644
> --- a/gdb/microblaze-tdep.c
> +++ b/gdb/microblaze-tdep.c
> @@ -664,7 +664,7 @@ static int dwarf2_to_reg_map[78] =
> static int
> microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
> {
> - gdb_assert (reg < sizeof (dwarf2_to_reg_map));
> + gdb_assert ((size_t) reg < sizeof (dwarf2_to_reg_map));
> return dwarf2_to_reg_map[reg];
> }
I don't see anything in the patch which does what you describe,
checking whether reg is less than zero. Converting a signed
integer to an unsigned integer is not a way to check whether
it is less than zero. This is better:
+ gdb_assert (reg >= 0 && (size_t) reg < sizeof (dwarf2_to_reg_map));
I do not see this error message in my build which uses -Wall.
What is your build environment? Which version of GCC?
--
Michael Eager eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression
2014-07-23 20:04 ` Michael Eager
@ 2014-07-23 22:06 ` Chen Gang
2014-07-23 22:17 ` Michael Eager
2014-07-31 18:55 ` Chen Gang
0 siblings, 2 replies; 10+ messages in thread
From: Chen Gang @ 2014-07-23 22:06 UTC (permalink / raw)
To: Michael Eager; +Cc: eager, binutils, gdb-patches
On 07/24/2014 04:04 AM, Michael Eager wrote:
> On 07/20/14 07:03, Chen Gang wrote:
>> Use typecast 'size_t' on 'reg', not only avoid the related warning, but
>> also check whether less than zero -- for 'reg' is type 'int', and sizeof
>> (dwarf2_to_reg_map) is less than 0x7fff.
>>
>> It is quoted in gdb_assert(), so need check 'reg' whether less than zero.
>> And the related warning (with '-W'):
>>
>> ../../binutils-gdb/gdb/microblaze-tdep.c:667:3: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>>
>> ChangeLog:
>>
>> * microblaze-tdep.c (microblaze_dwarf2_reg_to_regnum): Check whether
>> less tha zero in conditional expression.
>>
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>> gdb/microblaze-tdep.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
>> index 7e89241..9bec260 100644
>> --- a/gdb/microblaze-tdep.c
>> +++ b/gdb/microblaze-tdep.c
>> @@ -664,7 +664,7 @@ static int dwarf2_to_reg_map[78] =
>> static int
>> microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
>> {
>> - gdb_assert (reg < sizeof (dwarf2_to_reg_map));
>> + gdb_assert ((size_t) reg < sizeof (dwarf2_to_reg_map));
>> return dwarf2_to_reg_map[reg];
>> }
>
> I don't see anything in the patch which does what you describe,
> checking whether reg is less than zero. Converting a signed
> integer to an unsigned integer is not a way to check whether
> it is less than zero. This is better:
>
> + gdb_assert (reg >= 0 && (size_t) reg < sizeof (dwarf2_to_reg_map));
>
Yeah, it is common statement. It is also OK to me, although after type
cast, 'reg >=0' can be omited (it can let code simpler, but let code
not quit easy understanding).
> I do not see this error message in my build which uses -Wall.
> What is your build environment? Which version of GCC?
>
Originally, I use "CFLAGS=-W -g -O2" pass to configure. And '-W' is
different from '-Wall'. '-W' reports real 'all' warnings which can be
found by compiler, but '-Wall' reports most valuable warnings.
Thanks.
--
Chen Gang
Open share and attitude like air water and life which God blessed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression
2014-07-23 22:06 ` Chen Gang
@ 2014-07-23 22:17 ` Michael Eager
2014-07-23 22:20 ` Chen Gang
2014-07-31 18:55 ` Chen Gang
1 sibling, 1 reply; 10+ messages in thread
From: Michael Eager @ 2014-07-23 22:17 UTC (permalink / raw)
To: Chen Gang, Michael Eager; +Cc: binutils, gdb-patches
On 07/23/14 15:06, Chen Gang wrote:
> On 07/24/2014 04:04 AM, Michael Eager wrote:
>> On 07/20/14 07:03, Chen Gang wrote:
>>> Use typecast 'size_t' on 'reg', not only avoid the related warning, but
>>> also check whether less than zero -- for 'reg' is type 'int', and sizeof
>>> (dwarf2_to_reg_map) is less than 0x7fff.
>>>
>>> It is quoted in gdb_assert(), so need check 'reg' whether less than zero.
>>> And the related warning (with '-W'):
>>>
>>> ../../binutils-gdb/gdb/microblaze-tdep.c:667:3: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>>>
>>> ChangeLog:
>>>
>>> * microblaze-tdep.c (microblaze_dwarf2_reg_to_regnum): Check whether
>>> less tha zero in conditional expression.
>>>
>>>
>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>> ---
>>> gdb/microblaze-tdep.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
>>> index 7e89241..9bec260 100644
>>> --- a/gdb/microblaze-tdep.c
>>> +++ b/gdb/microblaze-tdep.c
>>> @@ -664,7 +664,7 @@ static int dwarf2_to_reg_map[78] =
>>> static int
>>> microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
>>> {
>>> - gdb_assert (reg < sizeof (dwarf2_to_reg_map));
>>> + gdb_assert ((size_t) reg < sizeof (dwarf2_to_reg_map));
>>> return dwarf2_to_reg_map[reg];
>>> }
>>
>> I don't see anything in the patch which does what you describe,
>> checking whether reg is less than zero. Converting a signed
>> integer to an unsigned integer is not a way to check whether
>> it is less than zero. This is better:
>>
>> + gdb_assert (reg >= 0 && (size_t) reg < sizeof (dwarf2_to_reg_map));
>>
>
> Yeah, it is common statement. It is also OK to me, although after type
> cast, 'reg >=0' can be omited (it can let code simpler, but let code
> not quit easy understanding).
No, if you want to verify that the value is greater than zero,
this cannot be omitted. A negative value would converted to
a positive value by the cast. There no reason to believe that
this would cause the other half of the test to fail.
>> I do not see this error message in my build which uses -Wall.
>> What is your build environment? Which version of GCC?
>>
>
> Originally, I use "CFLAGS=-W -g -O2" pass to configure. And '-W' is
> different from '-Wall'. '-W' reports real 'all' warnings which can be
> found by compiler, but '-Wall' reports most valuable warnings.
OK
--
Michael Eager eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression
2014-07-23 22:17 ` Michael Eager
@ 2014-07-23 22:20 ` Chen Gang
2014-07-24 1:58 ` Chen Gang
0 siblings, 1 reply; 10+ messages in thread
From: Chen Gang @ 2014-07-23 22:20 UTC (permalink / raw)
To: Michael Eager; +Cc: Michael Eager, binutils, gdb-patches
On 07/24/2014 06:17 AM, Michael Eager wrote:
> On 07/23/14 15:06, Chen Gang wrote:
>> On 07/24/2014 04:04 AM, Michael Eager wrote:
>>> On 07/20/14 07:03, Chen Gang wrote:
>>>> Use typecast 'size_t' on 'reg', not only avoid the related warning, but
>>>> also check whether less than zero -- for 'reg' is type 'int', and sizeof
>>>> (dwarf2_to_reg_map) is less than 0x7fff.
>>>>
>>>> It is quoted in gdb_assert(), so need check 'reg' whether less than zero.
>>>> And the related warning (with '-W'):
>>>>
>>>> ../../binutils-gdb/gdb/microblaze-tdep.c:667:3: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>>>>
>>>> ChangeLog:
>>>>
>>>> * microblaze-tdep.c (microblaze_dwarf2_reg_to_regnum): Check whether
>>>> less tha zero in conditional expression.
>>>>
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>> ---
>>>> gdb/microblaze-tdep.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
>>>> index 7e89241..9bec260 100644
>>>> --- a/gdb/microblaze-tdep.c
>>>> +++ b/gdb/microblaze-tdep.c
>>>> @@ -664,7 +664,7 @@ static int dwarf2_to_reg_map[78] =
>>>> static int
>>>> microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
>>>> {
>>>> - gdb_assert (reg < sizeof (dwarf2_to_reg_map));
>>>> + gdb_assert ((size_t) reg < sizeof (dwarf2_to_reg_map));
>>>> return dwarf2_to_reg_map[reg];
>>>> }
>>>
>>> I don't see anything in the patch which does what you describe,
>>> checking whether reg is less than zero. Converting a signed
>>> integer to an unsigned integer is not a way to check whether
>>> it is less than zero. This is better:
>>>
>>> + gdb_assert (reg >= 0 && (size_t) reg < sizeof (dwarf2_to_reg_map));
>>>
>>
>> Yeah, it is common statement. It is also OK to me, although after type
>> cast, 'reg >=0' can be omited (it can let code simpler, but let code
>> not quit easy understanding).
>
> No, if you want to verify that the value is greater than zero,
> this cannot be omitted. A negative value would converted to
> a positive value by the cast. There no reason to believe that
> this would cause the other half of the test to fail.
>
When an 'int' negative value converted to a positive value, it will be
larger than 0x7fff which must be larget than 'sizeof (dwarf2_to_reg_map)'.
Thanks.
--
Chen Gang
Open share and attitude like air water and life which God blessed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression
2014-07-23 22:20 ` Chen Gang
@ 2014-07-24 1:58 ` Chen Gang
2014-07-24 2:24 ` Michael Eager
0 siblings, 1 reply; 10+ messages in thread
From: Chen Gang @ 2014-07-24 1:58 UTC (permalink / raw)
To: Michael Eager; +Cc: Michael Eager, binutils, gdb-patches
On 07/24/2014 06:20 AM, Chen Gang wrote:
> On 07/24/2014 06:17 AM, Michael Eager wrote:
>> On 07/23/14 15:06, Chen Gang wrote:
>>> On 07/24/2014 04:04 AM, Michael Eager wrote:
>>>> On 07/20/14 07:03, Chen Gang wrote:
>>>>> Use typecast 'size_t' on 'reg', not only avoid the related warning, but
>>>>> also check whether less than zero -- for 'reg' is type 'int', and sizeof
>>>>> (dwarf2_to_reg_map) is less than 0x7fff.
>>>>>
>>>>> It is quoted in gdb_assert(), so need check 'reg' whether less than zero.
>>>>> And the related warning (with '-W'):
>>>>>
>>>>> ../../binutils-gdb/gdb/microblaze-tdep.c:667:3: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>>>>>
>>>>> ChangeLog:
>>>>>
>>>>> * microblaze-tdep.c (microblaze_dwarf2_reg_to_regnum): Check whether
>>>>> less tha zero in conditional expression.
>>>>>
>>>>>
>>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>>> ---
>>>>> gdb/microblaze-tdep.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
>>>>> index 7e89241..9bec260 100644
>>>>> --- a/gdb/microblaze-tdep.c
>>>>> +++ b/gdb/microblaze-tdep.c
>>>>> @@ -664,7 +664,7 @@ static int dwarf2_to_reg_map[78] =
>>>>> static int
>>>>> microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
>>>>> {
>>>>> - gdb_assert (reg < sizeof (dwarf2_to_reg_map));
>>>>> + gdb_assert ((size_t) reg < sizeof (dwarf2_to_reg_map));
>>>>> return dwarf2_to_reg_map[reg];
>>>>> }
>>>>
>>>> I don't see anything in the patch which does what you describe,
>>>> checking whether reg is less than zero. Converting a signed
>>>> integer to an unsigned integer is not a way to check whether
>>>> it is less than zero. This is better:
>>>>
>>>> + gdb_assert (reg >= 0 && (size_t) reg < sizeof (dwarf2_to_reg_map));
>>>>
>>>
>>> Yeah, it is common statement. It is also OK to me, although after type
>>> cast, 'reg >=0' can be omited (it can let code simpler, but let code
>>> not quit easy understanding).
>>
>> No, if you want to verify that the value is greater than zero,
>> this cannot be omitted. A negative value would converted to
>> a positive value by the cast. There no reason to believe that
>> this would cause the other half of the test to fail.
>>
>
> When an 'int' negative value converted to a positive value, it will be
> larger than 0x7fff which must be larget than 'sizeof (dwarf2_to_reg_map)'.
>
If what I said is correct, your idea/suggestions is still OK to me: easy
understanding has higher priority than keeping source code simple.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression
2014-07-24 1:58 ` Chen Gang
@ 2014-07-24 2:24 ` Michael Eager
2014-07-24 2:39 ` Chen Gang
0 siblings, 1 reply; 10+ messages in thread
From: Michael Eager @ 2014-07-24 2:24 UTC (permalink / raw)
To: Chen Gang, Michael Eager; +Cc: binutils, gdb-patches
On 07/23/14 18:58, Chen Gang wrote:
>
>
> On 07/24/2014 06:20 AM, Chen Gang wrote:
>> On 07/24/2014 06:17 AM, Michael Eager wrote:
>>> On 07/23/14 15:06, Chen Gang wrote:
>>>> On 07/24/2014 04:04 AM, Michael Eager wrote:
>>>>> On 07/20/14 07:03, Chen Gang wrote:
>>>>>> Use typecast 'size_t' on 'reg', not only avoid the related warning, but
>>>>>> also check whether less than zero -- for 'reg' is type 'int', and sizeof
>>>>>> (dwarf2_to_reg_map) is less than 0x7fff.
>>>>>>
>>>>>> It is quoted in gdb_assert(), so need check 'reg' whether less than zero.
>>>>>> And the related warning (with '-W'):
>>>>>>
>>>>>> ../../binutils-gdb/gdb/microblaze-tdep.c:667:3: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>>>>>>
>>>>>> ChangeLog:
>>>>>>
>>>>>> * microblaze-tdep.c (microblaze_dwarf2_reg_to_regnum): Check whether
>>>>>> less tha zero in conditional expression.
>>>>>>
>>>>>>
>>>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>>>> ---
>>>>>> gdb/microblaze-tdep.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
>>>>>> index 7e89241..9bec260 100644
>>>>>> --- a/gdb/microblaze-tdep.c
>>>>>> +++ b/gdb/microblaze-tdep.c
>>>>>> @@ -664,7 +664,7 @@ static int dwarf2_to_reg_map[78] =
>>>>>> static int
>>>>>> microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
>>>>>> {
>>>>>> - gdb_assert (reg < sizeof (dwarf2_to_reg_map));
>>>>>> + gdb_assert ((size_t) reg < sizeof (dwarf2_to_reg_map));
>>>>>> return dwarf2_to_reg_map[reg];
>>>>>> }
>>>>>
>>>>> I don't see anything in the patch which does what you describe,
>>>>> checking whether reg is less than zero. Converting a signed
>>>>> integer to an unsigned integer is not a way to check whether
>>>>> it is less than zero. This is better:
>>>>>
>>>>> + gdb_assert (reg >= 0 && (size_t) reg < sizeof (dwarf2_to_reg_map));
>>>>>
>>>>
>>>> Yeah, it is common statement. It is also OK to me, although after type
>>>> cast, 'reg >=0' can be omited (it can let code simpler, but let code
>>>> not quit easy understanding).
>>>
>>> No, if you want to verify that the value is greater than zero,
>>> this cannot be omitted. A negative value would converted to
>>> a positive value by the cast. There no reason to believe that
>>> this would cause the other half of the test to fail.
>>>
>>
>> When an 'int' negative value converted to a positive value, it will be
>> larger than 0x7fff which must be larget than 'sizeof (dwarf2_to_reg_map)'.
>>
>
> If what I said is correct, your idea/suggestions is still OK to me: easy
> understanding has higher priority than keeping source code simple.
Yes, you are correct. Took me a moment to think through.
I left your patch as is.
Committed a52b4d3e2.
--
Michael Eager eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression
2014-07-24 2:24 ` Michael Eager
@ 2014-07-24 2:39 ` Chen Gang
0 siblings, 0 replies; 10+ messages in thread
From: Chen Gang @ 2014-07-24 2:39 UTC (permalink / raw)
To: Michael Eager, Michael Eager; +Cc: binutils, gdb-patches
On 07/24/2014 10:24 AM, Michael Eager wrote:
> On 07/23/14 18:58, Chen Gang wrote:
>>
>>
>> On 07/24/2014 06:20 AM, Chen Gang wrote:
>>> On 07/24/2014 06:17 AM, Michael Eager wrote:
>>>> On 07/23/14 15:06, Chen Gang wrote:
>>>>> On 07/24/2014 04:04 AM, Michael Eager wrote:
>>>>>> On 07/20/14 07:03, Chen Gang wrote:
>>>>>>> Use typecast 'size_t' on 'reg', not only avoid the related
>>>>>>> warning, but
>>>>>>> also check whether less than zero -- for 'reg' is type 'int', and
>>>>>>> sizeof
>>>>>>> (dwarf2_to_reg_map) is less than 0x7fff.
>>>>>>>
>>>>>>> It is quoted in gdb_assert(), so need check 'reg' whether less
>>>>>>> than zero.
>>>>>>> And the related warning (with '-W'):
>>>>>>>
>>>>>>> ../../binutils-gdb/gdb/microblaze-tdep.c:667:3: error:
>>>>>>> comparison between signed and unsigned integer expressions
>>>>>>> [-Werror=sign-compare]
>>>>>>>
>>>>>>> ChangeLog:
>>>>>>>
>>>>>>> * microblaze-tdep.c (microblaze_dwarf2_reg_to_regnum): Check
>>>>>>> whether
>>>>>>> less tha zero in conditional expression.
>>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>>>>> ---
>>>>>>> gdb/microblaze-tdep.c | 2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
>>>>>>> index 7e89241..9bec260 100644
>>>>>>> --- a/gdb/microblaze-tdep.c
>>>>>>> +++ b/gdb/microblaze-tdep.c
>>>>>>> @@ -664,7 +664,7 @@ static int dwarf2_to_reg_map[78] =
>>>>>>> static int
>>>>>>> microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int
>>>>>>> reg)
>>>>>>> {
>>>>>>> - gdb_assert (reg < sizeof (dwarf2_to_reg_map));
>>>>>>> + gdb_assert ((size_t) reg < sizeof (dwarf2_to_reg_map));
>>>>>>> return dwarf2_to_reg_map[reg];
>>>>>>> }
>>>>>>
>>>>>> I don't see anything in the patch which does what you describe,
>>>>>> checking whether reg is less than zero. Converting a signed
>>>>>> integer to an unsigned integer is not a way to check whether
>>>>>> it is less than zero. This is better:
>>>>>>
>>>>>> + gdb_assert (reg >= 0 && (size_t) reg < sizeof (dwarf2_to_reg_map));
>>>>>>
>>>>>
>>>>> Yeah, it is common statement. It is also OK to me, although after type
>>>>> cast, 'reg >=0' can be omited (it can let code simpler, but let code
>>>>> not quit easy understanding).
>>>>
>>>> No, if you want to verify that the value is greater than zero,
>>>> this cannot be omitted. A negative value would converted to
>>>> a positive value by the cast. There no reason to believe that
>>>> this would cause the other half of the test to fail.
>>>>
>>>
>>> When an 'int' negative value converted to a positive value, it will be
>>> larger than 0x7fff which must be larget than 'sizeof
>>> (dwarf2_to_reg_map)'.
>>>
>>
>> If what I said is correct, your idea/suggestions is still OK to me: easy
>> understanding has higher priority than keeping source code simple.
>
> Yes, you are correct. Took me a moment to think through.
>
OK, thank you for your work.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression
2014-07-23 22:06 ` Chen Gang
2014-07-23 22:17 ` Michael Eager
@ 2014-07-31 18:55 ` Chen Gang
2014-07-31 21:53 ` Michael Eager
1 sibling, 1 reply; 10+ messages in thread
From: Chen Gang @ 2014-07-31 18:55 UTC (permalink / raw)
To: Michael Eager; +Cc: eager, binutils, gdb-patches
On 07/24/2014 06:06 AM, Chen Gang wrote:
>
>> > I do not see this error message in my build which uses -Wall.
>> > What is your build environment? Which version of GCC?
>> >
> Originally, I use "CFLAGS=-W -g -O2" pass to configure. And '-W' is
> different from '-Wall'. '-W' reports real 'all' warnings which can be
> found by compiler, but '-Wall' reports most valuable warnings.
Sorry, after check details ("info gcc" in Chapter "Warning Options"),
What I said is incorrect. I list the related contents below (-W means
-Wextra, it can be used together with -Wall):
`-Wall'
This enables all the warnings about constructions that some users
consider questionable, and that are easy to avoid (or modify to
prevent the warning), even in conjunction with macros. This also
enables some language-specific warnings described in *note C++
Dialect Options:: and *note Objective-C and Objective-C++ Dialect
Options::.
`-Wall' turns on the following warning flags:
-Waddress
-Warray-bounds (only with `-O2')
-Wc++11-compat
-Wchar-subscripts
-Wenum-compare (in C/Objc; this is on by default in C++)
-Wimplicit-int (C and Objective-C only)
-Wimplicit-function-declaration (C and Objective-C only)
-Wcomment
-Wformat
-Wmain (only for C/ObjC and unless `-ffreestanding')
-Wmaybe-uninitialized
-Wmissing-braces
-Wnonnull
-Wparentheses
-Wpointer-sign
-Wreorder
-Wreturn-type
-Wsequence-point
-Wsign-compare (only in C++)
-Wstrict-aliasing
-Wstrict-overflow=1
-Wswitch
-Wtrigraphs
-Wuninitialized
-Wunknown-pragmas
-Wunused-function
-Wunused-label
-Wunused-value
-Wunused-variable
-Wvolatile-register-var
Note that some warning flags are not implied by `-Wall'. Some of
them warn about constructions that users generally do not consider
questionable, but which occasionally you might wish to check for;
others warn about constructions that are necessary or hard to
avoid in some cases, and there is no simple way to modify the code
to suppress the warning. Some of them are enabled by `-Wextra' but
many of them must be enabled individually.
`-Wextra'
This enables some extra warning flags that are not enabled by
`-Wall'. (This option used to be called `-W'. The older name is
still supported, but the newer name is more descriptive.)
-Wclobbered
-Wempty-body
-Wignored-qualifiers
-Wmissing-field-initializers
-Wmissing-parameter-type (C only)
-Wold-style-declaration (C only)
-Woverride-init
-Wsign-compare
-Wtype-limits
-Wuninitialized
-Wunused-parameter (only with `-Wunused' or `-Wall')
-Wunused-but-set-parameter (only with `-Wunused' or `-Wall')
The option `-Wextra' also prints warning messages for the
following cases:
* A pointer is compared against integer zero with `<', `<=',
`>', or `>='.
* (C++ only) An enumerator and a non-enumerator both appear in a
conditional expression.
* (C++ only) Ambiguous virtual bases.
* (C++ only) Subscripting an array that has been declared
`register'.
* (C++ only) Taking the address of a variable that has been
declared `register'.
* (C++ only) A base class is not initialized in a derived
class' copy constructor.
Thanks.
--
Chen Gang
Open share and attitude like air water and life which God blessed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression
2014-07-31 18:55 ` Chen Gang
@ 2014-07-31 21:53 ` Michael Eager
0 siblings, 0 replies; 10+ messages in thread
From: Michael Eager @ 2014-07-31 21:53 UTC (permalink / raw)
To: Chen Gang; +Cc: eager, binutils, gdb-patches
On 07/31/14 11:55, Chen Gang wrote:
> On 07/24/2014 06:06 AM, Chen Gang wrote:
>>
>>>> I do not see this error message in my build which uses -Wall.
>>>> What is your build environment? Which version of GCC?
>>>>
>> Originally, I use "CFLAGS=-W -g -O2" pass to configure. And '-W' is
>> different from '-Wall'. '-W' reports real 'all' warnings which can be
>> found by compiler, but '-Wall' reports most valuable warnings.
>
> Sorry, after check details ("info gcc" in Chapter "Warning Options"),
> What I said is incorrect. I list the related contents below (-W means
> -Wextra, it can be used together with -Wall):
Thanks for the update.
--
Michael Eager eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-07-31 21:53 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-20 14:03 [PATCH] gdb/microblaze-tdep.c: Check whether less than zero in conditional expression Chen Gang
2014-07-23 20:04 ` Michael Eager
2014-07-23 22:06 ` Chen Gang
2014-07-23 22:17 ` Michael Eager
2014-07-23 22:20 ` Chen Gang
2014-07-24 1:58 ` Chen Gang
2014-07-24 2:24 ` Michael Eager
2014-07-24 2:39 ` Chen Gang
2014-07-31 18:55 ` Chen Gang
2014-07-31 21:53 ` Michael Eager
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).