Re: [Libguestfs] [PATCH nbdkit 1/2] common/include: Change unique-name macros to use __COUNTER__

2022-02-23 Thread Richard W.M. Jones 
On Wed, Feb 23, 2022 at 10:28:17AM +0100, Laszlo Ersek wrote:
> On 02/23/22 00:05, Eric Blake wrote:
> > On Tue, Feb 22, 2022 at 09:35:03PM +, Richard W.M. Jones wrote:
> >> Previously the macros used __LINE__ which meant they created a unique
> >> name specific to the line on which the macro was expanded.  This
> >> worked to a limited degree for cases like:
> >>
> >>   #define FOO \
> >>  ({ int NBDKIT_UNIQUE_NAME(foo) = 42; \
> >> NBDKIT_UNIQUE_NAME(foo) * 2 })
> > 
> > Missing a ; after '* 2'.
> > 
> >>
> >> since the “FOO” macro is usually expanded at one place so both uses of
> >> NBDKIT_UNIQUE_NAME expanded to the same unique name.  It didn't work
> >> if FOO was used twice on the same line, eg:
> >>
> >>   int i = FOO * FOO;
> > 
> > This didn't actually fail. The failure we saw was more subtle:
> > 
> > MIN (MIN (1, 2), 3)
> > 
> > compiled (the inner MIN() creates an _x evaluated in the context of
> > the initializer of the outer MIN's _x, which is not in scope until the
> > initializer completes), but:
> > 
> > MIN (1, MIN (2, 3))
> > 
> > failed to compile with -Werror -Wshadow, because now the inner MIN's
> > _x is declared inside the scope of the initializer of the outer MIN's
> > _y, when an outer _x is already in scope.
> 
> I wish we could suppress -Wshadow in macro definitions, but I think
> "#pragma GCC diagnostics" may not work that way (or is not available on
> all the necessary compilers) :/

I tried that (using _Pragma) but couldn't make it work.  It hits this
nest of bugs in GCC:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78657

I think the current solution is better in the end, so all good.

Rich.

> for the series
> 
> Acked-by: Laszlo Ersek 
> 
> Laszlo
> 
> > 
> >>
> >> would fail, but this would work:
> >>
> >>   int i = FOO *
> >>   FOO;
> > 
> > Or, back to the example we actually hit,
> > 
> > MIN (1,
> >  MIN (2, 3))
> > 
> > worked.
> > 
> >>
> >> Use __COUNTER__ instead of __LINE__, but NBDKIT_UNIQUE_NAME must now
> >> be used differently.  The FOO macro above must be rewritten as:
> >>
> >>   #define FOO FOO_1(NBDKIT_UNIQUE_NAME(foo))
> >>   #define FOO_1(foo) \
> >>  ({ int foo = 42; \
> >> foo * 2 })
> >>
> >> Thanks: Eric Blake
> >> ---
> >> +++ b/common/include/test-checked-overflow.c
> >> @@ -39,29 +39,25 @@
> >>  
> >>  #define TEST_MUL(a, b, result, expected_overflow, expected_result)  \
> >>do {  \
> >> -bool NBDKIT_UNIQUE_NAME(_actual_overflow);  \
> >> +bool actual_overflow;   \
> >>  \
> >> -NBDKIT_UNIQUE_NAME(_actual_overflow) =  \
> >> -  MUL_OVERFLOW_FALLBACK ((a), (b), (result));   \
> >> -assert (NBDKIT_UNIQUE_NAME(_actual_overflow) == (expected_overflow)); 
> >> \
> >> +actual_overflow =   MUL_OVERFLOW_FALLBACK ((a), (b), (result)); \
> > 
> > Extra spacing after =
> > 
> > The commit message may need touching up, but ACK to the change itself.
> > 

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH nbdkit 1/2] common/include: Change unique-name macros to use __COUNTER__

2022-02-23 Thread Laszlo Ersek 
On 02/23/22 00:05, Eric Blake wrote:
> On Tue, Feb 22, 2022 at 09:35:03PM +, Richard W.M. Jones wrote:
>> Previously the macros used __LINE__ which meant they created a unique
>> name specific to the line on which the macro was expanded.  This
>> worked to a limited degree for cases like:
>>
>>   #define FOO \
>>  ({ int NBDKIT_UNIQUE_NAME(foo) = 42; \
>> NBDKIT_UNIQUE_NAME(foo) * 2 })
> 
> Missing a ; after '* 2'.
> 
>>
>> since the “FOO” macro is usually expanded at one place so both uses of
>> NBDKIT_UNIQUE_NAME expanded to the same unique name.  It didn't work
>> if FOO was used twice on the same line, eg:
>>
>>   int i = FOO * FOO;
> 
> This didn't actually fail. The failure we saw was more subtle:
> 
> MIN (MIN (1, 2), 3)
> 
> compiled (the inner MIN() creates an _x evaluated in the context of
> the initializer of the outer MIN's _x, which is not in scope until the
> initializer completes), but:
> 
> MIN (1, MIN (2, 3))
> 
> failed to compile with -Werror -Wshadow, because now the inner MIN's
> _x is declared inside the scope of the initializer of the outer MIN's
> _y, when an outer _x is already in scope.

I wish we could suppress -Wshadow in macro definitions, but I think
"#pragma GCC diagnostics" may not work that way (or is not available on
all the necessary compilers) :/

for the series

Acked-by: Laszlo Ersek 

Laszlo

> 
>>
>> would fail, but this would work:
>>
>>   int i = FOO *
>>   FOO;
> 
> Or, back to the example we actually hit,
> 
> MIN (1,
>  MIN (2, 3))
> 
> worked.
> 
>>
>> Use __COUNTER__ instead of __LINE__, but NBDKIT_UNIQUE_NAME must now
>> be used differently.  The FOO macro above must be rewritten as:
>>
>>   #define FOO FOO_1(NBDKIT_UNIQUE_NAME(foo))
>>   #define FOO_1(foo) \
>>  ({ int foo = 42; \
>> foo * 2 })
>>
>> Thanks: Eric Blake
>> ---
>> +++ b/common/include/test-checked-overflow.c
>> @@ -39,29 +39,25 @@
>>  
>>  #define TEST_MUL(a, b, result, expected_overflow, expected_result)  \
>>do {  \
>> -bool NBDKIT_UNIQUE_NAME(_actual_overflow);  \
>> +bool actual_overflow;   \
>>  \
>> -NBDKIT_UNIQUE_NAME(_actual_overflow) =  \
>> -  MUL_OVERFLOW_FALLBACK ((a), (b), (result));   \
>> -assert (NBDKIT_UNIQUE_NAME(_actual_overflow) == (expected_overflow)); \
>> +actual_overflow =   MUL_OVERFLOW_FALLBACK ((a), (b), (result)); \
> 
> Extra spacing after =
> 
> The commit message may need touching up, but ACK to the change itself.
> 

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH nbdkit 1/2] common/include: Change unique-name macros to use __COUNTER__

2022-02-22 Thread Eric Blake 
On Tue, Feb 22, 2022 at 09:35:03PM +, Richard W.M. Jones wrote:
> Previously the macros used __LINE__ which meant they created a unique
> name specific to the line on which the macro was expanded.  This
> worked to a limited degree for cases like:
> 
>   #define FOO \
>  ({ int NBDKIT_UNIQUE_NAME(foo) = 42; \
> NBDKIT_UNIQUE_NAME(foo) * 2 })

Missing a ; after '* 2'.

> 
> since the “FOO” macro is usually expanded at one place so both uses of
> NBDKIT_UNIQUE_NAME expanded to the same unique name.  It didn't work
> if FOO was used twice on the same line, eg:
> 
>   int i = FOO * FOO;

This didn't actually fail. The failure we saw was more subtle:

MIN (MIN (1, 2), 3)

compiled (the inner MIN() creates an _x evaluated in the context of
the initializer of the outer MIN's _x, which is not in scope until the
initializer completes), but:

MIN (1, MIN (2, 3))

failed to compile with -Werror -Wshadow, because now the inner MIN's
_x is declared inside the scope of the initializer of the outer MIN's
_y, when an outer _x is already in scope.

> 
> would fail, but this would work:
> 
>   int i = FOO *
>   FOO;

Or, back to the example we actually hit,

MIN (1,
 MIN (2, 3))

worked.

> 
> Use __COUNTER__ instead of __LINE__, but NBDKIT_UNIQUE_NAME must now
> be used differently.  The FOO macro above must be rewritten as:
> 
>   #define FOO FOO_1(NBDKIT_UNIQUE_NAME(foo))
>   #define FOO_1(foo) \
>  ({ int foo = 42; \
> foo * 2 })
> 
> Thanks: Eric Blake
> ---
> +++ b/common/include/test-checked-overflow.c
> @@ -39,29 +39,25 @@
>  
>  #define TEST_MUL(a, b, result, expected_overflow, expected_result)  \
>do {  \
> -bool NBDKIT_UNIQUE_NAME(_actual_overflow);  \
> +bool actual_overflow;   \
>  \
> -NBDKIT_UNIQUE_NAME(_actual_overflow) =  \
> -  MUL_OVERFLOW_FALLBACK ((a), (b), (result));   \
> -assert (NBDKIT_UNIQUE_NAME(_actual_overflow) == (expected_overflow)); \
> +actual_overflow =   MUL_OVERFLOW_FALLBACK ((a), (b), (result)); \

Extra spacing after =

The commit message may need touching up, but ACK to the change itself.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH nbdkit 1/2] common/include: Change unique-name macros to use __COUNTER__

2022-02-22 Thread Richard W.M. Jones 
Previously the macros used __LINE__ which meant they created a unique
name specific to the line on which the macro was expanded.  This
worked to a limited degree for cases like:

  #define FOO \
 ({ int NBDKIT_UNIQUE_NAME(foo) = 42; \
NBDKIT_UNIQUE_NAME(foo) * 2 })

since the “FOO” macro is usually expanded at one place so both uses of
NBDKIT_UNIQUE_NAME expanded to the same unique name.  It didn't work
if FOO was used twice on the same line, eg:

  int i = FOO * FOO;

would fail, but this would work:

  int i = FOO *
  FOO;

Use __COUNTER__ instead of __LINE__, but NBDKIT_UNIQUE_NAME must now
be used differently.  The FOO macro above must be rewritten as:

  #define FOO FOO_1(NBDKIT_UNIQUE_NAME(foo))
  #define FOO_1(foo) \
 ({ int foo = 42; \
foo * 2 })

Thanks: Eric Blake
---
 common/include/checked-overflow.h  | 36 --
 common/include/test-checked-overflow.c | 24 +++--
 common/include/unique-name.h   | 10 ---
 common/utils/cleanup.h | 26 ---
 4 files changed, 54 insertions(+), 42 deletions(-)

diff --git a/common/include/checked-overflow.h 
b/common/include/checked-overflow.h
index 4f81d4c1..546c930b 100644
--- a/common/include/checked-overflow.h
+++ b/common/include/checked-overflow.h
@@ -131,33 +131,41 @@
  * that the test suite can always test the fallback.
  */
 #define ADD_OVERFLOW_FALLBACK(a, b, r)\
+  ADD_OVERFLOW_FALLBACK_1 ((a), (b), (r), \
+   NBDKIT_UNIQUE_NAME (_overflow),\
+   NBDKIT_UNIQUE_NAME (_tmp))
+#define ADD_OVERFLOW_FALLBACK_1(a, b, r, overflow, tmp)   \
   ({  \
-bool NBDKIT_UNIQUE_NAME(_overflow);   \
-uintmax_t NBDKIT_UNIQUE_NAME(_tmp);   \
+bool overflow;\
+uintmax_t tmp;\
   \
 STATIC_ASSERT_UNSIGNED_INT (a);   \
 STATIC_ASSERT_UNSIGNED_INT (b);   \
 STATIC_ASSERT_UNSIGNED_INT (*(r));\
-NBDKIT_UNIQUE_NAME(_overflow) = check_add_overflow ((a), (b), \
- (typeof (*(r)))-1,   \
- _UNIQUE_NAME(_tmp)); \
-*(r) = NBDKIT_UNIQUE_NAME(_tmp);  \
-NBDKIT_UNIQUE_NAME(_overflow);\
+overflow = check_add_overflow ((a), (b),  \
+   (typeof (*(r)))-1, \
+   ); \
+*(r) = tmp;   \
+overflow; \
   })
 
 #define MUL_OVERFLOW_FALLBACK(a, b, r)\
+  MUL_OVERFLOW_FALLBACK_1 ((a), (b), (r), \
+   NBDKIT_UNIQUE_NAME (_overflow),\
+   NBDKIT_UNIQUE_NAME (_tmp))
+#define MUL_OVERFLOW_FALLBACK_1(a, b, r, overflow, tmp)   \
   ({  \
-bool NBDKIT_UNIQUE_NAME(_overflow);   \
-uintmax_t NBDKIT_UNIQUE_NAME(_tmp);   \
+bool overflow;\
+uintmax_t tmp;\
   \
 STATIC_ASSERT_UNSIGNED_INT (a);   \
 STATIC_ASSERT_UNSIGNED_INT (b);   \
 STATIC_ASSERT_UNSIGNED_INT (*(r));\
-NBDKIT_UNIQUE_NAME(_overflow) = check_mul_overflow ((a), (b), \
- (typeof (*(r)))-1,   \
- _UNIQUE_NAME(_tmp)); \
-*(r) = NBDKIT_UNIQUE_NAME(_tmp);  \
-NBDKIT_UNIQUE_NAME(_overflow);\
+overflow = check_mul_overflow ((a), (b),  \
+   (typeof (*(r)))-1, \
+   ); \
+*(r) = tmp;   \
+overflow; \
   })
 
 /* Assert, at compile time, that the expression "x" has some unsigned integer
diff --git