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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 31.08.2018 18:16, Yuri Volchkov wrote:
> 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.
Hey,

one remark regarding using UK_ASSERT() in the /include folder: Since 
UK_ASSERT is provided by libukdebug, you have to make sure that your 
header is not forcing a dependency on libukdebug. The problem is that 
everything in /include is defining the lowest-level UKARCH and UKPLAT 
API of Unikraft. It is okay that a particular platform library depends 
on another library (expressed by `select` in its Config.uk) but the API 
definitions in  /include should not by design. You would force everyone 
to compile this particular library in even for the one that provides its 
own completely independent platform library implementation.
You can have a look to /include/uk/refcount.h to see how we solved this 
chicken-egg problem with UK_ASSERT.
Thanks,

Simon

> -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
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel