Re: clang warning about mini-gmp.c when used in Emacs

[Vincent Lefevre]

On 2022-04-19 20:16:51 +0200, Niels Möller wrote:
> I've always liked gcc -Wall. I think it adheres to its design to warn
> only about things that (1) are prone to be errors, and (2) in the case
> that they aren't errors, are easy to clarify in a way that makes the
> compiler stop warning about them.
> 
> Use of -Werror, on the other hand, is something that I can't generally
> recommend. Imposing it on library users by default is certainly a bad
> idea. Using it when compiling tests that are part of configure is almost
> sure to break.
> 
> But it could make sense to enable in certain nightly builds with known
> compilers, if one has the ambition that the code should build without
> (certain) warnings in that configuration, and stay that way.

-Werror may be useful for developers in order to automatically find
potential errors in new code, but it may be necessary to disable some
errors.

I generally test MPFR with

  -Wall -Wold-style-declaration -Wold-style-definition -Wmissing-parameter-type 
-Wmissing-prototypes -Wmissing-declarations -Wmissing-field-initializers 
-Wimplicit-fallthrough -Wcast-function-type -Wcast-align=strict -Wwrite-strings 
-Wno-error=unused-function -Werror -fsanitize=undefined -fno-sanitize-recover

and

  -std=c90 -pedantic -Wall -Wold-style-declaration -Wold-style-definition 
-Wmissing-parameter-type -Wmissing-prototypes -Wmissing-declarations 
-Wmissing-field-initializers -Wimplicit-fallthrough -Wcast-function-type 
-Wcast-align=strict -Wwrite-strings -Wno-error=unused-function 
-Wno-error=overlength-strings -Werror

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: clang warning about mini-gmp.c when used in Emacs

[Marc Glisse]

On Tue, 19 Apr 2022, Niels Möller wrote:


I've always liked gcc -Wall. I think it adheres to its design to warn
only about things that (1) are prone to be errors, and (2) in the case
that they aren't errors, are easy to clarify in a way that makes the
compiler stop warning about them.


That stopped being the case some years ago, sadly. The first offender was 
-Wmaybe-uninitialized, and it was joined by others recently.



Use of -Werror, on the other hand, is something that I can't generally
recommend. Imposing it on library users by default is certainly a bad
idea. Using it when compiling tests that are part of configure is almost
sure to break.


On the other hand, at least in the past, configure scripts often contained 
C code that was much uglier than necessary. If people compiling with 
-Werror helped clean that up, I will thank them for that ;-) (without 
using -Werror myself).


--
Marc Glisse
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: clang warning about mini-gmp.c when used in Emacs

[Niels Möller]

Torbjörn Granlund  writes:

> The language accepted by -Wall -Werror ain't C.  Unfortunately, GMP and
> its configure snippets are written in C.

I've always liked gcc -Wall. I think it adheres to its design to warn
only about things that (1) are prone to be errors, and (2) in the case
that they aren't errors, are easy to clarify in a way that makes the
compiler stop warning about them.

Use of -Werror, on the other hand, is something that I can't generally
recommend. Imposing it on library users by default is certainly a bad
idea. Using it when compiling tests that are part of configure is almost
sure to break.

But it could make sense to enable in certain nightly builds with known
compilers, if one has the ambition that the code should build without
(certain) warnings in that configuration, and stay that way.

Regards,
/Niels

-- 
Niels Möller. PGP key CB4962D070D77D7FCB8BA36271D8F1FF368C6677.
Internet email is subject to wholesale government surveillance.
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: clang warning about mini-gmp.c when used in Emacs

[Torbjörn Granlund]

"Marco Bodrato"  writes:

  gmp$ ./configure CC="gcc -Wall -Werror"
  [...]
  configure: error: could not find a working compiler, see config.log for
  details

  Yes, you are right... a too fussy compiler is not considered a working
  compiler by the "configure" script :-)

The language accepted by -Wall -Werror ain't C.  Unfortunately, GMP and
its configure snippets are written in C.

-- 
Torbjörn
Please encrypt, key id 0xC8601622
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: clang warning about mini-gmp.c when used in Emacs

[Marco Bodrato]

Ciao,

Il Lun, 18 Aprile 2022 9:38 am, Paul Eggert ha scritto:
> mini-gmp.c is different from gmp-impl.h, because it's intended to be
> used by lots of downstream projects which may use compilers that
> gmp-impl.h does not use. This may help to explain why it needs casts
> that gmp-impl.h doesn't need.

gmp$ ./configure CC="gcc -Wall -Werror"
[...]
configure: error: could not find a working compiler, see config.log for
details

Yes, you are right... a too fussy compiler is not considered a working
compiler by the "configure" script :-)

Ĝis,
m

___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: clang warning about mini-gmp.c when used in Emacs

[Paul Eggert]

On 4/14/22 22:29, Niels Möller wrote:


1. Is the void cast really needed?


Yes, I think so. Various compilers complain if a statement 'E;' has an 
expression E without side effects. The standard way to pacify them is to 
cast E to void. See, for example, 
.


mini-gmp.c is different from gmp-impl.h, because it's intended to be 
used by lots of downstream projects which may use compilers that 
gmp-impl.h does not use. This may help to explain why it needs casts 
that gmp-impl.h doesn't need.




Alternative patch:

--- a/mini-gmp/mini-gmp.c   Tue Feb 15 09:18:40 2022 +0100
+++ b/mini-gmp/mini-gmp.c   Fri Apr 15 07:20:40 2022 +0200
@@ -90,6 +90,7 @@ see https://www.gnu.org/licenses/.  */
  #define gmp_assert_nocarry(x) do { \
  mp_limb_t __cy = (x);\
  assert (__cy == 0);  \
+(void) __cy;  \
} while (0)


Yes, thanks, that should work too.
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: clang warning about mini-gmp.c when used in Emacs

[Niels Möller]

Paul Eggert  writes:

> Mattias Engdegård reported that the Emacs master source currently
> generates the following warning when Emacs is built with mini-gmp.c
> under Clang 13:

Probably not that compiler specific. Minimal example:

  #include 
  void foo(void) {int x = 1; assert(x);}

No warnings with gcc -Wall -c, but warns with gcc -Wall -DNDEBUG -c.

We currently don't use -Wall in gmp or mini-gmp Makefiles. And I suspect
no nightly builds try -DNDEBUG (since main gmp code doesn't use plain
assert, it's not that an interesting configuration). To do things
specifically for mini-gmp, I guess one could tweak the check-mini-gmp
make target, or the arguments passed to it in nightly builds.

Regards,
/Niels

-- 
Niels Möller. PGP key CB4962D070D77D7FCB8BA36271D8F1FF368C6677.
Internet email is subject to wholesale government surveillance.
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: clang warning about mini-gmp.c when used in Emacs

[Niels Möller]

Vincent Lefevre  writes:

>> 1. Is the void cast really needed? Corresponding macros in gmp-impl.h
>> are defined like
>> 
>>   #if WANT_ASSERT
>>   #define ASSERT_CARRY(expr) ASSERT_ALWAYS ((expr) != 0)
>>   #define ASSERT_NOCARRY(expr)   ASSERT_ALWAYS ((expr) == 0)
>>   #else
>>   #define ASSERT_CARRY(expr) (expr)
>>   #define ASSERT_NOCARRY(expr)   (expr)
>>   #endif
>
> I'd say that a cast to void is better in order to ensure that
> ASSERT_CARRY(expr) and ASSERT_NOCARRY(expr) have type void
> whether WANT_ASSERT is defined or not, so that type issues
> could be detected more easily.

I see, it would matter for invalid use such as

mp_limb_t cy = ASSERT_NOCARRY(...);

Leaving unchanged for now.

>> Alternative patch:
>> 
>> --- a/mini-gmp/mini-gmp.cTue Feb 15 09:18:40 2022 +0100
>> +++ b/mini-gmp/mini-gmp.cFri Apr 15 07:20:40 2022 +0200
>> @@ -90,6 +90,7 @@ see https://www.gnu.org/licenses/.  */
>>  #define gmp_assert_nocarry(x) do { \
>>  mp_limb_t __cy = (x);  \
>>  assert (__cy == 0);\
>> +(void) __cy;   \
>>} while (0)
>
> I prefer this patch.

I pushed this version to the repo.

Regards,
/Niels

-- 
Niels Möller. PGP key CB4962D070D77D7FCB8BA36271D8F1FF368C6677.
Internet email is subject to wholesale government surveillance.
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: clang warning about mini-gmp.c when used in Emacs

[Vincent Lefevre]

On 2022-04-15 07:29:34 +0200, Niels Möller wrote:
> I have two minor comments on this proposed fix:
> 
> 1. Is the void cast really needed? Corresponding macros in gmp-impl.h
> are defined like
> 
>   #if WANT_ASSERT
>   #define ASSERT_CARRY(expr) ASSERT_ALWAYS ((expr) != 0)
>   #define ASSERT_NOCARRY(expr)   ASSERT_ALWAYS ((expr) == 0)
>   #else
>   #define ASSERT_CARRY(expr) (expr)
>   #define ASSERT_NOCARRY(expr)   (expr)
>   #endif

I'd say that a cast to void is better in order to ensure that
ASSERT_CARRY(expr) and ASSERT_NOCARRY(expr) have type void
whether WANT_ASSERT is defined or not, so that type issues
could be detected more easily.

> 2. I'm a bit uneasy about side effects in the argument to the standard
> assert macro, even with the NDEBUG ifdefs.

Ditto. It seems that the ISO C standard doesn't say anything if
NDEBUG not defined (the expression might be evaluated several
times, like with any macro).

> Alternative patch:
> 
> --- a/mini-gmp/mini-gmp.c Tue Feb 15 09:18:40 2022 +0100
> +++ b/mini-gmp/mini-gmp.c Fri Apr 15 07:20:40 2022 +0200
> @@ -90,6 +90,7 @@ see https://www.gnu.org/licenses/.  */
>  #define gmp_assert_nocarry(x) do { \
>  mp_limb_t __cy = (x);   \
>  assert (__cy == 0); \
> +(void) __cy;\
>} while (0)

I prefer this patch.

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: clang warning about mini-gmp.c when used in Emacs

[Niels Möller]

Paul Eggert  writes:

> Mattias Engdegård reported that the Emacs master source currently
> generates the following warning when Emacs is built with mini-gmp.c
> under Clang 13:
>
>> In file included from /Users/mattias/emacs/lib/mini-gmp-gnulib.c:47:
>> /Users/mattias/emacs/lib/mini-gmp.c:1138:2: warning: unused variable '__cy' 
>> [-Wunused-variable]
>> gmp_assert_nocarry (mpn_rshift (np, np, dn, shift));
>> ^
>> /Users/mattias/emacs/lib/mini-gmp.c:91:15: note: expanded from macro 
>> 'gmp_assert_nocarry'
>> mp_limb_t __cy = (x);  \
>
> The problem occurs because assert(X) does not evaluate X when NDEBUG
> is defined. Proposed patch attached.

Thanks for the report. I suspect builds with NDEBUG defined are not that
well tested.

> diff -r d45103d658ca mini-gmp/mini-gmp.c
> --- a/mini-gmp/mini-gmp.c Wed Mar 30 23:16:18 2022 +0200
> +++ b/mini-gmp/mini-gmp.c Fri Apr 08 16:18:06 2022 -0700
> @@ -87,10 +87,11 @@
>  #define GMP_MPN_OVERLAP_P(xp, xsize, yp, ysize)  
> \
>((xp) + (xsize) > (yp) && (yp) + (ysize) > (xp))
>  
> -#define gmp_assert_nocarry(x) do { \
> -mp_limb_t __cy = (x);   \
> -assert (__cy == 0); \
> -  } while (0)
> +#ifdef NDEBUG
> +# define gmp_assert_nocarry(x) ((void) (x))
> +#else
> +# define gmp_assert_nocarry(x) assert ((x) == 0)
> +#endif

I have two minor comments on this proposed fix:

1. Is the void cast really needed? Corresponding macros in gmp-impl.h
are defined like

  #if WANT_ASSERT
  #define ASSERT_CARRY(expr) ASSERT_ALWAYS ((expr) != 0)
  #define ASSERT_NOCARRY(expr)   ASSERT_ALWAYS ((expr) == 0)
  #else
  #define ASSERT_CARRY(expr) (expr)
  #define ASSERT_NOCARRY(expr)   (expr)
  #endif

I.e., in builds without run-time asserts, ASSERT_NO_CARRY(X) expands to
just (X).

2. I'm a bit uneasy about side effects in the argument to the standard
assert macro, even with the NDEBUG ifdefs.

Alternative patch:

--- a/mini-gmp/mini-gmp.c   Tue Feb 15 09:18:40 2022 +0100
+++ b/mini-gmp/mini-gmp.c   Fri Apr 15 07:20:40 2022 +0200
@@ -90,6 +90,7 @@ see https://www.gnu.org/licenses/.  */
 #define gmp_assert_nocarry(x) do { \
 mp_limb_t __cy = (x); \
 assert (__cy == 0);   \
+(void) __cy;  \
   } while (0)

Regards,
/Niels

-- 
Niels Möller. PGP key CB4962D070D77D7FCB8BA36271D8F1FF368C6677.
Internet email is subject to wholesale government surveillance.
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

clang warning about mini-gmp.c when used in Emacs

[Paul Eggert]

Mattias Engdegård reported that the Emacs master source currently 
generates the following warning when Emacs is built with mini-gmp.c 
under Clang 13:



In file included from /Users/mattias/emacs/lib/mini-gmp-gnulib.c:47:
/Users/mattias/emacs/lib/mini-gmp.c:1138:2: warning: unused variable '__cy' 
[-Wunused-variable]
gmp_assert_nocarry (mpn_rshift (np, np, dn, shift));
^
/Users/mattias/emacs/lib/mini-gmp.c:91:15: note: expanded from macro 
'gmp_assert_nocarry'
mp_limb_t __cy = (x);  \


The problem occurs because assert(X) does not evaluate X when NDEBUG is 
defined. Proposed patch attached.


Another advantage of this patch is that any messages generated by 
assertion failures are more informative.diff -r d45103d658ca mini-gmp/mini-gmp.c
--- a/mini-gmp/mini-gmp.c	Wed Mar 30 23:16:18 2022 +0200
+++ b/mini-gmp/mini-gmp.c	Fri Apr 08 16:18:06 2022 -0700
@@ -87,10 +87,11 @@
 #define GMP_MPN_OVERLAP_P(xp, xsize, yp, ysize)\
   ((xp) + (xsize) > (yp) && (yp) + (ysize) > (xp))
 
-#define gmp_assert_nocarry(x) do { \
-mp_limb_t __cy = (x);	   \
-assert (__cy == 0);		   \
-  } while (0)
+#ifdef NDEBUG
+# define gmp_assert_nocarry(x) ((void) (x))
+#else
+# define gmp_assert_nocarry(x) assert ((x) == 0)
+#endif
 
 #define gmp_clz(count, x) do {		\
 mp_limb_t __clz_x = (x);		\
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs