[dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64

[Morten Brørup]

> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tyler Retzlaff
> Sent: Friday, March 12, 2021 7:24 PM
> 
> On Fri, Mar 12, 2021 at 08:34:50AM +0100, Morten Brørup wrote:
> > CC: ABI Policy maintainers. You might have an opinion. Or not. :-)
> >
> >
> > Please also update the similar math functions in rte_common.h, so the
> return type is consistent across these functions:
> > - rte_bsf32()
> > - rte_bsf32_safe()
> > - rte_fls_u32()
> > - rte_bsf64()
> > - rte_fls_u64()
> > - rte_log2_u32()
> > - rte_log2_u64()
> 
> agreed, happy to review the whole set and deal with it all at once.

Ups. I should have omitted rte_bsf32_safe() from the list. It returns a Boolean.

> 
> >
> > They should all return either int or uint32_t.
> >
> > Standard C conventions would have them all return int (probably due to
> C's default type promotion to int when used in calculations), which is also
> the type returned by their underlying implementation.
> 
> yes, i suspect gcc builtins return int because of the default type
> promotion. probably historical be interesting to get an old gcc hand to
> tell us a story.
> 
> >
> > For some unknown reason, DPDK often uses uint32_t where you would
> normally use int. I guess it was inspired by MISRA C (for embedded
> systems); but it is not a documented conventions, and often deviated from.
> 
> horses for courses, if it doesn't make sense to interpret as signed then
> i don't see a lot of value in using signed and it can open up avenues of
> exploit.

I agree with you on this. The best return type is determined by considering how the return value is going to be used.

I could argue that these are mathematical functions, so they can be used for any kind of math, including math involving negative numbers. On the other hand, DPDK generally uses uint32_t for positive integers; and this also seems to be the original author's intention.

> 
> >
> > I don't have a personal preference for int or uint32_t here. But at least
> follow the same convention in the same header file.
> 
> agree completely, consistency.

Looking closer at it, uint32_t is probably closer to general DPDK consistency.

> 
> >
> > (Please note that the functions returning a Boolean value as an int type
> should keep doing that.)
> 
> i'm not planning on changing int -> _Bool. but i am curious about your
> comment. stdbool.h is already used in the code base is there a compiler
> in use that does not support _Bool. this is purely my interest, i don't
> propose any change.

I only mentioned this to ensure that you don't change the Boolean return values from int to uint32_t.

For arguments sate, they could be changed to bool, which is an acceptable type in DPDK, ref.:
https://elixir.bootlin.com/dpdk/latest/C/ident/bool

However, I agree with you to not propose any change here!