Horton

Thread

Commits

  1. Suppress implicit-conversion warnings seen with newer clang versions.

  1. [PATCH] Suppress Clang 3.9 warnings

    Aleksander Alekseev <a.alekseev@postgrespro.ru> — 2017-02-20T14:12:39Z

    Hello.

I've just tried to build PostgreSQL with Clang 3.9.1 (default version
currently available in Arch Linux) and noticed that it outputs lots of
warning messages. Most of them are result of a bug in Clang itself:

```
postinit.c:846:3: note: include the header <string.h> or explicitly
provide a declaration for 'strlcpy'
```

I've reported it to Clang developers almost a year ago but apparently
no one cares. You can find all the details in a corresponding thread
[1]. Frankly I'm not sure what to do about it. 

The rest of warnings looks more like something we could easily deal with:

```
xloginsert.c:742:18: warning: implicit conversion from 'int' to 'char'
changes value from 253 to -3 [-Wconstant-conversion]
```

Patch that fixes these warnings is attached to this email.

[1] http://lists.llvm.org/pipermail/cfe-dev/2016-March/048126.html

-- 
Best regards,
Aleksander Alekseev

  2. Re: [PATCH] Suppress Clang 3.9 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-20T14:26:44Z

    Aleksander Alekseev <a.alekseev@postgrespro.ru> writes:
> I've just tried to build PostgreSQL with Clang 3.9.1 (default version
> currently available in Arch Linux) and noticed that it outputs lots of
> warning messages. Most of them are result of a bug in Clang itself:
> 
> postinit.c:846:3: note: include the header <string.h> or explicitly
> provide a declaration for 'strlcpy'

It might be an incompatibility with the platform-supplied string.h
rather than an outright bug, but yeah, that's pretty annoying.

> The rest of warnings looks more like something we could easily deal with:

It's hard to get excited about these if there are going to be hundreds
of the other ones ...

			regards, tom lane

  3. Re: [PATCH] Suppress Clang 3.9 warnings

    Aleksander Alekseev <a.alekseev@postgrespro.ru> — 2017-02-20T14:34:30Z

    In theory - could we just always use our internal strl* implementations? 

On Mon, Feb 20, 2017 at 09:26:44AM -0500, Tom Lane wrote:
> Aleksander Alekseev <a.alekseev@postgrespro.ru> writes:
> > I've just tried to build PostgreSQL with Clang 3.9.1 (default version
> > currently available in Arch Linux) and noticed that it outputs lots of
> > warning messages. Most of them are result of a bug in Clang itself:
> > 
> > postinit.c:846:3: note: include the header <string.h> or explicitly
> > provide a declaration for 'strlcpy'
> 
> It might be an incompatibility with the platform-supplied string.h
> rather than an outright bug, but yeah, that's pretty annoying.
> 
> > The rest of warnings looks more like something we could easily deal with:
> 
> It's hard to get excited about these if there are going to be hundreds
> of the other ones ...
> 
> 			regards, tom lane

-- 
Best regards,
Aleksander Alekseev

  4. Re: [PATCH] Suppress Clang 3.9 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-20T15:37:41Z

    Aleksander Alekseev <a.alekseev@postgrespro.ru> writes:
> In theory - could we just always use our internal strl* implementations? 

Hmm, maybe configure's test to see if a declaration has been provided
is going wrong?  I notice that anchovy, which is supposedly current
Arch Linux, doesn't think the platform has it:

checking whether strlcat is declared... no
checking whether strlcpy is declared... no
...
checking for strlcat... no
checking for strlcpy... no

But that's using gcc.  Perhaps clang behaves differently?

			regards, tom lane

  5. Re: [PATCH] Suppress Clang 3.9 warnings

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2017-02-20T15:48:38Z

    On 02/20/2017 04:37 PM, Tom Lane wrote:
> Aleksander Alekseev <a.alekseev@postgrespro.ru> writes:
>> In theory - could we just always use our internal strl* implementations?
>
> Hmm, maybe configure's test to see if a declaration has been provided
> is going wrong?  I notice that anchovy, which is supposedly current
> Arch Linux, doesn't think the platform has it:
>
> checking whether strlcat is declared... no
> checking whether strlcpy is declared... no
> ...
> checking for strlcat... no
> checking for strlcpy... no
>
> But that's using gcc.  Perhaps clang behaves differently?
>

AFAIK it happens because clang treats missing declarations as warnings, 
which confuses configure:

https://bugs.llvm.org//show_bug.cgi?id=20820

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: [PATCH] Suppress Clang 3.9 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-20T15:56:47Z

    Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On 02/20/2017 04:37 PM, Tom Lane wrote:
>> But that's using gcc.  Perhaps clang behaves differently?

> AFAIK it happens because clang treats missing declarations as warnings, 
> which confuses configure:
> https://bugs.llvm.org//show_bug.cgi?id=20820

Ah, right.  Looks like the autoconf people still haven't made a
release incorporating the fix Noah provided :-(

			regards, tom lane

  7. Re: [PATCH] Suppress Clang 3.9 warnings

    Fabien COELHO <coelho@cri.ensmp.fr> — 2017-02-22T14:43:34Z

    Hello Aleksander,

> ```
> xloginsert.c:742:18: warning: implicit conversion from 'int' to 'char'
> changes value from 253 to -3 [-Wconstant-conversion]
> ```

There is a bunch of these in "xlog.c" as well, and the same code is used 
in "pg_resetwal.c".

> Patch that fixes these warnings is attached to this email.

My 0.02€:

I'm not at ease at putting the thing bluntly under the carpet with a cast.

Why not update the target type to "unsigned char" instead, so that no cast 
is needed and the value compatibility is checked by the compiler? I guess 
there would be some more changes (question is how much), but it would be 
cleaner.

-- 
Fabien.

  8. Re: [PATCH] Suppress Clang 3.9 warnings

    David Steele <david@pgmasters.net> — 2017-03-13T14:19:57Z

    Hi Aleksander,

On 2/22/17 9:43 AM, Fabien COELHO wrote:
> 
> Hello Aleksander,
> 
>> ```
>> xloginsert.c:742:18: warning: implicit conversion from 'int' to 'char'
>> changes value from 253 to -3 [-Wconstant-conversion]
>> ```
> 
> There is a bunch of these in "xlog.c" as well, and the same code is used
> in "pg_resetwal.c".
> 
>> Patch that fixes these warnings is attached to this email.
> 
> My 0.02€:
> 
> I'm not at ease at putting the thing bluntly under the carpet with a cast.
> 
> Why not update the target type to "unsigned char" instead, so that no
> cast is needed and the value compatibility is checked by the compiler? I
> guess there would be some more changes (question is how much), but it
> would be cleaner.

There's been no discussion or new patch on this thread recently.  If you
are planning to address the issues raised please plan to do so by
Thursday, March 16th.

If no new patch is submitted by that date I will mark this patch
"Returned with Feedback".

Thanks,
-- 
-David
david@pgmasters.net

  9. Re: [PATCH] Suppress Clang 3.9 warnings

    Aleksander Alekseev <a.alekseev@postgrespro.ru> — 2017-03-13T15:35:53Z

    Hi David,

Thank you for reminding about this patch!

Here is a new patch. I tried to make as little changes as possible. This
is no doubt not the most beautiful patch on Earth but it removes all
warnings. I anyone could suggest an approach that would be significantly
better please don't hesitate to share your ideas.

Tested on Clang 3.9.1 and GCC 6.3.1.

> Why not update the target type to "unsigned char" instead, so that no
> cast is needed and the value compatibility is checked by the compiler? I
> guess there would be some more changes (question is how much), but it
> would be cleaner.

I tried this way as well. After rebuilding PostgreSQL in 5th time I
discovered that now I have to redefine a Poiner typedef. I don't think
we can avoid using type casts. There will be just different type casts
in other places, or we'll have to break most of existing PostgreSQL
extensions.

On Mon, Mar 13, 2017 at 10:19:57AM -0400, David Steele wrote:
> Hi Aleksander,
> 
> On 2/22/17 9:43 AM, Fabien COELHO wrote:
> > 
> > Hello Aleksander,
> > 
> >> ```
> >> xloginsert.c:742:18: warning: implicit conversion from 'int' to 'char'
> >> changes value from 253 to -3 [-Wconstant-conversion]
> >> ```
> > 
> > There is a bunch of these in "xlog.c" as well, and the same code is used
> > in "pg_resetwal.c".
> > 
> >> Patch that fixes these warnings is attached to this email.
> > 
> > My 0.02€:
> > 
> > I'm not at ease at putting the thing bluntly under the carpet with a cast.
> > 
> > Why not update the target type to "unsigned char" instead, so that no
> > cast is needed and the value compatibility is checked by the compiler? I
> > guess there would be some more changes (question is how much), but it
> > would be cleaner.
> 
> There's been no discussion or new patch on this thread recently.  If you
> are planning to address the issues raised please plan to do so by
> Thursday, March 16th.
> 
> If no new patch is submitted by that date I will mark this patch
> "Returned with Feedback".
> 
> Thanks,
> -- 
> -David
> david@pgmasters.net

-- 
Best regards,
Aleksander Alekseev

  10. Re: [PATCH] Suppress Clang 3.9 warnings

    Noah Misch <noah@leadboat.com> — 2017-03-15T02:25:38Z

    On Mon, Mar 13, 2017 at 06:35:53PM +0300, Aleksander Alekseev wrote:
> --- a/src/include/port.h
> +++ b/src/include/port.h
> @@ -395,11 +395,22 @@ extern double rint(double x);
>  extern int	inet_aton(const char *cp, struct in_addr * addr);
>  #endif
>  
> -#if !HAVE_DECL_STRLCAT
> +/*
> + * Unfortunately in case of strlcat and strlcpy we can't trust tests
> + * executed by Autotools if Clang > 3.6 is used. Clang manages to compile
> + * a program that shouldn't compile which causes wrong values of
> + * HAVE_DECL_STRLCAT and HAVE_DECL_STRLCPY. More details could be found here:
> + *
> + * http://lists.llvm.org/pipermail/cfe-dev/2016-March/048126.html
> + *
> + * This is no doubt a dirty hack but apparently alternative solutions are
> + * not much better.
> + */
> +#if !HAVE_DECL_STRLCAT || defined(__clang__)
>  extern size_t strlcat(char *dst, const char *src, size_t siz);
>  #endif
>  
> -#if !HAVE_DECL_STRLCPY
> +#if !HAVE_DECL_STRLCPY || defined(__clang__)
>  extern size_t strlcpy(char *dst, const char *src, size_t siz);
>  #endif

This is wrong on platforms that do have strlcpy() in libc.

If I recall correctly, you can suppress the warnings in your own build by
adding "ac_cv_func_strlcpy=no ac_cv_have_decl_strlcpy=no ac_cv_func_strlcat=no
ac_cv_have_decl_strlcat=no" to the "configure" command line.

  11. Re: [PATCH] Suppress Clang 3.9 warnings

    Aleksander Alekseev <a.alekseev@postgrespro.ru> — 2017-03-15T09:04:11Z

    Hi Hoah.

Thanks a lot for a reply!

> This is wrong on platforms that do have strlcpy() in libc.

If it no too much trouble could you please explain what will happen
on such platforms? On what platform did you check it? I'm sure it
fixable. And I got a strong feeling that "wrong" could be a bit exaggerated.

> If I recall correctly, you can suppress the warnings in your own build by
> adding "ac_cv_func_strlcpy=no ac_cv_have_decl_strlcpy=no ac_cv_func_strlcat=no
> ac_cv_have_decl_strlcat=no" to the "configure" command line.

It's not exactly what I would call a solution. For instance on FreeBSD
Clang is a default compiler and many users build a software from source
code (FreeBSD ports). For some reason I doubt that many of them know
about these flags.

On Wed, Mar 15, 2017 at 02:25:38AM +0000, Noah Misch wrote:
> On Mon, Mar 13, 2017 at 06:35:53PM +0300, Aleksander Alekseev wrote:
> > --- a/src/include/port.h
> > +++ b/src/include/port.h
> > @@ -395,11 +395,22 @@ extern double rint(double x);
> >  extern int	inet_aton(const char *cp, struct in_addr * addr);
> >  #endif
> >  
> > -#if !HAVE_DECL_STRLCAT
> > +/*
> > + * Unfortunately in case of strlcat and strlcpy we can't trust tests
> > + * executed by Autotools if Clang > 3.6 is used. Clang manages to compile
> > + * a program that shouldn't compile which causes wrong values of
> > + * HAVE_DECL_STRLCAT and HAVE_DECL_STRLCPY. More details could be found here:
> > + *
> > + * http://lists.llvm.org/pipermail/cfe-dev/2016-March/048126.html
> > + *
> > + * This is no doubt a dirty hack but apparently alternative solutions are
> > + * not much better.
> > + */
> > +#if !HAVE_DECL_STRLCAT || defined(__clang__)
> >  extern size_t strlcat(char *dst, const char *src, size_t siz);
> >  #endif
> >  
> > -#if !HAVE_DECL_STRLCPY
> > +#if !HAVE_DECL_STRLCPY || defined(__clang__)
> >  extern size_t strlcpy(char *dst, const char *src, size_t siz);
> >  #endif
> 
> This is wrong on platforms that do have strlcpy() in libc.
> 
> If I recall correctly, you can suppress the warnings in your own build by
> adding "ac_cv_func_strlcpy=no ac_cv_have_decl_strlcpy=no ac_cv_func_strlcat=no
> ac_cv_have_decl_strlcat=no" to the "configure" command line.

-- 
Best regards,
Aleksander Alekseev

  12. Re: [PATCH] Suppress Clang 3.9 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-15T14:57:15Z

    Noah Misch <noah@leadboat.com> writes:
> On Mon, Mar 13, 2017 at 06:35:53PM +0300, Aleksander Alekseev wrote:
>> + * Unfortunately in case of strlcat and strlcpy we can't trust tests
>> + * executed by Autotools if Clang > 3.6 is used.

> This is wrong on platforms that do have strlcpy() in libc.

Didn't you submit a patch to upstream autoconf awhile ago to fix the
AC_CHECK_DECLS test for this?  Seems like the correct solution is to
absorb that fix, either by updating to a newer autoconf release or by
carrying our own version of AC_CHECK_DECLS until they come out with one.

			regards, tom lane

  13. Re: [PATCH] Suppress Clang 3.9 warnings

    Noah Misch <noah@leadboat.com> — 2017-03-15T16:09:40Z

    On Wed, Mar 15, 2017 at 10:57:15AM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Mon, Mar 13, 2017 at 06:35:53PM +0300, Aleksander Alekseev wrote:
> >> + * Unfortunately in case of strlcat and strlcpy we can't trust tests
> >> + * executed by Autotools if Clang > 3.6 is used.
> 
> > This is wrong on platforms that do have strlcpy() in libc.
> 
> Didn't you submit a patch to upstream autoconf awhile ago to fix the
> AC_CHECK_DECLS test for this?

Yes.

> Seems like the correct solution is to
> absorb that fix, either by updating to a newer autoconf release or by
> carrying our own version of AC_CHECK_DECLS until they come out with one.

As you mention upthread, that Autoconf commit is still newer than every
Autoconf release.  (Last release was 58 months ago.)  Altering configure.ac to
work around the bug would be reasonable, but it feels heavy relative to the
benefit of suppressing some warnings.

  14. Re: [PATCH] Suppress Clang 3.9 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-15T16:21:21Z

    Noah Misch <noah@leadboat.com> writes:
> On Wed, Mar 15, 2017 at 10:57:15AM -0400, Tom Lane wrote:
>> Seems like the correct solution is to
>> absorb that fix, either by updating to a newer autoconf release or by
>> carrying our own version of AC_CHECK_DECLS until they come out with one.

> As you mention upthread, that Autoconf commit is still newer than every
> Autoconf release.  (Last release was 58 months ago.)  Altering configure.ac to
> work around the bug would be reasonable, but it feels heavy relative to the
> benefit of suppressing some warnings.

It does seem like rather a lot of work, but I think it's preferable to
hacking up the coding in port.h.  Mainly because we could booby-trap the
substitute AC_CHECK_DECLS to make sure we revert it whenever autoconf 2.70
does materialize (a check on m4_PACKAGE_VERSION, like the one at
configure.in line 22, ought to do the trick); whereas I do not think
we'd remember to de-kluge port.h if we kluge around it there.

I'm fine with leaving it alone, too.

			regards, tom lane

  15. Re: [PATCH] Suppress Clang 3.9 warnings

    Noah Misch <noah@leadboat.com> — 2017-03-16T06:55:20Z

    This mailing list does not welcome top-post replies.

On Wed, Mar 15, 2017 at 12:04:11PM +0300, Aleksander Alekseev wrote:
> > This is wrong on platforms that do have strlcpy() in libc.
> 
> If it no too much trouble could you please explain what will happen
> on such platforms?

Both port.h and a system header will furnish a strlcpy() declaration.  The #if
you modified exists to avoid that, and your change would make it ineffective
for Clang.  This will have no symptoms, or it will elicit a warning.

> On what platform did you check it?

None.

  16. Re: [PATCH] Suppress Clang 3.9 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-16T14:17:45Z

    Noah Misch <noah@leadboat.com> writes:
> On Wed, Mar 15, 2017 at 12:04:11PM +0300, Aleksander Alekseev wrote:
>> If it no too much trouble could you please explain what will happen
>> on such platforms?

> Both port.h and a system header will furnish a strlcpy() declaration.  The #if
> you modified exists to avoid that, and your change would make it ineffective
> for Clang.  This will have no symptoms, or it will elicit a warning.

The reason why this is bad is that port.h's declaration might be different
from the system headers'.  That's not hypothetical; for example, on my
Mac laptop, strlcpy is declared

size_t strlcpy(char * restrict dst, const char * restrict src, size_t size);

whereas of course there's no "restrict" in port.h.  To make matters worse,
it looks like strlcpy is actually a macro expanding to
'__builtin___strlcpy_chk'.  And the compiler on this box *is* clang,
meaning the proposed patch would affect it.  When I try it, I get
boatloads of errors (not warnings) like these:

In file included from ../../src/include/postgres_fe.h:25:
In file included from ../../src/include/c.h:1125:
../../src/include/port.h:403:15: error: expected parameter declarator
extern size_t strlcpy(char *dst, const char *src, size_t siz);
              ^
/usr/include/secure/_string.h:105:44: note: expanded from macro 'strlcpy'
  __builtin___strlcpy_chk (dest, src, len, __darwin_obsz (dest))
                                           ^

../../src/include/port.h:403:15: error: conflicting types for '__builtin___strlcpy_chk'
/usr/include/secure/_string.h:105:3: note: expanded from macro 'strlcpy'

In short, if this were to get committed, it would get reverted within
minutes, because more than a few of us use Macs.

			regards, tom lane

  17. Re: [PATCH] Suppress Clang 3.9 warnings

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-03-17T22:02:40Z

    On 3/13/17 11:35, Aleksander Alekseev wrote:
> Here is a new patch. I tried to make as little changes as possible. This
> is no doubt not the most beautiful patch on Earth but it removes all
> warnings. I anyone could suggest an approach that would be significantly
> better please don't hesitate to share your ideas.

I'm also seeing the -Wconstant-conversion warnings with clang-4.0.  The
warnings about strlcpy don't appear here.  That might be something
specific to the operating system.

To address the -Wconstant-conversion warnings, I suggest changing the
variables to unsigned char * as appropriate.

However, this would require a large number of changes to all call sites
of XLogRegisterData(), because they all have a cast like this:

    XLogRegisterData((char *) &xlrec, SizeOfHashMovePageContents);

Perhaps the first argument could be changed to void *.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  18. Re: [PATCH] Suppress Clang 3.9 warnings

    Aleksander Alekseev <a.alekseev@postgrespro.ru> — 2017-03-24T12:52:26Z

    Hi Tom,

Since no one seems to be particularly excited about this patch I'm
marking it as "Returned with feedback" to save reviewers time.

On Wed, Mar 15, 2017 at 12:21:21PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Wed, Mar 15, 2017 at 10:57:15AM -0400, Tom Lane wrote:
> >> Seems like the correct solution is to
> >> absorb that fix, either by updating to a newer autoconf release or by
> >> carrying our own version of AC_CHECK_DECLS until they come out with one.
> 
> > As you mention upthread, that Autoconf commit is still newer than every
> > Autoconf release.  (Last release was 58 months ago.)  Altering configure.ac to
> > work around the bug would be reasonable, but it feels heavy relative to the
> > benefit of suppressing some warnings.
> 
> It does seem like rather a lot of work, but I think it's preferable to
> hacking up the coding in port.h.  Mainly because we could booby-trap the
> substitute AC_CHECK_DECLS to make sure we revert it whenever autoconf 2.70
> does materialize (a check on m4_PACKAGE_VERSION, like the one at
> configure.in line 22, ought to do the trick); whereas I do not think
> we'd remember to de-kluge port.h if we kluge around it there.
> 
> I'm fine with leaving it alone, too.
> 
> 			regards, tom lane

-- 
Best regards,
Aleksander Alekseev