Thread: fix warnings in 9.6, 10, 11's contrib when compiling without openssl

Hello,

after plain ./configure without options (including noticeable absence of 
--with-openssl) and make,
make -C contrib COPT=-Werror gives error with gcc-11 on REL_9_6_STABLE, 
REL_10_STABLE or REL_11_STABLE.

The warning/error is about misleading indent in 
contrib/pgcrypto/imath.c's CLAMP macro:

imath.c: In function ‘mp_int_add’:
imath.c:133:1: error: this ‘while’ clause does not guard... 
[-Werror=misleading-indentation]
   133 | while(uz_ > 1 && (*dz_-- == 0)) --uz_;MP_USED(z_)=uz_;}while(0)
       | ^~~~~
imath.c:670:17: note: in expansion of macro ‘CLAMP’
   670 |                 CLAMP(c);
       |                 ^~~~~
In file included from imath.c:34:
imath.h:68:26: note: ...this statement, but the latter is misleadingly 
indented as if it were guarded by the ‘while’
    68 | #define MP_USED(Z)       ((Z)->used)
       |                          ^
imath.c:133:39: note: in expansion of macro ‘MP_USED’
   133 | while(uz_ > 1 && (*dz_-- == 0)) --uz_;MP_USED(z_)=uz_;}while(0)
       |                                       ^~~~~~~
imath.c:670:17: note: in expansion of macro ‘CLAMP’
   670 |                 CLAMP(c);
       |                 ^~~~~

pgcrypto-fix-imath-clamp-warning.patch, attached, is a suggested minimal 
fix:
diff --git a/contrib/pgcrypto/imath.c b/contrib/pgcrypto/imath.c
index b94a51b81a4..801b843cbe3 100644
--- a/contrib/pgcrypto/imath.c
+++ b/contrib/pgcrypto/imath.c
@@ -130,7 +130,8 @@ do{T *u_=(A),*v_=u_+(N)-1;while(u_<v_){T 
xch=*u_;*u_++=*v_;*v_--=xch;}}while(0)
  #else
  #define CLAMP(Z) \
  do{mp_int z_=(Z);mp_size uz_=MP_USED(z_);mp_digit 
*dz_=MP_DIGITS(z_)+uz_-1;\
-while(uz_ > 1 && (*dz_-- == 0)) --uz_;MP_USED(z_)=uz_;}while(0)
+while(uz_ > 1 && (*dz_-- == 0)) --uz_;\
+MP_USED(z_)=uz_;}while(0)
  #endif

  #undef MIN

The same patch works for all 9.6, 10 and 11. It's not needed in 12 or 
later, they compile without warnings just fine even without --with-openssl.

In addition, 9.6 (unlike 10 and 11) gives four "array-parameter=" 
warnings about contrib/pgcrypto/sha2.c:

sha2.c:552:20: error: argument 1 of type ‘uint8[]’ {aka ‘unsigned 
char[]’} with mismatched bound [-Werror=array-parameter=]
   552 | SHA256_Final(uint8 digest[], SHA256_CTX *context)
       |              ~~~~~~^~~~~~~~
In file included from sha2.c:44:
sha2.h:93:30: note: previously declared as ‘uint8[32]’ {aka ‘unsigned 
char[32]’}
    93 | void            SHA256_Final(uint8[SHA256_DIGEST_LENGTH], 
SHA256_CTX *);
       |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~

and the same for SHA{512,384,224}_Final.

pgcrypto-fix-sha2-warning.patch, attached, is a suggested minimal fix 
for that:
  void
-SHA256_Final(uint8 digest[], SHA256_CTX *context)
+SHA256_Final(uint8 digest[SHA256_DIGEST_LENGTH], SHA256_CTX *context)
  {
etc.

Please consider fixing those warnings.

-- 
Anton Voloshin
Postgres Professional, The Russian Postgres Company
https://postgrespro.ru

> On 10 Nov 2021, at 11:14, Anton Voloshin <a.voloshin@postgrespro.ru> wrote:

> The warning/error is about misleading indent in contrib/pgcrypto/imath.c's CLAMP macro:

This is upstream code in a stable branch formatted by pgindent at the time of
the release, I'm not sure the churn is worth it for fixing warnings for
non-standard compiler options here.  I don't see any of these warnings skimming
logs in the buildfarm.

> In addition, 9.6 (unlike 10 and 11) gives four "array-parameter=" warnings about contrib/pgcrypto/sha2.c:

9.6 will be EOL from the release tomorrow, no more fixes are applied to that
branch.

--
Daniel Gustafsson        https://vmware.com/

On Wed, Nov 10, 2021 at 02:56:00PM +0100, Daniel Gustafsson wrote:
> This is upstream code in a stable branch formatted by pgindent at the time of
> the release, I'm not sure the churn is worth it for fixing warnings for
> non-standard compiler options here.  I don't see any of these warnings skimming
> logs in the buildfarm.

I would suggest to send any improvements for imath directly to
upstream, then Postgres would just feed on that when we update it:
https://github.com/creachadair/imath

Peter has removed this code as of db7d1a7, but there could still be a
point in updating imath on the stable branches where it exists if
there is an issue with it impacting pgcrypto without OpenSSL.
--
Michael

On 11/11/2021 08:14, Michael Paquier wrote:
> I would suggest to send any improvements for imath directly to
> upstream, then Postgres would just feed on that when we update it:
> https://github.com/creachadair/imath

I didn't realise imath comes (occasionally) from external repo. Thank you.

The compilation warnings are only relevant for their old releases, 
before imath v1.27 where they've replaced macros with inline functions, see
https://github.com/creachadair/imath/commit/d4760eef8

So imath in upstream doesn't require updates (especially since those 
warnings come specifically from pgindent's work, not from the upstream 
source). But Postgres' 10_STABLE and 11_STABLE still use old version 
with CLAMP as a macro.

Noah Misch was last to update imath from upstream in February 2019 and 
that change became part of REL_12_STABLE and later:
 > Import changes from IMath versions (1.3, 1.29].
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=48e24ba6b7fd3bfd156b51e8d768fd48df0d288b

Noah and others, do you think it would be a good idea to update imath in 
REL_10_STABLE and REL_11_STABLE to a later upstream version to avoid 
those warnings about misleading indent? (see beginning of this thread) 
Admittedly, those warnings can only be seen in unusual circumstances (no 
openssl), but still, as you have mentioned in your commit message,
 > We're better off naively tracking upstream than reactively
 > maintaining a twelve-year-old snapshot of upstream.

Perhaps even update imath in all relevant stable branches, 10..14 
inclusive, to the same upstream version?

On 11/11/2021 08:14, Michael Paquier wrote:
> Peter has removed this code as of db7d1a7, but there could still be a
> point in updating imath on the stable branches where it exists if
> there is an issue with it impacting pgcrypto without OpenSSL.

Perhaps that would be a good idea, see above.

-- 
Anton Voloshin
https://postgrespro.ru
Postgres Professional, The Russian Postgres Company