* Re: [PATCH] lib/idr.c: remove duplicated bound checking in sub_alloc
2015-03-20 21:28 ` [PATCH] lib/idr.c: remove duplicated bound checking in sub_alloc Andrew Morton
@ 2015-03-21 7:47 ` Liu Sha
0 siblings, 0 replies; 2+ messages in thread
From: Liu Sha @ 2015-03-21 7:47 UTC (permalink / raw)
To: Andrew Morton; +Cc: Liu Sha, linux-kernel
On Sat, Mar 21, 2015 at 5:28 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 19 Mar 2015 17:57:07 +0000 Liu Sha <rednoax@qq.com> wrote:
>
>> From: Liu Sha <rednoax@gmail.com>
>>
>> The INT_MAX bound checking in sub_alloc checks two conditions to see
>> whether the signed integer "id" is beyond INT_MAX:
>>
>> if ((id >= MAX_IDR_BIT) || (id < 0))
>> return -ENOSPC;
>>
>> These two conditions are actually the same for "int" variable so one
>> of them can be removed. If the above snippet is compiled with -Os option
>> of gcc, only one checking will remain in disassembly code.
>>
>> --- a/lib/idr.c
>> +++ b/lib/idr.c
>> @@ -262,7 +262,7 @@ static int sub_alloc(struct idr *idp, int *starting_id, struct idr_layer **pa,
>> sh = IDR_BITS*l;
>> id = ((id >> sh) ^ n ^ m) << sh;
>> }
>> - if ((id >= MAX_IDR_BIT) || (id < 0))
>> + if (id >= MAX_IDR_BIT)
>> return -ENOSPC;
>> if (l == 0)
>> break;
>
> Well. This only works because MAX_IDR_BIT happens to have unsigned
> type, so the comparison is done with unsigned arithmetic.
>
> The patch makes no difference to code size with my gcc and I'm inclined
> to leave the code as-is for reasons of safety and clarity.
Thanks for your explaination. Because address "qq.com" of this mail is
among the "not liked source for email" of LKML and has been returned. I
wrote 2nd mail with the same patch using another email address. You may
have received two similar mail. Please ignore the 2nd.
I think it may be better to shrink this checking for the following reasons :)
1. two equivalent conditions is a little confusing. When I first see it, I know
what "id < 0" means but not 100% sure about "id > MAX_IDR_BIT". "id" is
signed while "MAX_IDR_BIT" is unsigned. So after test I belive it becomes
an unsigned comparsion, as you said. And its function is equivalent to "id < 0".
I tried two c files, using "id > MAX_IDR_BIT" and "id < 0" respectively, gcc
will interpreter both as "id < 0". So just one condition will be much clear to
let people know that it only want to filter "id < 0".
2. The top Makefile specifies either "-Os" or "-O2" for gcc. So there is not
any difference in generated assembly code in idr.lst, no matter which one of
these two conditions is removed. The valid idr range is [0, INT_MAX], which
has been so since 2.6.12. There are some other places in idr.c checking INT_MAX.
They either use "> MAX_IDR_BIT" or "< 0", but not both. So only one is safe
enough.
--
Liu Sha
^ permalink raw reply [flat|nested] 2+ messages in thread