eal, power: don't use '-' sign with unsigned literals
Checks
Commit Message
use ~0ULL instead of -1ULL to avoid contridctory application of '-' sign
to integer literal where the desired type is unsigned.
Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
lib/librte_eal/common/eal_common_fbarray.c | 12 ++++++------
lib/librte_power/rte_power_pmd_mgmt.c | 2 +-
2 files changed, 7 insertions(+), 7 deletions(-)
Comments
On 09-Mar-21 11:44 PM, Tyler Retzlaff wrote:
> use ~0ULL instead of -1ULL to avoid contridctory application of '-' sign
> to integer literal where the desired type is unsigned.
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
Not sure i agree. It's a very common pattern and is widely used and
understood. I mean, if anything, seeing `~0` would have me stop and
think as i've literally never seen such code before.
On Fri, Mar 12, 2021 at 12:51:46PM +0000, Burakov, Anatoly wrote:
> On 09-Mar-21 11:44 PM, Tyler Retzlaff wrote:
> >use ~0ULL instead of -1ULL to avoid contridctory application of '-' sign
> >to integer literal where the desired type is unsigned.
> >
> >Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >---
>
> Not sure i agree. It's a very common pattern and is widely used and
> understood. I mean, if anything, seeing `~0` would have me stop and
> think as i've literally never seen such code before.
it produces warnings under some compilers. in some enterprises we are
required to fix certain classes of warnings (not suppress them from the
command line) as a function of security policies.
as an alternative would you be more willing to accept something like the
following? ``(unsigned long long)-1LL'' if you don't like ``~0ULL'' it
would make explicit what the compiler is already doing.
the issue is the application of the sign to what is clearly something not
signed; it get's flagged. so the cast is an explicit expression of intent
that will not generate the warnings.
appreciate you're help in finding a solution even if it isn't the
proposed solution.
thanks!
>
> --
> Thanks,
> Anatoly
On Fri, Mar 12, 2021 at 09:05:58AM -0800, Tyler Retzlaff wrote:
> On Fri, Mar 12, 2021 at 12:51:46PM +0000, Burakov, Anatoly wrote:
> > On 09-Mar-21 11:44 PM, Tyler Retzlaff wrote:
> > >use ~0ULL instead of -1ULL to avoid contridctory application of '-' sign
> > >to integer literal where the desired type is unsigned.
> > >
> > >Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > >---
> >
> > Not sure i agree. It's a very common pattern and is widely used and
> > understood. I mean, if anything, seeing `~0` would have me stop and
> > think as i've literally never seen such code before.
>
> it produces warnings under some compilers. in some enterprises we are
> required to fix certain classes of warnings (not suppress them from the
> command line) as a function of security policies.
>
> as an alternative would you be more willing to accept something like the
> following? ``(unsigned long long)-1LL'' if you don't like ``~0ULL'' it
> would make explicit what the compiler is already doing.
>
> the issue is the application of the sign to what is clearly something not
> signed; it get's flagged. so the cast is an explicit expression of intent
> that will not generate the warnings.
>
> appreciate you're help in finding a solution even if it isn't the
> proposed solution.
>
What about using ULLONG_MAX and similar defines from limits.h?
On Fri, Mar 12, 2021 at 05:37:41PM +0000, Bruce Richardson wrote:
> On Fri, Mar 12, 2021 at 09:05:58AM -0800, Tyler Retzlaff wrote:
> > > Not sure i agree. It's a very common pattern and is widely used and
> > > understood. I mean, if anything, seeing `~0` would have me stop and
> > > think as i've literally never seen such code before.
> >
> > it produces warnings under some compilers. in some enterprises we are
> > required to fix certain classes of warnings (not suppress them from the
> > command line) as a function of security policies.
> >
> > as an alternative would you be more willing to accept something like the
> > following? ``(unsigned long long)-1LL'' if you don't like ``~0ULL'' it
> > would make explicit what the compiler is already doing.
> >
> > the issue is the application of the sign to what is clearly something not
> > signed; it get's flagged. so the cast is an explicit expression of intent
> > that will not generate the warnings.
> >
> > appreciate you're help in finding a solution even if it isn't the
> > proposed solution.
> >
> What about using ULLONG_MAX and similar defines from limits.h?
i think this would be okay even in circumstances where the code is
building masks so long as in practice it results in "all bits being
set". i'm not aware of a XXX_MAX where max isn't all bits set.. is
there?
On Fri, Mar 12, 2021 at 10:36:15AM -0800, Tyler Retzlaff wrote:
> On Fri, Mar 12, 2021 at 05:37:41PM +0000, Bruce Richardson wrote:
> > On Fri, Mar 12, 2021 at 09:05:58AM -0800, Tyler Retzlaff wrote:
> > > > Not sure i agree. It's a very common pattern and is widely used and
> > > > understood. I mean, if anything, seeing `~0` would have me stop and
> > > > think as i've literally never seen such code before.
> > >
> > > it produces warnings under some compilers. in some enterprises we are
> > > required to fix certain classes of warnings (not suppress them from the
> > > command line) as a function of security policies.
> > >
> > > as an alternative would you be more willing to accept something like the
> > > following? ``(unsigned long long)-1LL'' if you don't like ``~0ULL'' it
> > > would make explicit what the compiler is already doing.
> > >
> > > the issue is the application of the sign to what is clearly something not
> > > signed; it get's flagged. so the cast is an explicit expression of intent
> > > that will not generate the warnings.
> > >
> > > appreciate you're help in finding a solution even if it isn't the
> > > proposed solution.
> > >
> > What about using ULLONG_MAX and similar defines from limits.h?
>
> i think this would be okay even in circumstances where the code is
> building masks so long as in practice it results in "all bits being
> set". i'm not aware of a XXX_MAX where max isn't all bits set.. is
> there?
just a qualification to my previous.
specifically for the UXXX_MAX (unsigned) preprocessor definitions, we
aren't talking about signed here (or at least i wasn't).
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tyler Retzlaff
> Sent: Friday, March 12, 2021 7:40 PM
>
> On Fri, Mar 12, 2021 at 10:36:15AM -0800, Tyler Retzlaff wrote:
> > On Fri, Mar 12, 2021 at 05:37:41PM +0000, Bruce Richardson wrote:
> > > On Fri, Mar 12, 2021 at 09:05:58AM -0800, Tyler Retzlaff wrote:
> > > > > Not sure i agree. It's a very common pattern and is widely used and
> > > > > understood. I mean, if anything, seeing `~0` would have me stop and
> > > > > think as i've literally never seen such code before.
> > > >
> > > > it produces warnings under some compilers. in some enterprises we are
> > > > required to fix certain classes of warnings (not suppress them from
> the
> > > > command line) as a function of security policies.
> > > >
> > > > as an alternative would you be more willing to accept something like
> the
> > > > following? ``(unsigned long long)-1LL'' if you don't like ``~0ULL''
> it
> > > > would make explicit what the compiler is already doing.
> > > >
> > > > the issue is the application of the sign to what is clearly something
> not
> > > > signed; it get's flagged. so the cast is an explicit expression of
> intent
> > > > that will not generate the warnings.
> > > >
> > > > appreciate you're help in finding a solution even if it isn't the
> > > > proposed solution.
> > > >
> > > What about using ULLONG_MAX and similar defines from limits.h?
The type of the variable being manipulated is not unsigned long long, but uint64_t, which theoretically is not the same. Long long can be more than 64 bit, although all current implementations use 64 bit for long long.
So -1ULL was wrong from the start, and ~0ULL is similarly incorrect. The correct value would be UINT64_MAX from stdint.h.
> >
> > i think this would be okay even in circumstances where the code is
> > building masks so long as in practice it results in "all bits being
> > set". i'm not aware of a XXX_MAX where max isn't all bits set.. is
> > there?
>
> just a qualification to my previous.
>
> specifically for the UXXX_MAX (unsigned) preprocessor definitions, we
> aren't talking about signed here (or at least i wasn't).
@@ -138,7 +138,7 @@ find_next_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
*/
last = MASK_LEN_TO_IDX(arr->len);
last_mod = MASK_LEN_TO_MOD(arr->len);
- last_msk = ~(-1ULL << last_mod);
+ last_msk = ~(~0ULL << last_mod);
for (msk_idx = first; msk_idx < msk->n_masks; msk_idx++) {
uint64_t cur_msk, lookahead_msk;
@@ -398,8 +398,8 @@ find_prev_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
first_mod = MASK_LEN_TO_MOD(start);
/* we're going backwards, so mask must start from the top */
ignore_msk = first_mod == MASK_ALIGN - 1 ?
- -1ULL : /* prevent overflow */
- ~(-1ULL << (first_mod + 1));
+ ~0ULL : /* prevent overflow */
+ ~(~0ULL << (first_mod + 1));
/* go backwards, include zero */
msk_idx = first;
@@ -513,7 +513,7 @@ find_prev_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
* no runs in the space we've lookbehind-scanned
* as well, so skip that on next iteration.
*/
- ignore_msk = -1ULL << need;
+ ignore_msk = ~0ULL << need;
msk_idx = lookbehind_idx;
break;
}
@@ -560,8 +560,8 @@ find_prev(const struct rte_fbarray *arr, unsigned int start, bool used)
first_mod = MASK_LEN_TO_MOD(start);
/* we're going backwards, so mask must start from the top */
ignore_msk = first_mod == MASK_ALIGN - 1 ?
- -1ULL : /* prevent overflow */
- ~(-1ULL << (first_mod + 1));
+ ~0ULL : /* prevent overflow */
+ ~(~0ULL << (first_mod + 1));
/* go backwards, include zero */
idx = first;
@@ -111,7 +111,7 @@ clb_umwait(uint16_t port_id, uint16_t qidx, struct rte_mbuf **pkts __rte_unused,
ret = rte_eth_get_monitor_addr(port_id, qidx,
&pmc);
if (ret == 0)
- rte_power_monitor(&pmc, -1ULL);
+ rte_power_monitor(&pmc, ~0ULL);
}
q_conf->umwait_in_progress = false;