Обсуждение: Small patch: fix warnings during compilation on FreeBSD

Hello

I noticed that master branch of PostgreSQL currently compiles with
warnings on FreeBSD 10.2 RELEASE:

```
pg_locale.c:1290:12: warning: implicit declaration of function
'wcstombs_l' is invalid in C99 [-Wimplicit-function-declaration]

result = wcstombs_l(to, from, tolen, locale);

pg_locale.c:1365:13: warning: implicit declaration of function
'mbstowcs_l' is invalid in C99 [-Wimplicit-function-declaration]

result = mbstowcs_l(to, str, tolen, locale);

2 warnings generated.
```

The problem is that both procedures are declared in xlocale.h file
which is not included in given context. If I remember correctly in
this case compiler assumes that procedures return `int` (instead of
size_t) so I doubt we can ignore this issue.

Frankly I'm not sure what is a right way of fixing this. My best guess
is attached to this message. I tested this patch on Ubuntu Linux 14.04
and FreeBSD 10.2. It solves a described problem and apparently doesn't
break anything (code compiles, regression tests pass, etc).

Best regards,
Aleksander

Aleksander Alekseev <a.alekseev@postgrespro.ru> writes:
> I noticed that master branch of PostgreSQL currently compiles with
> warnings on FreeBSD 10.2 RELEASE:
> pg_locale.c:1290:12: warning: implicit declaration of function
> 'wcstombs_l' is invalid in C99 [-Wimplicit-function-declaration]

> The problem is that both procedures are declared in xlocale.h file
> which is not included in given context.

OK.

> Frankly I'm not sure what is a right way of fixing this.

Not like that, as it will break entirely on machines without xlocale.h
(which I presume is pretty nonstandard; it's certainly not mentioned
in the POSIX spec).  It will also break things on glibc, according to
this comment in our c-library.m4:

# Check for the locale_t type and find the right header file.  Mac OS
# X needs xlocale.h; standard is locale.h, but glibc also has an
# xlocale.h file that we should not use.

I think what we need is configure logic to find out where wcstombs_l()
is declared, and then #include <xlocale.h> only if it's necessary to get
that definition.  I haven't experimented but probably you could make such
a check with nested uses of AC_CHECK_DECL.
        regards, tom lane

Hello, Tom

> I think what we need is configure logic to find out where wcstombs_l()
> is declared, and then #include <xlocale.h> only if it's necessary to
> get that definition.  I haven't experimented but probably you could
> make such a check with nested uses of AC_CHECK_DECL.

Sounds like quite a dirty hack to me. Besides so far we have only two
procedures from xlocale.h and this requires two checks. If we go this
way someday there will be 15 checks for every procedure from xlocale.h
and logic like:

```
#if PROC1_DEFINED_IN_XLOCALE || PROC2_DEFINED_IN_XLOCALE ...
#include <xlocale.h>
#endif
```

Perhaps we could just use __FreeBSD__ macro instead (see attachments)?
Or maybe create our own macro ALWAYS_INCLUDE_XLOCALE in configure
script which currently will depend only on used OS? Naturally this
definition could be changed in the future.

Best regards,
Aleksander

On Fri, Mar 11, 2016 at 9:13 AM, Aleksander Alekseev
<a.alekseev@postgrespro.ru> wrote:
> Hello, Tom
>
>> I think what we need is configure logic to find out where wcstombs_l()
>> is declared, and then #include <xlocale.h> only if it's necessary to
>> get that definition.  I haven't experimented but probably you could
>> make such a check with nested uses of AC_CHECK_DECL.
>
> Sounds like quite a dirty hack to me. Besides so far we have only two
> procedures from xlocale.h and this requires two checks. If we go this
> way someday there will be 15 checks for every procedure from xlocale.h

Eh, probably not.  Most likely, if you check whether one of the
functions you care about is in that file, that's good enough.  Either
all of them will be there or none of them.  That may sound
unprincipled, but I think in practice it works pretty well, and I
think it's basically the autoconf way.  You make the checks just
sophisticated enough to work on all of the platforms that actually
exist, and don't worry about hypothetical platforms where the header
file authors conspire to hose you.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Mar 11, 2016 at 9:13 AM, Aleksander Alekseev
> <a.alekseev@postgrespro.ru> wrote:
>> Sounds like quite a dirty hack to me. Besides so far we have only two
>> procedures from xlocale.h and this requires two checks. If we go this
>> way someday there will be 15 checks for every procedure from xlocale.h

> Eh, probably not.  Most likely, if you check whether one of the
> functions you care about is in that file, that's good enough.

Yeah.  In practice, there are exactly two cases we care about: either
both of these functions will be declared in <stdlib.h> like POSIX
says, or both of them will be in <xlocale.h>.  There's no need to
work harder than we have to do to figure that out.

I'm totally unimpressed with the proposal of depending on the __FreeBSD__
macro instead of having a proper configure check.  For one thing, we
have no idea whether NetBSD or OpenBSD have this same issue.  For
another, it might be version-specific, or might become so if FreeBSD
decides to start following POSIX on this point someday.
        regards, tom lane

> Yeah.  In practice, there are exactly two cases we care about: either
> both of these functions will be declared in <stdlib.h> like POSIX
> says, or both of them will be in <xlocale.h>.  There's no need to
> work harder than we have to do to figure that out.
>
> I'm totally unimpressed with the proposal of depending on the
> __FreeBSD__ macro instead of having a proper configure check.  For
> one thing, we have no idea whether NetBSD or OpenBSD have this same
> issue.  For another, it might be version-specific, or might become so
> if FreeBSD decides to start following POSIX on this point someday.

OK, I'm not an expert in Autotools but this patch (see attachment) seems
to solve a problem.

Here are some notes.

Unfortunately test program requires two include files:

```
#include <stdlib.h>
#include <xlocale.h>

int main()
{
  size_t tmp = wcstombs_l(NULL, NULL, 0, 0);
  return 0;
}
```

Thus I need two checks - 1) that test program compiles when xlocal.h is
included 2) that test program does not compile if only stdlib.h is
included (what if wcstombs_l is actually declared there?).

On Ubuntu 14.04:

```
$ ./configure
...
checking whether wcstombs_l is available if stdlib.h is included... no
checking whether wcstombs_l is available if stdlib.h and xlocale.h are
included... no ...

$ cat ./src/include/pg_config.h | grep WCSTOMBS_L_IN_XLOCALE
/* #undef WCSTOMBS_L_IN_XLOCALE */

$ make -j2 -s
Writing postgres.bki
Writing schemapg.h
Writing postgres.description
Writing postgres.shdescription
Writing fmgroids.h
Writing fmgrtab.c
In file included from gram.y:14933:0:
scan.c: In function ‘yy_try_NUL_trans’:
scan.c:10321:23: warning: unused variable ‘yyg’ [-Wunused-variable]
     struct yyguts_t * yyg = (struct yyguts_t*)yyscanner; /* This var
may be unused depending upon options. */ ^
$ make check

...
=======================
 All 161 tests passed.
=======================

```

On FreeBSD 10.2:

```
$ ./configure
...
checking whether wcstombs_l is available if stdlib.h is included... no
checking whether wcstombs_l is available if stdlib.h and xlocale.h are
included... yes ...

$ cat ./src/include/pg_config.h | grep WCSTOMBS_L_IN_XLOCALE
#define WCSTOMBS_L_IN_XLOCALE 1

$ gmake -j2 -s
Writing postgres.bki
Writing schemapg.h
Writing postgres.description
Writing postgres.shdescription
Writing fmgroids.h
Writing fmgrtab.c

$ gmake check

...
=======================
 All 161 tests passed.
=======================
```

As you can see warnings are gone. Warning on Ubuntu was always there
and as comment suggests is irrelevant in current context.

Please note that these changes:

```
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 <<
31) << 31))
```

... were generated but `autoreconf -iv`. I was not sure what to do
about them. Eventually I decided to keep them. Still these changes could
be safely discarded.

--
Best regards,
Aleksander Alekseev
http://eax.me/

Aleksander Alekseev wrote:

> Please note that these changes:
> 
> ```
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 <<
> 31) << 31))
> ```
> 
> ... were generated but `autoreconf -iv`. I was not sure what to do
> about them. Eventually I decided to keep them. Still these changes could
> be safely discarded.

I have noticed these while messing with configure.in too.  These are
generated by an autoconf as patched by Debian; see
http://bugs.debian.org/742780
This is said to fix undefined behavior when off_t is 32 bits.

They are not present in stock GNU autoconf 2.69 nor are in autoconf's
git repo.  I think we should continue to use the output of stock,
unpatched autoconf.

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

Aleksander Alekseev <a.alekseev@postgrespro.ru> writes:
> OK, I'm not an expert in Autotools but this patch (see attachment) seems
> to solve a problem.

I fooled around with this some.  I felt originally that it should use
AC_CHECK_DECL, but that turns out not to work because AC_CHECK_DECL
has caching behavior built-in and so the second call did nothing.
However, we should still adopt the probe methodology it uses rather
than inventing our own; basically that's

#ifndef wcstombs_l
(void) wcstombs_l;
#endif

Also, after reviewing the bidding a bit more it seems likely to me
that wcstombs_l() might be declared in <locale.h> on some platforms;
which would be problematic for this test as written if <xlocale.h>
pulls in <locale.h>.  So the right way to make the comparison is to
determine whether stdlib.h+locale.h+xlocale.h succeeds where
stdlib.h+locale.h fails.

I've checked this on my OS X box, which turns out to have the interesting
property that xlocale.h declares wcstombs_l(), but only if you previously
included stdlib.h ... wtf?  But anyway that behavior is part of the
motivation for leaving stdlib.h in the test.

Please verify that the committed version solves your problem on FreeBSD.

> Please note that these changes:
> ... were generated but `autoreconf -iv`. I was not sure what to do
> about them. Eventually I decided to keep them. Still these changes could
> be safely discarded.

Yeah, it's not uncommon for platforms to carry local patches in their
autoconf packages, which results in changes like these.  We make a point
of generating our configure using unmodified GNU autoconf installations,
so as to avoid dependencies on which platform a committer was using to
run autoconf.

            regards, tom lane

diff --git a/config/c-library.m4 b/config/c-library.m4
index 1d28c45..50d068d 100644
*** a/config/c-library.m4
--- b/config/c-library.m4
*************** fi
*** 316,319 ****
  if test "$pgac_cv_type_locale_t" = 'yes (in xlocale.h)'; then
    AC_DEFINE(LOCALE_T_IN_XLOCALE, 1,
              [Define to 1 if `locale_t' requires <xlocale.h>.])
! fi])])# PGAC_HEADER_XLOCALE
--- 316,349 ----
  if test "$pgac_cv_type_locale_t" = 'yes (in xlocale.h)'; then
    AC_DEFINE(LOCALE_T_IN_XLOCALE, 1,
              [Define to 1 if `locale_t' requires <xlocale.h>.])
! fi])# PGAC_TYPE_LOCALE_T
!
!
! # PGAC_FUNC_WCSTOMBS_L
! # --------------------
! # Try to find a declaration for wcstombs_l().  It might be in stdlib.h
! # (following the POSIX requirement for wcstombs()), or in locale.h, or in
! # xlocale.h.  If it's in the latter, define WCSTOMBS_L_IN_XLOCALE.
! #
! AC_DEFUN([PGAC_FUNC_WCSTOMBS_L],
! [AC_CACHE_CHECK([for wcstombs_l declaration], pgac_cv_func_wcstombs_l,
! [AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
! [#include <stdlib.h>
! #include <locale.h>],
! [#ifndef wcstombs_l
! (void) wcstombs_l;
! #endif])],
! [pgac_cv_func_wcstombs_l='yes'],
! [AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
! [#include <stdlib.h>
! #include <locale.h>
! #include <xlocale.h>],
! [#ifndef wcstombs_l
! (void) wcstombs_l;
! #endif])],
! [pgac_cv_func_wcstombs_l='yes (in xlocale.h)'],
! [pgac_cv_func_wcstombs_l='no'])])])
! if test "$pgac_cv_func_wcstombs_l" = 'yes (in xlocale.h)'; then
!   AC_DEFINE(WCSTOMBS_L_IN_XLOCALE, 1,
!             [Define to 1 if `wcstombs_l' requires <xlocale.h>.])
! fi])# PGAC_FUNC_WCSTOMBS_L
diff --git a/configure b/configure
index 08cff23..a45be67 100755
*** a/configure
--- b/configure
*************** $as_echo "#define GETTIMEOFDAY_1ARG 1" >
*** 12364,12369 ****
--- 12364,12422 ----

  fi

+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for wcstombs_l declaration" >&5
+ $as_echo_n "checking for wcstombs_l declaration... " >&6; }
+ if ${pgac_cv_func_wcstombs_l+:} false; then :
+   $as_echo_n "(cached) " >&6
+ else
+   cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+ /* end confdefs.h.  */
+ #include <stdlib.h>
+ #include <locale.h>
+ int
+ main ()
+ {
+ #ifndef wcstombs_l
+ (void) wcstombs_l;
+ #endif
+   ;
+   return 0;
+ }
+ _ACEOF
+ if ac_fn_c_try_compile "$LINENO"; then :
+   pgac_cv_func_wcstombs_l='yes'
+ else
+   cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+ /* end confdefs.h.  */
+ #include <stdlib.h>
+ #include <locale.h>
+ #include <xlocale.h>
+ int
+ main ()
+ {
+ #ifndef wcstombs_l
+ (void) wcstombs_l;
+ #endif
+   ;
+   return 0;
+ }
+ _ACEOF
+ if ac_fn_c_try_compile "$LINENO"; then :
+   pgac_cv_func_wcstombs_l='yes (in xlocale.h)'
+ else
+   pgac_cv_func_wcstombs_l='no'
+ fi
+ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ fi
+ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ fi
+ { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_func_wcstombs_l" >&5
+ $as_echo "$pgac_cv_func_wcstombs_l" >&6; }
+ if test "$pgac_cv_func_wcstombs_l" = 'yes (in xlocale.h)'; then
+
+ $as_echo "#define WCSTOMBS_L_IN_XLOCALE 1" >>confdefs.h
+
+ fi

  # Some versions of libedit contain strlcpy(), setproctitle(), and other
  # symbols that that library has no business exposing to the world.  Pending
diff --git a/configure.in b/configure.in
index 0b7dd97..c298926 100644
*** a/configure.in
--- b/configure.in
*************** fi
*** 1423,1428 ****
--- 1423,1429 ----
  PGAC_VAR_INT_TIMEZONE
  AC_FUNC_ACCEPT_ARGTYPES
  PGAC_FUNC_GETTIMEOFDAY_1ARG
+ PGAC_FUNC_WCSTOMBS_L

  # Some versions of libedit contain strlcpy(), setproctitle(), and other
  # symbols that that library has no business exposing to the world.  Pending
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index b3ceea5..3813226 100644
*** a/src/include/pg_config.h.in
--- b/src/include/pg_config.h.in
***************
*** 851,856 ****
--- 851,859 ----
  /* Define to select Win32-style shared memory. */
  #undef USE_WIN32_SHARED_MEMORY

+ /* Define to 1 if `wcstombs_l' requires <xlocale.h>. */
+ #undef WCSTOMBS_L_IN_XLOCALE
+
  /* Define WORDS_BIGENDIAN to 1 if your processor stores words with the most
     significant byte first (like Motorola and SPARC, unlike Intel). */
  #if defined AC_APPLE_UNIVERSAL_BUILD
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index 8566065..eba36df 100644
*** a/src/include/pg_config.h.win32
--- b/src/include/pg_config.h.win32
***************
*** 657,662 ****
--- 657,665 ----
  /* Define to select Win32-style semaphores. */
  #define USE_WIN32_SEMAPHORES 1

+ /* Define to 1 if `wcstombs_l' requires <xlocale.h>. */
+ /* #undef WCSTOMBS_L_IN_XLOCALE */
+
  /* Number of bits in a file offset, on hosts where this is settable. */
  /* #undef _FILE_OFFSET_BITS */

diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index 2e6dba1..0a4b9f7 100644
*** a/src/include/utils/pg_locale.h
--- b/src/include/utils/pg_locale.h
***************
*** 13,19 ****
  #define _PG_LOCALE_

  #include <locale.h>
! #ifdef LOCALE_T_IN_XLOCALE
  #include <xlocale.h>
  #endif

--- 13,19 ----
  #define _PG_LOCALE_

  #include <locale.h>
! #if defined(LOCALE_T_IN_XLOCALE) || defined(WCSTOMBS_L_IN_XLOCALE)
  #include <xlocale.h>
  #endif

> Please verify that the committed version solves your problem on
> FreeBSD.

I confirm this patch solves a problem.

> I've checked this on my OS X box, which turns out to have the
> interesting property that xlocale.h declares wcstombs_l(), but only
> if you previously included stdlib.h ... wtf?

Same on FreeBSD.

-- 
Best regards,
Aleksander Alekseev
http://eax.me/