[RFC,2/7] eal: add generic bit manipulation macros

  Add bit-level test/set/clear/assign macros operating on both 32-bit
and 64-bit words by means of C11 generic selection.

Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 lib/eal/include/rte_bitops.h | 81 ++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)
  

Hi Mattias,
  I have a comment about the _Generic. What if the user gives uint8_t * or uint16_t * as the address. One improvement is that we could add a default branch in _Generic to throw a compiler error or assert false.
  Another question is what if nr >= sizeof(type) ? What if you do, for example, (uint32_t)1 << 35? Maybe we could add an assert in the implementation?

Regards,
Heng

-----Original Message-----
From: Mattias Rönnblom <mattias.ronnblom@ericsson.com> 
Sent: Saturday, March 2, 2024 2:53 PM
To: dev@dpdk.org
Cc: hofors@lysator.liu.se; Heng Wang <heng.wang@ericsson.com>; Mattias Rönnblom <mattias.ronnblom@ericsson.com>
Subject: [RFC 2/7] eal: add generic bit manipulation macros

Add bit-level test/set/clear/assign macros operating on both 32-bit and 64-bit words by means of C11 generic selection.

Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 lib/eal/include/rte_bitops.h | 81 ++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h index 9a368724d5..afd0f11033 100644
--- a/lib/eal/include/rte_bitops.h
+++ b/lib/eal/include/rte_bitops.h
@@ -107,6 +107,87 @@ extern "C" {
 #define RTE_FIELD_GET64(mask, reg) \
 		((typeof(mask))(((reg) & (mask)) >> rte_ctz64(mask)))
 
+/**
+ * Test bit in word.
+ *
+ * Generic selection macro to test the value of a bit in a 32-bit or
+ * 64-bit word. The type of operation depends on the type of the @c
+ * addr parameter.
+ *
+ * This macro does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the word to modify.
+ * @param nr
+ *   The index of the bit.
+ */
+#define rte_bit_test(addr, nr)				\
+	_Generic((addr),				\
+		 uint32_t *: rte_bit_test32,		\
+		 uint64_t *: rte_bit_test64)(addr, nr)
+
+/**
+ * Set bit in word.
+ *
+ * Generic selection macro to set a bit in a 32-bit or 64-bit
+ * word. The type of operation depends on the type of the @c addr
+ * parameter.
+ *
+ * This macro does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the word to modify.
+ * @param nr
+ *   The index of the bit.
+ */
+#define rte_bit_set(addr, nr)				\
+	_Generic((addr),				\
+		 uint32_t *: rte_bit_set32,		\
+		 uint64_t *: rte_bit_set64)(addr, nr)
+
+/**
+ * Clear bit in word.
+ *
+ * Generic selection macro to clear a bit in a 32-bit or 64-bit
+ * word. The type of operation depends on the type of the @c addr
+ * parameter.
+ *
+ * This macro does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the word to modify.
+ * @param nr
+ *   The index of the bit.
+ */
+#define rte_bit_clear(addr, nr)			\
+	_Generic((addr),				\
+		 uint32_t *: rte_bit_clear32,		\
+		 uint64_t *: rte_bit_clear64)(addr, nr)
+
+/**
+ * Assign a value to a bit in word.
+ *
+ * Generic selection macro to assign a value to a bit in a 32-bit or 
+64-bit
+ * word. The type of operation depends on the type of the @c addr parameter.
+ *
+ * This macro does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the word to modify.
+ * @param nr
+ *   The index of the bit.
+ * @param value
+ *   The new value of the bit - true for '1', or false for '0'.
+ */
+#define rte_bit_assign(addr, nr, value)			\
+	_Generic((addr),				\
+		 uint32_t *: rte_bit_assign32,			\
+		 uint64_t *: rte_bit_assign64)(addr, nr, value)
+
 /**
  * Test if a particular bit in a 32-bit word is set.
  *
--
2.34.1
  

On 2024-03-04 09:16, Heng Wang wrote:
> Hi Mattias,
>    I have a comment about the _Generic. What if the user gives uint8_t * or uint16_t * as the address. One improvement is that we could add a default branch in _Generic to throw a compiler error or assert false.

If the user pass an incompatible pointer, the compiler will generate an 
error.

>    Another question is what if nr >= sizeof(type) ? What if you do, for example, (uint32_t)1 << 35? Maybe we could add an assert in the implementation?
> 

There are already such asserts in the functions the macro delegates to.

That said, DPDK RTE_ASSERT()s are disabled even in debug builds, so I'm 
not sure it's going to help anyone.

> Regards,
> Heng
> 
> -----Original Message-----
> From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Sent: Saturday, March 2, 2024 2:53 PM
> To: dev@dpdk.org
> Cc: hofors@lysator.liu.se; Heng Wang <heng.wang@ericsson.com>; Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Subject: [RFC 2/7] eal: add generic bit manipulation macros
> 
> Add bit-level test/set/clear/assign macros operating on both 32-bit and 64-bit words by means of C11 generic selection.
> 
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> ---
>   lib/eal/include/rte_bitops.h | 81 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 81 insertions(+)
> 
> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h index 9a368724d5..afd0f11033 100644
> --- a/lib/eal/include/rte_bitops.h
> +++ b/lib/eal/include/rte_bitops.h
> @@ -107,6 +107,87 @@ extern "C" {
>   #define RTE_FIELD_GET64(mask, reg) \
>   		((typeof(mask))(((reg) & (mask)) >> rte_ctz64(mask)))
>   
> +/**
> + * Test bit in word.
> + *
> + * Generic selection macro to test the value of a bit in a 32-bit or
> + * 64-bit word. The type of operation depends on the type of the @c
> + * addr parameter.
> + *
> + * This macro does not give any guarantees in regards to memory
> + * ordering or atomicity.
> + *
> + * @param addr
> + *   A pointer to the word to modify.
> + * @param nr
> + *   The index of the bit.
> + */
> +#define rte_bit_test(addr, nr)				\
> +	_Generic((addr),				\
> +		 uint32_t *: rte_bit_test32,		\
> +		 uint64_t *: rte_bit_test64)(addr, nr)
> +
> +/**
> + * Set bit in word.
> + *
> + * Generic selection macro to set a bit in a 32-bit or 64-bit
> + * word. The type of operation depends on the type of the @c addr
> + * parameter.
> + *
> + * This macro does not give any guarantees in regards to memory
> + * ordering or atomicity.
> + *
> + * @param addr
> + *   A pointer to the word to modify.
> + * @param nr
> + *   The index of the bit.
> + */
> +#define rte_bit_set(addr, nr)				\
> +	_Generic((addr),				\
> +		 uint32_t *: rte_bit_set32,		\
> +		 uint64_t *: rte_bit_set64)(addr, nr)
> +
> +/**
> + * Clear bit in word.
> + *
> + * Generic selection macro to clear a bit in a 32-bit or 64-bit
> + * word. The type of operation depends on the type of the @c addr
> + * parameter.
> + *
> + * This macro does not give any guarantees in regards to memory
> + * ordering or atomicity.
> + *
> + * @param addr
> + *   A pointer to the word to modify.
> + * @param nr
> + *   The index of the bit.
> + */
> +#define rte_bit_clear(addr, nr)			\
> +	_Generic((addr),				\
> +		 uint32_t *: rte_bit_clear32,		\
> +		 uint64_t *: rte_bit_clear64)(addr, nr)
> +
> +/**
> + * Assign a value to a bit in word.
> + *
> + * Generic selection macro to assign a value to a bit in a 32-bit or
> +64-bit
> + * word. The type of operation depends on the type of the @c addr parameter.
> + *
> + * This macro does not give any guarantees in regards to memory
> + * ordering or atomicity.
> + *
> + * @param addr
> + *   A pointer to the word to modify.
> + * @param nr
> + *   The index of the bit.
> + * @param value
> + *   The new value of the bit - true for '1', or false for '0'.
> + */
> +#define rte_bit_assign(addr, nr, value)			\
> +	_Generic((addr),				\
> +		 uint32_t *: rte_bit_assign32,			\
> +		 uint64_t *: rte_bit_assign64)(addr, nr, value)
> +
>   /**
>    * Test if a particular bit in a 32-bit word is set.
>    *
> --
> 2.34.1
>
  

On Sat, Mar 02, 2024 at 02:53:23PM +0100, Mattias Rönnblom wrote:
> Add bit-level test/set/clear/assign macros operating on both 32-bit
> and 64-bit words by means of C11 generic selection.
> 
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> ---

_Generic is nice here. should we discourage direct use of the inline
functions in preference of using the macro always? either way lgtm.

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

>  lib/eal/include/rte_bitops.h | 81 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
> index 9a368724d5..afd0f11033 100644
> --- a/lib/eal/include/rte_bitops.h
> +++ b/lib/eal/include/rte_bitops.h
> @@ -107,6 +107,87 @@ extern "C" {
>  #define RTE_FIELD_GET64(mask, reg) \
>  		((typeof(mask))(((reg) & (mask)) >> rte_ctz64(mask)))
>  
> +/**
> + * Test bit in word.
> + *
> + * Generic selection macro to test the value of a bit in a 32-bit or
> + * 64-bit word. The type of operation depends on the type of the @c
> + * addr parameter.
> + *
> + * This macro does not give any guarantees in regards to memory
> + * ordering or atomicity.
> + *
> + * @param addr
> + *   A pointer to the word to modify.
> + * @param nr
> + *   The index of the bit.
> + */
> +#define rte_bit_test(addr, nr)				\
> +	_Generic((addr),				\
> +		 uint32_t *: rte_bit_test32,		\
> +		 uint64_t *: rte_bit_test64)(addr, nr)
> +
> +/**
> + * Set bit in word.
> + *
> + * Generic selection macro to set a bit in a 32-bit or 64-bit
> + * word. The type of operation depends on the type of the @c addr
> + * parameter.
> + *
> + * This macro does not give any guarantees in regards to memory
> + * ordering or atomicity.
> + *
> + * @param addr
> + *   A pointer to the word to modify.
> + * @param nr
> + *   The index of the bit.
> + */
> +#define rte_bit_set(addr, nr)				\
> +	_Generic((addr),				\
> +		 uint32_t *: rte_bit_set32,		\
> +		 uint64_t *: rte_bit_set64)(addr, nr)
> +
> +/**
> + * Clear bit in word.
> + *
> + * Generic selection macro to clear a bit in a 32-bit or 64-bit
> + * word. The type of operation depends on the type of the @c addr
> + * parameter.
> + *
> + * This macro does not give any guarantees in regards to memory
> + * ordering or atomicity.
> + *
> + * @param addr
> + *   A pointer to the word to modify.
> + * @param nr
> + *   The index of the bit.
> + */
> +#define rte_bit_clear(addr, nr)			\
> +	_Generic((addr),				\
> +		 uint32_t *: rte_bit_clear32,		\
> +		 uint64_t *: rte_bit_clear64)(addr, nr)
> +
> +/**
> + * Assign a value to a bit in word.
> + *
> + * Generic selection macro to assign a value to a bit in a 32-bit or 64-bit
> + * word. The type of operation depends on the type of the @c addr parameter.
> + *
> + * This macro does not give any guarantees in regards to memory
> + * ordering or atomicity.
> + *
> + * @param addr
> + *   A pointer to the word to modify.
> + * @param nr
> + *   The index of the bit.
> + * @param value
> + *   The new value of the bit - true for '1', or false for '0'.
> + */
> +#define rte_bit_assign(addr, nr, value)			\
> +	_Generic((addr),				\
> +		 uint32_t *: rte_bit_assign32,			\
> +		 uint64_t *: rte_bit_assign64)(addr, nr, value)
> +
>  /**
>   * Test if a particular bit in a 32-bit word is set.
>   *
> -- 
> 2.34.1
  

On 2024-03-04 17:42, Tyler Retzlaff wrote:
> On Sat, Mar 02, 2024 at 02:53:23PM +0100, Mattias Rönnblom wrote:
>> Add bit-level test/set/clear/assign macros operating on both 32-bit
>> and 64-bit words by means of C11 generic selection.
>>
>> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>> ---
> 
> _Generic is nice here. should we discourage direct use of the inline
> functions in preference of using the macro always? either way lgtm.
> 

That was something I considered, but decided against it for RFC v1. I 
wasn't even sure people would like _Generic.

The big upside of having only the _Generic macros would be a much 
smaller API, but maybe a tiny bit less (type-)safe to use.

Also, _Generic is new for DPDK, so who knows what issues it might cause 
with old compilers.

Thanks.

> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 
>>   lib/eal/include/rte_bitops.h | 81 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 81 insertions(+)
>>
>> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
>> index 9a368724d5..afd0f11033 100644
>> --- a/lib/eal/include/rte_bitops.h
>> +++ b/lib/eal/include/rte_bitops.h
>> @@ -107,6 +107,87 @@ extern "C" {
>>   #define RTE_FIELD_GET64(mask, reg) \
>>   		((typeof(mask))(((reg) & (mask)) >> rte_ctz64(mask)))
>>   
>> +/**
>> + * Test bit in word.
>> + *
>> + * Generic selection macro to test the value of a bit in a 32-bit or
>> + * 64-bit word. The type of operation depends on the type of the @c
>> + * addr parameter.
>> + *
>> + * This macro does not give any guarantees in regards to memory
>> + * ordering or atomicity.
>> + *
>> + * @param addr
>> + *   A pointer to the word to modify.
>> + * @param nr
>> + *   The index of the bit.
>> + */
>> +#define rte_bit_test(addr, nr)				\
>> +	_Generic((addr),				\
>> +		 uint32_t *: rte_bit_test32,		\
>> +		 uint64_t *: rte_bit_test64)(addr, nr)
>> +
>> +/**
>> + * Set bit in word.
>> + *
>> + * Generic selection macro to set a bit in a 32-bit or 64-bit
>> + * word. The type of operation depends on the type of the @c addr
>> + * parameter.
>> + *
>> + * This macro does not give any guarantees in regards to memory
>> + * ordering or atomicity.
>> + *
>> + * @param addr
>> + *   A pointer to the word to modify.
>> + * @param nr
>> + *   The index of the bit.
>> + */
>> +#define rte_bit_set(addr, nr)				\
>> +	_Generic((addr),				\
>> +		 uint32_t *: rte_bit_set32,		\
>> +		 uint64_t *: rte_bit_set64)(addr, nr)
>> +
>> +/**
>> + * Clear bit in word.
>> + *
>> + * Generic selection macro to clear a bit in a 32-bit or 64-bit
>> + * word. The type of operation depends on the type of the @c addr
>> + * parameter.
>> + *
>> + * This macro does not give any guarantees in regards to memory
>> + * ordering or atomicity.
>> + *
>> + * @param addr
>> + *   A pointer to the word to modify.
>> + * @param nr
>> + *   The index of the bit.
>> + */
>> +#define rte_bit_clear(addr, nr)			\
>> +	_Generic((addr),				\
>> +		 uint32_t *: rte_bit_clear32,		\
>> +		 uint64_t *: rte_bit_clear64)(addr, nr)
>> +
>> +/**
>> + * Assign a value to a bit in word.
>> + *
>> + * Generic selection macro to assign a value to a bit in a 32-bit or 64-bit
>> + * word. The type of operation depends on the type of the @c addr parameter.
>> + *
>> + * This macro does not give any guarantees in regards to memory
>> + * ordering or atomicity.
>> + *
>> + * @param addr
>> + *   A pointer to the word to modify.
>> + * @param nr
>> + *   The index of the bit.
>> + * @param value
>> + *   The new value of the bit - true for '1', or false for '0'.
>> + */
>> +#define rte_bit_assign(addr, nr, value)			\
>> +	_Generic((addr),				\
>> +		 uint32_t *: rte_bit_assign32,			\
>> +		 uint64_t *: rte_bit_assign64)(addr, nr, value)
>> +
>>   /**
>>    * Test if a particular bit in a 32-bit word is set.
>>    *
>> -- 
>> 2.34.1
  

On Tue, Mar 05, 2024 at 07:08:36PM +0100, Mattias Rönnblom wrote:
> On 2024-03-04 17:42, Tyler Retzlaff wrote:
> >On Sat, Mar 02, 2024 at 02:53:23PM +0100, Mattias Rönnblom wrote:
> >>Add bit-level test/set/clear/assign macros operating on both 32-bit
> >>and 64-bit words by means of C11 generic selection.
> >>
> >>Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> >>---
> >
> >_Generic is nice here. should we discourage direct use of the inline
> >functions in preference of using the macro always? either way lgtm.
> >
> 
> That was something I considered, but decided against it for RFC v1.
> I wasn't even sure people would like _Generic.
> 
> The big upside of having only the _Generic macros would be a much
> smaller API, but maybe a tiny bit less (type-)safe to use.

i'm curious what misuse pattern you anticipate or have seen that may be
less type-safe? just so i can look out for them.

i (perhaps naively) have liked generic functions for their selection of
the "correct" type and for _Generic if no leg/case exists compiler
error (as opposed to e.g. silent truncation).

> 
> Also, _Generic is new for DPDK, so who knows what issues it might
> cause with old compilers.

i was thinking about this overnight, it's supposed to be standard C11
and my use on various compilers showed no problem but I can't recall if
i did any evaluation when consuming as a part of a C++ translation unit
so there could be problems.

> 
> Thanks.
> 
> >Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >
> >>  lib/eal/include/rte_bitops.h | 81 ++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 81 insertions(+)
> >>
> >>diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
> >>index 9a368724d5..afd0f11033 100644
> >>--- a/lib/eal/include/rte_bitops.h
> >>+++ b/lib/eal/include/rte_bitops.h
> >>@@ -107,6 +107,87 @@ extern "C" {
> >>  #define RTE_FIELD_GET64(mask, reg) \
> >>  		((typeof(mask))(((reg) & (mask)) >> rte_ctz64(mask)))
> >>+/**
> >>+ * Test bit in word.
> >>+ *
> >>+ * Generic selection macro to test the value of a bit in a 32-bit or
> >>+ * 64-bit word. The type of operation depends on the type of the @c
> >>+ * addr parameter.
> >>+ *
> >>+ * This macro does not give any guarantees in regards to memory
> >>+ * ordering or atomicity.
> >>+ *
> >>+ * @param addr
> >>+ *   A pointer to the word to modify.
> >>+ * @param nr
> >>+ *   The index of the bit.
> >>+ */
> >>+#define rte_bit_test(addr, nr)				\
> >>+	_Generic((addr),				\
> >>+		 uint32_t *: rte_bit_test32,		\
> >>+		 uint64_t *: rte_bit_test64)(addr, nr)
> >>+
> >>+/**
> >>+ * Set bit in word.
> >>+ *
> >>+ * Generic selection macro to set a bit in a 32-bit or 64-bit
> >>+ * word. The type of operation depends on the type of the @c addr
> >>+ * parameter.
> >>+ *
> >>+ * This macro does not give any guarantees in regards to memory
> >>+ * ordering or atomicity.
> >>+ *
> >>+ * @param addr
> >>+ *   A pointer to the word to modify.
> >>+ * @param nr
> >>+ *   The index of the bit.
> >>+ */
> >>+#define rte_bit_set(addr, nr)				\
> >>+	_Generic((addr),				\
> >>+		 uint32_t *: rte_bit_set32,		\
> >>+		 uint64_t *: rte_bit_set64)(addr, nr)
> >>+
> >>+/**
> >>+ * Clear bit in word.
> >>+ *
> >>+ * Generic selection macro to clear a bit in a 32-bit or 64-bit
> >>+ * word. The type of operation depends on the type of the @c addr
> >>+ * parameter.
> >>+ *
> >>+ * This macro does not give any guarantees in regards to memory
> >>+ * ordering or atomicity.
> >>+ *
> >>+ * @param addr
> >>+ *   A pointer to the word to modify.
> >>+ * @param nr
> >>+ *   The index of the bit.
> >>+ */
> >>+#define rte_bit_clear(addr, nr)			\
> >>+	_Generic((addr),				\
> >>+		 uint32_t *: rte_bit_clear32,		\
> >>+		 uint64_t *: rte_bit_clear64)(addr, nr)
> >>+
> >>+/**
> >>+ * Assign a value to a bit in word.
> >>+ *
> >>+ * Generic selection macro to assign a value to a bit in a 32-bit or 64-bit
> >>+ * word. The type of operation depends on the type of the @c addr parameter.
> >>+ *
> >>+ * This macro does not give any guarantees in regards to memory
> >>+ * ordering or atomicity.
> >>+ *
> >>+ * @param addr
> >>+ *   A pointer to the word to modify.
> >>+ * @param nr
> >>+ *   The index of the bit.
> >>+ * @param value
> >>+ *   The new value of the bit - true for '1', or false for '0'.
> >>+ */
> >>+#define rte_bit_assign(addr, nr, value)			\
> >>+	_Generic((addr),				\
> >>+		 uint32_t *: rte_bit_assign32,			\
> >>+		 uint64_t *: rte_bit_assign64)(addr, nr, value)
> >>+
> >>  /**
> >>   * Test if a particular bit in a 32-bit word is set.
> >>   *
> >>-- 
> >>2.34.1
  

On 2024-03-05 19:22, Tyler Retzlaff wrote:
> On Tue, Mar 05, 2024 at 07:08:36PM +0100, Mattias Rönnblom wrote:
>> On 2024-03-04 17:42, Tyler Retzlaff wrote:
>>> On Sat, Mar 02, 2024 at 02:53:23PM +0100, Mattias Rönnblom wrote:
>>>> Add bit-level test/set/clear/assign macros operating on both 32-bit
>>>> and 64-bit words by means of C11 generic selection.
>>>>
>>>> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>>>> ---
>>>
>>> _Generic is nice here. should we discourage direct use of the inline
>>> functions in preference of using the macro always? either way lgtm.
>>>
>>
>> That was something I considered, but decided against it for RFC v1.
>> I wasn't even sure people would like _Generic.
>>
>> The big upside of having only the _Generic macros would be a much
>> smaller API, but maybe a tiny bit less (type-)safe to use.
> 
> i'm curious what misuse pattern you anticipate or have seen that may be
> less type-safe? just so i can look out for them.
> 

That was just a gut feeling, not to be taken too seriously.

uint32_t *p = some_void_pointer;
/../
rte_bit_set32(p, 17);

A code section like this is redundant in the way the type (or at least 
type size) is coded both into the function name, and the pointer type. 
The use of rte_set_bit() will eliminate this, which is good (DRY), and 
bad, because now the type isn't "double-checked".

As you can see, it's a pretty weak argument.

> i (perhaps naively) have liked generic functions for their selection of
> the "correct" type and for _Generic if no leg/case exists compiler
> error (as opposed to e.g. silent truncation).
> 
>>
>> Also, _Generic is new for DPDK, so who knows what issues it might
>> cause with old compilers.
> 
> i was thinking about this overnight, it's supposed to be standard C11
> and my use on various compilers showed no problem but I can't recall if
> i did any evaluation when consuming as a part of a C++ translation unit
> so there could be problems.
> 

It would be unfortunate if DPDK was prohibited from using _Generic.

>>
>> Thanks.
>>
>>> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>>>
>>>>   lib/eal/include/rte_bitops.h | 81 ++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 81 insertions(+)
>>>>
>>>> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
>>>> index 9a368724d5..afd0f11033 100644
>>>> --- a/lib/eal/include/rte_bitops.h
>>>> +++ b/lib/eal/include/rte_bitops.h
>>>> @@ -107,6 +107,87 @@ extern "C" {
>>>>   #define RTE_FIELD_GET64(mask, reg) \
>>>>   		((typeof(mask))(((reg) & (mask)) >> rte_ctz64(mask)))
>>>> +/**
>>>> + * Test bit in word.
>>>> + *
>>>> + * Generic selection macro to test the value of a bit in a 32-bit or
>>>> + * 64-bit word. The type of operation depends on the type of the @c
>>>> + * addr parameter.
>>>> + *
>>>> + * This macro does not give any guarantees in regards to memory
>>>> + * ordering or atomicity.
>>>> + *
>>>> + * @param addr
>>>> + *   A pointer to the word to modify.
>>>> + * @param nr
>>>> + *   The index of the bit.
>>>> + */
>>>> +#define rte_bit_test(addr, nr)				\
>>>> +	_Generic((addr),				\
>>>> +		 uint32_t *: rte_bit_test32,		\
>>>> +		 uint64_t *: rte_bit_test64)(addr, nr)
>>>> +
>>>> +/**
>>>> + * Set bit in word.
>>>> + *
>>>> + * Generic selection macro to set a bit in a 32-bit or 64-bit
>>>> + * word. The type of operation depends on the type of the @c addr
>>>> + * parameter.
>>>> + *
>>>> + * This macro does not give any guarantees in regards to memory
>>>> + * ordering or atomicity.
>>>> + *
>>>> + * @param addr
>>>> + *   A pointer to the word to modify.
>>>> + * @param nr
>>>> + *   The index of the bit.
>>>> + */
>>>> +#define rte_bit_set(addr, nr)				\
>>>> +	_Generic((addr),				\
>>>> +		 uint32_t *: rte_bit_set32,		\
>>>> +		 uint64_t *: rte_bit_set64)(addr, nr)
>>>> +
>>>> +/**
>>>> + * Clear bit in word.
>>>> + *
>>>> + * Generic selection macro to clear a bit in a 32-bit or 64-bit
>>>> + * word. The type of operation depends on the type of the @c addr
>>>> + * parameter.
>>>> + *
>>>> + * This macro does not give any guarantees in regards to memory
>>>> + * ordering or atomicity.
>>>> + *
>>>> + * @param addr
>>>> + *   A pointer to the word to modify.
>>>> + * @param nr
>>>> + *   The index of the bit.
>>>> + */
>>>> +#define rte_bit_clear(addr, nr)			\
>>>> +	_Generic((addr),				\
>>>> +		 uint32_t *: rte_bit_clear32,		\
>>>> +		 uint64_t *: rte_bit_clear64)(addr, nr)
>>>> +
>>>> +/**
>>>> + * Assign a value to a bit in word.
>>>> + *
>>>> + * Generic selection macro to assign a value to a bit in a 32-bit or 64-bit
>>>> + * word. The type of operation depends on the type of the @c addr parameter.
>>>> + *
>>>> + * This macro does not give any guarantees in regards to memory
>>>> + * ordering or atomicity.
>>>> + *
>>>> + * @param addr
>>>> + *   A pointer to the word to modify.
>>>> + * @param nr
>>>> + *   The index of the bit.
>>>> + * @param value
>>>> + *   The new value of the bit - true for '1', or false for '0'.
>>>> + */
>>>> +#define rte_bit_assign(addr, nr, value)			\
>>>> +	_Generic((addr),				\
>>>> +		 uint32_t *: rte_bit_assign32,			\
>>>> +		 uint64_t *: rte_bit_assign64)(addr, nr, value)
>>>> +
>>>>   /**
>>>>    * Test if a particular bit in a 32-bit word is set.
>>>>    *
>>>> -- 
>>>> 2.34.1
  

On Tue, Mar 05, 2024 at 09:02:34PM +0100, Mattias Rönnblom wrote:
> On 2024-03-05 19:22, Tyler Retzlaff wrote:
> >On Tue, Mar 05, 2024 at 07:08:36PM +0100, Mattias Rönnblom wrote:
> >>On 2024-03-04 17:42, Tyler Retzlaff wrote:
> >>>On Sat, Mar 02, 2024 at 02:53:23PM +0100, Mattias Rönnblom wrote:
> >>>>Add bit-level test/set/clear/assign macros operating on both 32-bit
> >>>>and 64-bit words by means of C11 generic selection.
> >>>>
> >>>>Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> >>>>---
> >>>
> >>>_Generic is nice here. should we discourage direct use of the inline
> >>>functions in preference of using the macro always? either way lgtm.
> >>>
> >>
> >>That was something I considered, but decided against it for RFC v1.
> >>I wasn't even sure people would like _Generic.
> >>
> >>The big upside of having only the _Generic macros would be a much
> >>smaller API, but maybe a tiny bit less (type-)safe to use.
> >
> >i'm curious what misuse pattern you anticipate or have seen that may be
> >less type-safe? just so i can look out for them.
> >
> 
> That was just a gut feeling, not to be taken too seriously.
> 
> uint32_t *p = some_void_pointer;
> /../
> rte_bit_set32(p, 17);
> 
> A code section like this is redundant in the way the type (or at
> least type size) is coded both into the function name, and the
> pointer type. The use of rte_set_bit() will eliminate this, which is
> good (DRY), and bad, because now the type isn't "double-checked".
> 
> As you can see, it's a pretty weak argument.
> 
> >i (perhaps naively) have liked generic functions for their selection of
> >the "correct" type and for _Generic if no leg/case exists compiler
> >error (as opposed to e.g. silent truncation).
> >
> >>
> >>Also, _Generic is new for DPDK, so who knows what issues it might
> >>cause with old compilers.
> >
> >i was thinking about this overnight, it's supposed to be standard C11
> >and my use on various compilers showed no problem but I can't recall if
> >i did any evaluation when consuming as a part of a C++ translation unit
> >so there could be problems.
> >
> 
> It would be unfortunate if DPDK was prohibited from using _Generic.

I agree, I don't think it should be prohibited. If C++ poses a problem
we can work to find solutions.

> 
> >>
> >>Thanks.
> >>
> >>>Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >>>
> >>>>  lib/eal/include/rte_bitops.h | 81 ++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 81 insertions(+)
> >>>>
> >>>>diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
> >>>>index 9a368724d5..afd0f11033 100644
> >>>>--- a/lib/eal/include/rte_bitops.h
> >>>>+++ b/lib/eal/include/rte_bitops.h
> >>>>@@ -107,6 +107,87 @@ extern "C" {
> >>>>  #define RTE_FIELD_GET64(mask, reg) \
> >>>>  		((typeof(mask))(((reg) & (mask)) >> rte_ctz64(mask)))
> >>>>+/**
> >>>>+ * Test bit in word.
> >>>>+ *
> >>>>+ * Generic selection macro to test the value of a bit in a 32-bit or
> >>>>+ * 64-bit word. The type of operation depends on the type of the @c
> >>>>+ * addr parameter.
> >>>>+ *
> >>>>+ * This macro does not give any guarantees in regards to memory
> >>>>+ * ordering or atomicity.
> >>>>+ *
> >>>>+ * @param addr
> >>>>+ *   A pointer to the word to modify.
> >>>>+ * @param nr
> >>>>+ *   The index of the bit.
> >>>>+ */
> >>>>+#define rte_bit_test(addr, nr)				\
> >>>>+	_Generic((addr),				\
> >>>>+		 uint32_t *: rte_bit_test32,		\
> >>>>+		 uint64_t *: rte_bit_test64)(addr, nr)
> >>>>+
> >>>>+/**
> >>>>+ * Set bit in word.
> >>>>+ *
> >>>>+ * Generic selection macro to set a bit in a 32-bit or 64-bit
> >>>>+ * word. The type of operation depends on the type of the @c addr
> >>>>+ * parameter.
> >>>>+ *
> >>>>+ * This macro does not give any guarantees in regards to memory
> >>>>+ * ordering or atomicity.
> >>>>+ *
> >>>>+ * @param addr
> >>>>+ *   A pointer to the word to modify.
> >>>>+ * @param nr
> >>>>+ *   The index of the bit.
> >>>>+ */
> >>>>+#define rte_bit_set(addr, nr)				\
> >>>>+	_Generic((addr),				\
> >>>>+		 uint32_t *: rte_bit_set32,		\
> >>>>+		 uint64_t *: rte_bit_set64)(addr, nr)
> >>>>+
> >>>>+/**
> >>>>+ * Clear bit in word.
> >>>>+ *
> >>>>+ * Generic selection macro to clear a bit in a 32-bit or 64-bit
> >>>>+ * word. The type of operation depends on the type of the @c addr
> >>>>+ * parameter.
> >>>>+ *
> >>>>+ * This macro does not give any guarantees in regards to memory
> >>>>+ * ordering or atomicity.
> >>>>+ *
> >>>>+ * @param addr
> >>>>+ *   A pointer to the word to modify.
> >>>>+ * @param nr
> >>>>+ *   The index of the bit.
> >>>>+ */
> >>>>+#define rte_bit_clear(addr, nr)			\
> >>>>+	_Generic((addr),				\
> >>>>+		 uint32_t *: rte_bit_clear32,		\
> >>>>+		 uint64_t *: rte_bit_clear64)(addr, nr)
> >>>>+
> >>>>+/**
> >>>>+ * Assign a value to a bit in word.
> >>>>+ *
> >>>>+ * Generic selection macro to assign a value to a bit in a 32-bit or 64-bit
> >>>>+ * word. The type of operation depends on the type of the @c addr parameter.
> >>>>+ *
> >>>>+ * This macro does not give any guarantees in regards to memory
> >>>>+ * ordering or atomicity.
> >>>>+ *
> >>>>+ * @param addr
> >>>>+ *   A pointer to the word to modify.
> >>>>+ * @param nr
> >>>>+ *   The index of the bit.
> >>>>+ * @param value
> >>>>+ *   The new value of the bit - true for '1', or false for '0'.
> >>>>+ */
> >>>>+#define rte_bit_assign(addr, nr, value)			\
> >>>>+	_Generic((addr),				\
> >>>>+		 uint32_t *: rte_bit_assign32,			\
> >>>>+		 uint64_t *: rte_bit_assign64)(addr, nr, value)
> >>>>+
> >>>>  /**
> >>>>   * Test if a particular bit in a 32-bit word is set.
> >>>>   *
> >>>>-- 
> >>>>2.34.1