Re: gnulib stdint.h substitution of int64_t results in a linking error in GCC 4.(3|2|0) on OSX

[Jarno Rajahalme]

On Nov 24, 2010, at 22:07 , ext Paul Eggert wrote:

> I pushed the following patch; could you please give it a try?
> I don't have an OSX host to test it on.  Thanks.

The patch works, tested with 64-bit compile with GCC 4.2.

Thanks,

  Jarno

> 
> From 531b8a416b6ae40f89808e1db8976eb25972e661 Mon Sep 17 00:00:00 2001
> From: Paul Eggert 
> Date: Wed, 24 Nov 2010 12:05:43 -0800
> Subject: [PATCH] stdint: port to GCC 4.3 + OSX + Octave
> 
> On this platform, stdint.h is buggy and defines int64_t to long
> long int.  The replacement defined it to long int, causing
> problems with C++ style name mangling.  Instead, trust the system
> definition if INT64_MAX is defined, and likewise for the unsigned
> variant.   Problem reported by Jarno Rajahalme in
> .
> * lib/stdint.in.h (GL_INT64_T): Define if INT64_MAX is defined,
> and don't mess with int64_t and INT64_MAX in this case.
> (GL_UINT64_T): Likewise for UINT64_MAX and uint64_t.
> ---
> ChangeLog   |   13 ++
> lib/stdint.in.h |   68 +++---
> 2 files changed, 52 insertions(+), 29 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 86d939a..8dd51e6 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,16 @@
> +2010-11-24  Paul Eggert  
> +
> + stdint: port to GCC 4.3 + OSX + Octave
> + On this platform, stdint.h is buggy and defines int64_t to long
> + long int.  The replacement defined it to long int, causing
> + problems with C++ style name mangling.  Instead, trust the system
> + definition if INT64_MAX is defined, and likewise for the unsigned
> + variant.   Problem reported by Jarno Rajahalme in
> + .
> + * lib/stdint.in.h (GL_INT64_T): Define if INT64_MAX is defined,
> + and don't mess with int64_t and INT64_MAX in this case.
> + (GL_UINT64_T): Likewise for UINT64_MAX and uint64_t.
> +
> 2010-11-24  Bruno Haible  
> 
>   doc: Corrections regarding MacOS X 10.4 and 10.5.
> diff --git a/lib/stdint.in.h b/lib/stdint.in.h
> index e660cfb..70822b8 100644
> --- a/lib/stdint.in.h
> +++ b/lib/stdint.in.h
> @@ -134,40 +134,53 @@ typedef unsigned int gl_uint32_t;
> #define int32_t gl_int32_t
> #define uint32_t gl_uint32_t
> 
> +/* If the system defines INT64_MAX, assume int64_t works.  That way,
> +   if the underlying platform defines int64_t to be a 64-bit long long
> +   int, the code below won't mistakenly define it to be a 64-bit long
> +   int, which would mess up C++ name mangling.  */
> +
> +#if INT64_MAX
> +# define GL_INT64_T
> +#else
> /* Do not undefine int64_t if gnulib is not being used with 64-bit
>types, since otherwise it breaks platforms like Tandem/NSK.  */
> -#if LONG_MAX >> 31 >> 31 == 1
> -# undef int64_t
> +# if LONG_MAX >> 31 >> 31 == 1
> +#  undef int64_t
> typedef long int gl_int64_t;
> -# define int64_t gl_int64_t
> -# define GL_INT64_T
> -#elif defined _MSC_VER
> -# undef int64_t
> +#  define int64_t gl_int64_t
> +#  define GL_INT64_T
> +# elif defined _MSC_VER
> +#  undef int64_t
> typedef __int64 gl_int64_t;
> -# define int64_t gl_int64_t
> -# define GL_INT64_T
> -#elif @HAVE_LONG_LONG_INT@
> -# undef int64_t
> +#  define int64_t gl_int64_t
> +#  define GL_INT64_T
> +# elif @HAVE_LONG_LONG_INT@
> +#  undef int64_t
> typedef long long int gl_int64_t;
> -# define int64_t gl_int64_t
> -# define GL_INT64_T
> +#  define int64_t gl_int64_t
> +#  define GL_INT64_T
> +# endif
> #endif
> 
> -#if ULONG_MAX >> 31 >> 31 >> 1 == 1
> -# undef uint64_t
> -typedef unsigned long int gl_uint64_t;
> -# define uint64_t gl_uint64_t
> +#if UINT64_MAX
> # define GL_UINT64_T
> -#elif defined _MSC_VER
> -# undef uint64_t
> +#else
> +# if ULONG_MAX >> 31 >> 31 >> 1 == 1
> +#  undef uint64_t
> +typedef unsigned long int gl_uint64_t;
> +#  define uint64_t gl_uint64_t
> +#  define GL_UINT64_T
> +# elif defined _MSC_VER
> +#  undef uint64_t
> typedef unsigned __int64 gl_uint64_t;
> -# define uint64_t gl_uint64_t
> -# define GL_UINT64_T
> -#elif @HAVE_UNSIGNED_LONG_LONG_INT@
> -# undef uint64_t
> +#  define uint64_t gl_uint64_t
> +#  define GL_UINT64_T
> +# elif @HAVE_UNSIGNED_LONG_LONG_INT@
> +#  undef uint64_t
> typedef unsigned long long int gl_uint64_t;
> -# define uint64_t gl_uint64_t
> -# define GL_UINT64_T
> +#  define uint64_t gl_uint64_t
> +#  define GL_UINT64_T
> +# endif
> #endif
> 
> /* Avoid collision with Solaris 2.5.1  etc.  */
> @@ -312,17 +325,14 @@ typedef int _verify_intmax_size[sizeof (intmax_t) == 
> sizeof (uintmax_t)
> #define INT32_MAX  2147483647
> #define UINT32_MAX  4294967295U
> 
> -#undef INT64_MIN
> -#undef INT64_MAX
> -#ifdef GL_INT64_T
> +#if defined GL_INT64_T && ! defined INT64_MAX
> /* Prefer (- INTMAX_C (1) << 63) over (~ INT64_MAX) because SunPRO C 5.0
>evaluates the latter incorrectly in preprocessor expressions.  */
> # define INT64_MIN  (- INTMAX_C (1) << 63)
> # define I

Re: gnulib stdint.h substitution of int64_t results in a linking error in GCC 4.(3|2|0) on OSX

[Paul Eggert]

On 11/27/10 00:59, Ralf Wildenhues wrote:
> Should this patch have a similar pendant for the AC_TYPE_INT64_T macro
> in Autoconf?

Offhand I would say not, since that macro tests for int64_t
individually, whereas the problem in gnulib is that all of
stdint.h was being tested collectively.  But perhaps I'm
missing something?

Re: nproc -> LGPL

[Paul Eggert]

On 12/02/10 12:13, Ludovic Courtès wrote:

> I’d like to use ‘nproc’ in libguile, which is LGPLv3+.  Could you change
> the license accordingly?

nproc does feel quite libraryish and it'd be fine with me.
Bruno has contributed the most to it lately, though, and
I'd value his opinion.

gnulib and threaded execution

[Ralf Wildenhues]

The recent coreutils sort bug related to threading made me take a look
at gnulib for similar issues.  When you try to use gnulib in threaded
code, any process-global state can potentially cause problems, whether
that be static data, file descriptor state, current directory, umask,
etc.  For a lot of these data and in a lot of the cases, gnulib is safe.
Still, it might make sense to make an effort to document exceptions to
this rule: users might not even be aware that three levels down the
module dependency tree, they are using something potentially unsafe.

I egrepped for '([   ]static |static [^()]*;)' (TAB inside) and
among the first few hits (of a few hundred) I found these issues:

* Unless STACK_DIRECTION is defined, gnulib/lib/alloca.c sets and uses
STACK_DIR and find_stack_direction:addr in the first alloca call.  When
that first call is from threads, and racing with another one, the value
for STACK_DIRECTION may be computed wrongly, and the code may corrupt
the stack.

This is an issue only when the compiler/libc doesn't provide it, and
even then, the race seems unlikely to be won, so it's not surprising we
haven't seen a report.  Still, this particular issue could be fixed by
computing STACK_DIRECTION from configure iff we choose to compile
alloca.c.

* error_at_line.c has a function-static old_file_name, such that if
error_at_line is called concurrently with a status of 0, the global
error_one_per_line is set to nonzero, and one of the calls passes NULL
as file_name, then NULL may be passed to strcmp in the other thread, if
the race is won.  Less severely, the old_line_number static could cause
the wrong number to be compared, and since it's an int rather than a
sig_atomic_t, in theory it could even contain an inconsistent value at
times.

Arguably, this use case is pretty contrived, since calling error_at_line
concurrently doesn't mesh with wanting only one error per line, but
IMVHO it makes sense to at least document the requirement for the caller
here.

* in getloadavg.c, getloadavg_initialized should probably be a
sig_atomic_t not a bool (no idea whether this can ever be a problem in
practice[1]).

* register_close_hook is not thread-safe.  Doesn't seem a big issue at
first, it's called only from gl_sockets_startup, but there is no hint
attached to the latter function about not being callable from a threaded
context.


I'm sure a number of other issues are lurking there.  The increased use
of LTO (link-time optimization) will surely over time expose more of
these issues (also things like missing volatile) due to the compiler
being able to optimize much more aggressively.

Cheers,
Ralf

Re: nproc -> LGPL

[Bruno Haible]

Paul Eggert wrote:
> > I’d like to use ‘nproc’ in libguile, which is LGPLv3+.  Could you change
> > the license accordingly?
> 
> nproc does feel quite libraryish and it'd be fine with me.

Yes, nproc is libraryish, comparable to parts of the contents of libgomp.
Therefore it's fine with me too. I was about to commit the change, but just
notice that Glenn Lenker's contributions are visible in the ChangeLog but not
in the git history:

   2009-03-25  Paul Eggert  

New modules nproc, pthread, contributed by Glen Lenker.

* MODULES.html.sh: Add pthread, nproc.
* lib/nproc.c: New file.
* lib/nproc.h: New file.

Glenn, do you also agree with Ludo's request?



2010-12-02  Paul Eggert  
Bruno Haible  

nproc: Relax license.
* modules/nproc (License): Change to LGPL.
Requested by Ludovic Courtès .

--- modules/nproc.orig  Thu Dec  2 23:33:49 2010
+++ modules/nproc   Thu Dec  2 23:31:24 2010
@@ -21,7 +21,7 @@
 "nproc.h"
 
 License:
-GPL
+LGPL
 
 Maintainer:
 Glen Lenker and Paul Eggert

Re: [libvirt] make syntax-check -> make: *** [sc_check_author_list] Error 1 on libvirt-0.8.5

[Eric Blake]

[adding bug-gnulib, thanks to some issues with 'make syntax-check' in
gnulib-provided files]

On 12/02/2010 02:25 AM, Kenneth Nagin wrote:
>>> > I am receiving syntax error when compiling libvirt-0.8.5.
>>> > However, make without syntax-check completes successfully.
>>> > 
>>> > check_author_list
>>> > %aE
>>> > maint.mk: committer(s) not listed in AUTHORS
>> That means your version of git is too old to support the specific log
>> formatting directive that we are using.  What version of git are you
>> using, and is it worth us fixing that syntax check to skip if git is too
>> old?

>> And ultimately, failure of 'make syntax-check' is non-fatal; it is not a
>> prerequisite for building working binaries, so much as a way to try and
>> enforce consistent style within the code base.
>>
> I decided that it made more sense to simply work with 0.8.6 (rather than
> 0.8.5).
> But now I am getting another error, i.e "Failed to determine type of
> version control used in /home/nagin/LIBVIRT/libvirt-0.8.6".  It prints it a
> lot of times and then hangs.
> make without syntax-check works fine.  Here are the error messages:

You still haven't told me what 'git --version' outputs on your system.
That may be the clue to solving all of this.

> 
> na...@croton:~/LIBVIRT/libvirt-0.8.6$ make syntax-check
> GFDL_version
> ./build-aux/vc-list-files: Failed to determine type of version control used
> in /home/nagin/LIBVIRT/libvirt-0.8.6

Also caused if git is too old or missing.  What does this output?

sh -vx build-aux/vc-list-files m4

That will help me figure out whether vc-list-files needs to be patched,
and or maint.mk taught to skip tests that require vc-list-files when run
from a tarball rather than from a git checkout.  Ultimately, 'make
syntax-check' is intended for developers working from the latest
development repository, and isn't really needed for end users working
from a tarball.

-- 
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: gnulib and threaded execution

[Paul Eggert]

On 12/02/10 13:47, Ralf Wildenhues wrote:

> * error_at_line.c ...
> IMVHO it makes sense to at least document the requirement for the caller
> here.

Yes.  This is also a restriction for the glibc implementation, no?
So it's fine if gnulib has the same restriction.

> * in getloadavg.c, getloadavg_initialized should probably be a
> sig_atomic_t not a bool (no idea whether this can ever be a problem in
> practice[1]).

In general sig_atomic_t does not guarantee atomic access with C99, any
more than bool does, across threads.  (C0x may be different; I haven't
been following that.)  In practice, loading and storing words tends to
be atomic across threads, if the instructions are not optimized away;
'volatile' may be needed to prevent that, but I don't offhand see why
'volatile' would be required here, even with link-time optimization.

That being said, there are race conditions there, even if one assumes
sig_atomic_t access is atomic across threads, since multiple threads
could be initializing the buffers simultaneously.  Fixing this won't
be trivial.  It may well be better to document getloadavg as not being
thread-safe, for now.

> * register_close_hook is not thread-safe.

Yes, worth documenting.

> I'm sure a number of other issues are lurking there.  The increased use
> of LTO (link-time optimization) will surely over time expose more of
> these issues (also things like missing volatile) due to the compiler
> being able to optimize much more aggressively.

Yes and yes!

> * Unless STACK_DIRECTION is defined, gnulib/lib/alloca.c sets and uses
> STACK_DIR and find_stack_direction:addr in the first alloca call.  When
> that first call is from threads, and racing with another one, the value
> for STACK_DIRECTION may be computed wrongly, and the code may corrupt
> the stack.

This should be fixable by making 'addr' auto instead of static, no?
Something like the following (untested) patch.  There is another race
if loading and storing into an 'int' is not atomic, which I suppose we
could document as an assumption (a safe one, I hope, as noted above).

diff --git a/lib/alloca.c b/lib/alloca.c
index b652765..2f5e27e 100644
--- a/lib/alloca.c
+++ b/lib/alloca.c
@@ -94,21 +94,20 @@ static int stack_dir;   /* 1 or -1 once known.  */
 #   define STACK_DIRstack_dir
 
 static void
-find_stack_direction (void)
+find_stack_direction (char **ptr)
 {
-  static char *addr = NULL; /* Address of first `dummy', once known.  */
   auto char dummy;  /* To get stack address.  */
 
-  if (addr == NULL)
+  if (*ptr == NULL)
 {   /* Initial entry.  */
-  addr = ADDRESS_FUNCTION (dummy);
+  *ptr = ADDRESS_FUNCTION (dummy);
 
-  find_stack_direction ();  /* Recurse once.  */
+  find_stack_direction (ptr);  /* Recurse once.  */
 }
   else
 {
   /* Second entry.  */
-  if (ADDRESS_FUNCTION (dummy) > addr)
+  if (ADDRESS_FUNCTION (dummy) > *ptr)
 stack_dir = 1;  /* Stack grew upward.  */
   else
 stack_dir = -1; /* Stack grew downward.  */
@@ -155,7 +154,10 @@ alloca (size_t size)
 
 #  if STACK_DIRECTION == 0
   if (STACK_DIR == 0)   /* Unknown growth direction.  */
-find_stack_direction ();
+{
+  char *addr = NULL;/* Address of first `dummy', once known.  */
+  find_stack_direction (&addr);
+}
 #  endif
 
   /* Reclaim garbage, defined as all alloca'd storage that

Re: gnulib and threaded execution

[Bruno Haible]

Hi Ralf,

> When you try to use gnulib in threaded
> code, any process-global state can potentially cause problems, whether
> that be static data, file descriptor state, current directory, umask,
> etc.  For a lot of these data and in a lot of the cases, gnulib is safe.
> Still, it might make sense to make an effort to document exceptions to
> this rule: users might not even be aware that three levels down the
> module dependency tree, they are using something potentially unsafe.

Very true. When using fstrcmp in a multithreaded situation in msgmerge,
I had to chase the module dependencies and then look at each involved
source code file. It's doable for fstrcmp(), but when you look at
larger pieces of code, the task becomes daunting.

I would find it useful and systematic if we started to add a
section "Multithreading:" to the module descriptions. Its contents
could be a simple statement like "MT-safe", or a description like
this for iconv_open:
  A conversion descriptor can not be used in multiple threads
  simultaneously.
or for localcharset:
  Depends on global state: The current locale. This function is
  unreliable if setlocale() is called in other threads.

Filling in this information requires a review of the code, looking
at global variables, system calls, and more.

> I egrepped for '([ ]static |static [^()]*;)' (TAB inside) and
> among the first few hits (of a few hundred) I found these issues:

This is just a portion of what needs to be looked at. There's also
the global variables (use 'nm' on the object files to find them),
sigaction() system calls, fork()/exec() calls (which operate in
platform dependent manner if threads are active), and many more.

> * Unless STACK_DIRECTION is defined, gnulib/lib/alloca.c sets and uses
> STACK_DIR and find_stack_direction:addr in the first alloca call.  When
> that first call is from threads, and racing with another one, the value
> for STACK_DIRECTION may be computed wrongly, and the code may corrupt
> the stack.

You're just scratching the surface. More importantly, the 'alloca'
module cannot be used _at_all_ in code meant to be used in multiple
threads.
  1. because the stack size for threads is often smaller than
 the stack size of the main thread. (16 KB vs. 8 MB, or so.)
 You have to use module 'safe-alloca' instead.
  2. because the alloca.c code assumes that there is a "stack
 direction". This is not the case any more with GCC's new
 "split stacks" .

> * error_at_line.c has a function-static old_file_name

Additionally, if error and error_at_line are called from
multiple threads, the output will intermingle on stderr.
So, locking is necessary. In error.c it is not enabled in
gnulib.

> * in getloadavg.c, getloadavg_initialized should probably be a
> sig_atomic_t not a bool (no idea whether this can ever be a problem in
> practice[1]).

I'd say, this looks more like one needs to use a gl_once_t or
pthread_once_t, to guarantee that the initialization is done
at most once (and not started in parallel by several threads
if the second thread arrives when the first thread has started
the initialization and is not yet done with it).

> * register_close_hook is not thread-safe.

Yes, this should be part of the MT related documentation:
which functions are meant to be executed only once, before
anything else.

Bruno

Re: gnulib and threaded execution

[Ben Pfaff]

Bruno Haible  writes:

> I would find it useful and systematic if we started to add a
> section "Multithreading:" to the module descriptions. Its contents
> could be a simple statement like "MT-safe", or a description like
> this for iconv_open:
>   A conversion descriptor can not be used in multiple threads
>   simultaneously.
> or for localcharset:
>   Depends on global state: The current locale. This function is
>   unreliable if setlocale() is called in other threads.
>
> Filling in this information requires a review of the code, looking
> at global variables, system calls, and more.

The individual function descriptions in the manual might be a
good place to document where the thread-safety of a function
departs from that implemented by glibc or specified by POSIX.
-- 
Ben Pfaff 
http://benpfaff.org

Re: gnulib and threaded execution

[Jim Meyering]

Hi Bruno,

Bruno Haible wrote:
> You're just scratching the surface. More importantly, the 'alloca'
> module cannot be used _at_all_ in code meant to be used in multiple
> threads.
>   1. because the stack size for threads is often smaller than
>  the stack size of the main thread. (16 KB vs. 8 MB, or so.)
>  You have to use module 'safe-alloca' instead.
>   2. because the alloca.c code assumes that there is a "stack
>  direction". This is not the case any more with GCC's new
>  "split stacks" .

Is #2 really relevant?
alloca.c is compiled with gcc only for 1.x versions.

Re: gnulib and threaded execution

[Ralf Wildenhues]

* Paul Eggert wrote on Fri, Dec 03, 2010 at 01:39:19AM CET:
> On 12/02/10 13:47, Ralf Wildenhues wrote:
> 
> > * error_at_line.c ...
> > IMVHO it makes sense to at least document the requirement for the caller
> > here.
> 
> Yes.  This is also a restriction for the glibc implementation, no?

Yes.

> So it's fine if gnulib has the same restriction.

Sure, but I think it still deserves to be documented.

> > * Unless STACK_DIRECTION is defined, gnulib/lib/alloca.c sets and uses
> > STACK_DIR and find_stack_direction:addr in the first alloca call.  When
> > that first call is from threads, and racing with another one, the value
> > for STACK_DIRECTION may be computed wrongly, and the code may corrupt
> > the stack.
> 
> This should be fixable by making 'addr' auto instead of static, no?

I think so (and it might be good to adjust the code in Autoconf
likewise).  Of course, if I were a C compiler, ...

>/* Second entry.  */
> -  if (ADDRESS_FUNCTION (dummy) > addr)
> +  if (ADDRESS_FUNCTION (dummy) > *ptr)

I'd just replace this pointer comparison with a random compile-time
constant, seeing that the pointers do not point into (or right above)
the same contiguous piece of memory.  I'm not sure whether any
non-Deathstation compilers would actually do that, though, neither if
making the pointer itself volatile could help.  gcc-4.6 -fsplit-stack
could make this more failure-prone, but let's hope that none of the
systems where -fsplit-stack will work, will ever need gnulib's alloca.

>  stack_dir = 1;  /* Stack grew upward.  */
>else
>  stack_dir = -1; /* Stack grew downward.  */

Cheers,
Ralf