Thread: [HACKERS] [PATCH] Suppress Clang 3.9 warnings
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
Attachment
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
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
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
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
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
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.
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
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
Attachment
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.
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
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
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.
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
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.
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
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
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