Re: Warning in the RecordTransactionAbort routine during compilation with O3 flag

09.12.2019 13:03, Michael Paquier пишет:
> On Mon, Dec 09, 2019 at 08:49:26AM +0500, Andrey Lepikhov wrote:
>> xact.c: In function ‘RecordTransactionAbort’:
>> xact.c:5709:55: warning: argument 1 null where non-null expected [-Wnonnull]
>>      XLogRegisterData(unconstify(char *, twophase_gid), strlen(twophase_gid)
>> + 1);
> 
>      Assert(twophase_gid != NULL);
> -
> -   if (XLogLogicalInfoActive())
> -       xl_xinfo.xinfo |= XACT_XINFO_HAS_GID;
> 	
> xlinfo is set in the first part logging the transaction commit and the
> record data is registered in the second, so I think that the original
> coding makes more sense than what you are suggesting.  Perhaps it
> would help to just add an assertion on twophase_gid to make sure that
> it is not NULL in the part registering the data?  After that we really
> have no bugs here, so it does not really help much..

We already have assertion on the twophase_gid variable. But compiler is 
not so smart and  can't find a link between the XACT_XINFO_HAS_GID flag 
and state of twophase_gid pointer.

> 
>> formatting.c: In function ‘parse_datetime’:
>> formatting.c:4229:13: warning: ‘flags’ may be used uninitialized in this
>> function [-Wmaybe-uninitialized]
>>     if (flags & DCH_ZONED)
> 
> -   uint32      flags;
> +   uint32      flags = 0;
> 
>      do_to_timestamp(date_txt, fmt, strict, &tm, &fsec, &fprec, &flags, have_error);
> 
> For this one, OK.  Wouldn't it be better to initialize flags, fprec
> and have_error directly in do_to_timestamp if they are not NULL?  This
> way future callers of the routine, if any, won't miss the
> initialization.

Ok. In accordance with your review, I have prepared a new version of the 
patch.

> 
> By the way, are you using more specific CFLAGS to see that?  With -O3
> and -Wnonnull I cannot spot both issues with GCC 9.2.1.

My compilation procedure:

export CFLAGS="-O3"
./configure --prefix=`pwd`/tmp_install --enable-tap-tests 
--enable-depend && make clean
make > /dev/null
make install > /dev/null

My compiler:

gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/7/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 
7.4.0-1ubuntu1~18.04.1' 
--with-bugurl=file:///usr/share/doc/gcc-7/README.Bugs 
--enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++ 
--prefix=/usr --with-gcc-major-version-only --program-suffix=-7 
--program-prefix=x86_64-linux-gnu- --enable-shared 
--enable-linker-build-id --libexecdir=/usr/lib 
--without-included-gettext --enable-threads=posix --libdir=/usr/lib 
--enable-nls --with-sysroot=/ --enable-clocale=gnu 
--enable-libstdcxx-debug --enable-libstdcxx-time=yes 
--with-default-libstdcxx-abi=new --enable-gnu-unique-object 
--disable-vtable-verify --enable-libmpx --enable-plugin 
--enable-default-pie --with-system-zlib --with-target-system-zlib 
--enable-objc-gc=auto --enable-multiarch --disable-werror 
--with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 
--enable-multilib --with-tune=generic 
--enable-offload-targets=nvptx-none --without-cuda-driver 
--enable-checking=release --build=x86_64-linux-gnu 
--host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 7.4.0 (Ubuntu 7.4.0-1ubuntu1~18.04.1)

-- 
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company

From 7d9ad2555a8cacc73b49f1d8bf16dcb2590ad10c Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <[email protected]>
Date: Mon, 9 Dec 2019 13:59:39 +0500
Subject: [PATCH] Make compiler quiet

---
 src/backend/access/transam/xact.c  | 2 +-
 src/backend/utils/adt/formatting.c | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5353b6ab0b..9511218e17 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5705,7 +5705,7 @@ XactLogAbortRecord(TimestampTz abort_time,
 	if (xl_xinfo.xinfo & XACT_XINFO_HAS_TWOPHASE)
 	{
 		XLogRegisterData((char *) (&xl_twophase), sizeof(xl_xact_twophase));
-		if (xl_xinfo.xinfo & XACT_XINFO_HAS_GID)
+		if (twophase_gid != NULL && xl_xinfo.xinfo & XACT_XINFO_HAS_GID)
 			XLogRegisterData(unconstify(char *, twophase_gid), strlen(twophase_gid) + 1);
 	}
 
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index f7175df8da..b4fb6bb59f 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -4327,6 +4327,10 @@ do_to_timestamp(text *date_txt, text *fmt, bool std,
 
 	fmt_len = VARSIZE_ANY_EXHDR(fmt);
 
+	/* Make compiler quiet */
+	if (flags)
+		*flags = 0;
+
 	if (fmt_len)
 	{
 		char	   *fmt_str;
-- 
2.17.1