Warning in the RecordTransactionAbort routine during compilation with O3 flag

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..

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.

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.
--
Michael

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

On Mon, Dec 09, 2019 at 02:03:43PM +0500, Andrey Lepikhov wrote:

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.

Well, gcc-9 got visibly smarter on that point :)

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

Regarding formatting.c, I can see the point of avoiding future
mistakes, and I would go a bit further as per the attached for
consistency between all variables. have_error is a bit trickier
though as it gets moved around more layers so doing an initialization
in the middle is not really an option. Anyway, we can do that rather
cleanly from the entry point of do_to_timestamp() to bring more
consistency for variables which are always expected and the optional
ones. What do you think?

For the second one in xact.c, I am not really on board of doing
something based on the proposals because this reduces the code
visibility, and gcc is clearly wrong in its assumptions because the
state cannot be reached.

My compiler:

gcc -v
[...]
gcc version 7.4.0 (Ubuntu 7.4.0-1ubuntu1~18.04.1)

Thanks, that's the difference. gcc-9 does not complain, but I can see
the warnings with gcc-7 (7.5.0 actually).
--
xMichael

10.12.2019 08:13, Michael Paquier wrote:

On Mon, Dec 09, 2019 at 02:03:43PM +0500, Andrey Lepikhov wrote:

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.

Well, gcc-9 got visibly smarter on that point :)

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

Regarding formatting.c, I can see the point of avoiding future
mistakes, and I would go a bit further as per the attached for
consistency between all variables. have_error is a bit trickier
though as it gets moved around more layers so doing an initialization
in the middle is not really an option. Anyway, we can do that rather
cleanly from the entry point of do_to_timestamp() to bring more
consistency for variables which are always expected and the optional
ones. What do you think?

I have small experience in formatting.c code. But this patch and idea
looks good.

For the second one in xact.c, I am not really on board of doing
something based on the proposals because this reduces the code
visibility, and gcc is clearly wrong in its assumptions because the
state cannot be reached.

Ok. I switched to gcc-9 and now have no problem.

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

On Tue, Dec 10, 2019 at 12:18:49PM +0500, Andrey Lepikhov wrote:

I have small experience in formatting.c code. But this patch and idea looks
good.

Thanks, fixed this part. Please note that this originated from
66c74f8 and d589f94.
--
Michael