Re: [Minios-devel] [UNIKRAFT PATCH v2 1/9] include/uk/arch: Add ukarch_ffs, ukarch_fls, ukarch_flsl functions for x86_64

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Hi,

I am ok with adding assert. I don't see it absolutely necessary, since
it is a lower level function. And normally would not be called directly.

-Yuri.

Florian Schmidt <Florian.Schmidt@xxxxxxxxx> writes:

> Hi,
>
> one drive-by comment about something I noticed while looking for 
> something else:
>
> On 08/27/2018 10:39 PM, Yuri Volchkov wrote:
>> From: Costin Lupu <costin.lupu@xxxxxxxxx>
>> 
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> Reviewed-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
>> ---
>>   include/uk/arch/x86_64/atomic.h | 46 +++++++++++++++++++++++++++++++--
>>   1 file changed, 44 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/uk/arch/x86_64/atomic.h 
>> b/include/uk/arch/x86_64/atomic.h
>> index c5f30cc..c48d5bd 100644
>> --- a/include/uk/arch/x86_64/atomic.h
>> +++ b/include/uk/arch/x86_64/atomic.h
>> @@ -30,6 +30,34 @@
>>   #error Do not include this header directly
>>   #endif
>>   
>> +/**
>> + * ukarch_ffs - find first (lowest) set bit in word.
>> + * @word: The word to search
>> + *
>> + * Undefined if no bit exists, so code should check against 0 first.
>> + */
>
> On this and the other functions, I'm not super happy with the fact that 
> we get an undefined behavior on 0. Should we maybe do a check before the 
> asm part? "If (!word) return 0;" or such? Or, at least add an 
> ASSERT(!word), so that this can be caught in debug builds?
>
> Cheers,
> Florian

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel