Обсуждение: Warning in the RecordTransactionAbort routine during compilation withO3 flag

Compiling of master postgres branch with CFLAGS="-O3" shows compiler 
warnings:

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);
                                                        ^~~~~~~~~~~~~~~~~~~~
In file included from ../../../../src/include/c.h:61:0,
                  from ../../../../src/include/postgres.h:46,
                  from xact.c:18:
/usr/include/string.h:384:15: note: in a call to function ‘strlen’ 
declared here
  extern size_t strlen (const char *__s)
                ^~~~~~
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)

It's not a bug. But I prepare the patch to make compiler quiet.

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

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