bug#7859: syncing Emacs from sources maintained elsewhere

bug#7859: syncing Emacs from sources maintained elsewhere

[Paul Eggert]

On 01/18/2011 05:20 PM, Glenn Morris wrote:

> the Mac (nextstep) port seems unhappy with the changes:
> 
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7856
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7859
> 
> [compiling unexmacosx.c]
> In file included from ../lib/unistd.h:110,
>                  from unexmacosx.c:94:
> ../lib/getopt.h:196: error: redefinition of 'struct option'

Thanks for mentioning that, as I don't follow the bug list.
This almost certainly seems to be a bug with gnulib's treatment
of getopt.h and unistd.h.

gnulib's unistd.h includes its getopt.h, which defines 'struct
option'.  I suspect the problem is that MacOS X already defines
'struct option' somewhere; or perhaps somehow we're including both the
MacOS X 'getopt.h' and Emacs's getopt.h.

What does src/config.h #define __GETOPT_PREFIX to?

Would it be possible to send the output of the command of "gcc -E",
e.g., something like this (substituting the -I options that the failed
compile used)?

  gcc -E -Demacs -DHAVE_CONFIG_H -I. -I/usr/local/emacs/trunk/src -I../lib \
    -I/usr/local/emacs/trunk/src/../lib -I/opt/local/include/libxml2 \
    -I/opt/local/include/dbus-1.0 -I/opt/local/lib/dbus-1.0/include \
    -I/opt/local/include \
    unexmacosx.c

That would help us find out which earlier header file is defining
'struct option'.  Also, it might be helpful to know what src/config.h
looks like.  Unfortunately I do not have access to a Mac OS X host
so I can't debug this directly.

bug#7856: syncing Emacs from sources maintained elsewhere

[Paul Eggert]

On 01/18/2011 05:20 PM, Glenn Morris wrote:

> the Mac (nextstep) port seems unhappy with the changes:
> 
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7856
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7859
> 
> [compiling unexmacosx.c]
> In file included from ../lib/unistd.h:110,
>                  from unexmacosx.c:94:
> ../lib/getopt.h:196: error: redefinition of 'struct option'

Thanks for mentioning that, as I don't follow the bug list.
This almost certainly seems to be a bug with gnulib's treatment
of getopt.h and unistd.h.

gnulib's unistd.h includes its getopt.h, which defines 'struct
option'.  I suspect the problem is that MacOS X already defines
'struct option' somewhere; or perhaps somehow we're including both the
MacOS X 'getopt.h' and Emacs's getopt.h.

What does src/config.h #define __GETOPT_PREFIX to?

Would it be possible to send the output of the command of "gcc -E",
e.g., something like this (substituting the -I options that the failed
compile used)?

  gcc -E -Demacs -DHAVE_CONFIG_H -I. -I/usr/local/emacs/trunk/src -I../lib \
    -I/usr/local/emacs/trunk/src/../lib -I/opt/local/include/libxml2 \
    -I/opt/local/include/dbus-1.0 -I/opt/local/lib/dbus-1.0/include \
    -I/opt/local/include \
    unexmacosx.c

That would help us find out which earlier header file is defining
'struct option'.  Also, it might be helpful to know what src/config.h
looks like.  Unfortunately I do not have access to a Mac OS X host
so I can't debug this directly.

bug#7856: syncing Emacs from sources maintained elsewhere

[Paul Eggert]

I got a copy of preprocessed sources from bryan d. o'connor.
Here's what seems to be the problem:

* <config.h> has "#define __GETOPT_PREFIX rpl_" as expected.

* unexmacosx.c includes the Gnulib <unistd.h>.

* The Gnulib <unistd.h> includes the system unistd.h, which declares
  the getopt-related names itself (without including anything) and then finishes.

* The Gnulib <unistd.h> then includes the Gnulib <getopt.h>.

* The Gnulib <getopt.h> includes the system getopt.h.

* The system getopt.h includes the Gnulib <unistd.h>.

* This inner Gnulib <unistd.h> does nothing, and finishes.

* The system getopt.h declares 'struct option' and other stuff, and finishes.

* The Gnulib <getopt.h> then does this:

     #if defined __GETOPT_PREFIX && !defined __need_getopt
     # if !@HAVE_GETOPT_H@
     #  include <stdlib.h>
     #  include <stdio.h>
     #  include <unistd.h>
     # endif
     # undef __need_getopt
     ...
     # undef option
     # define __GETOPT_CONCAT(x, y) x ## y
     # define __GETOPT_XCONCAT(x, y) __GETOPT_CONCAT (x, y)
     # define __GETOPT_ID(y) __GETOPT_XCONCAT (__GETOPT_PREFIX, y)
     ...
     # define option __GETOPT_ID (option)
     ...
     #endif

* This should "#define option rpl_option", but apparently it doesn't.

* Presumably __GETOPT_PREFIX is defined, but __need_getopt is also defined,
  so the entire #if is skipped.

* And when Gnulib <getopt.h> declares a 'struct option', this clashes
  with the 'struct option' declared in the system getopt.h.
       
If my analysis is right, we need to figure out why __need_getopt is
defined.  Can you send the output of the following?

cd emacs
grep -r __need_getopt . /usr/include


PS.  This all seems to be a case where we should be using Gnulib's
identifier replacement mechanism, but I don't understand it that
well and if memory serves it doesn't work with struct tags anyway.

bug#7859: syncing Emacs from sources maintained elsewhere

[Paul Eggert]

I got a copy of preprocessed sources from bryan d. o'connor.
Here's what seems to be the problem:

* <config.h> has "#define __GETOPT_PREFIX rpl_" as expected.

* unexmacosx.c includes the Gnulib <unistd.h>.

* The Gnulib <unistd.h> includes the system unistd.h, which declares
  the getopt-related names itself (without including anything) and then finishes.

* The Gnulib <unistd.h> then includes the Gnulib <getopt.h>.

* The Gnulib <getopt.h> includes the system getopt.h.

* The system getopt.h includes the Gnulib <unistd.h>.

* This inner Gnulib <unistd.h> does nothing, and finishes.

* The system getopt.h declares 'struct option' and other stuff, and finishes.

* The Gnulib <getopt.h> then does this:

     #if defined __GETOPT_PREFIX && !defined __need_getopt
     # if !@HAVE_GETOPT_H@
     #  include <stdlib.h>
     #  include <stdio.h>
     #  include <unistd.h>
     # endif
     # undef __need_getopt
     ...
     # undef option
     # define __GETOPT_CONCAT(x, y) x ## y
     # define __GETOPT_XCONCAT(x, y) __GETOPT_CONCAT (x, y)
     # define __GETOPT_ID(y) __GETOPT_XCONCAT (__GETOPT_PREFIX, y)
     ...
     # define option __GETOPT_ID (option)
     ...
     #endif

* This should "#define option rpl_option", but apparently it doesn't.

* Presumably __GETOPT_PREFIX is defined, but __need_getopt is also defined,
  so the entire #if is skipped.

* And when Gnulib <getopt.h> declares a 'struct option', this clashes
  with the 'struct option' declared in the system getopt.h.
       
If my analysis is right, we need to figure out why __need_getopt is
defined.  Can you send the output of the following?

cd emacs
grep -r __need_getopt . /usr/include


PS.  This all seems to be a case where we should be using Gnulib's
identifier replacement mechanism, but I don't understand it that
well and if memory serves it doesn't work with struct tags anyway.

bug#7859: syncing Emacs from sources maintained elsewhere

[Jan Djärv]

Paul Eggert skrev 2011-01-19 11.38:
> grep -r __need_getopt . /usr/include


grep -nr __need_getopt . /usr/include
./lib/getopt.h:38:#ifndef __need_getopt
./lib/getopt.h:51:#if defined __GETOPT_PREFIX && !defined __need_getopt
./lib/getopt.h:57:# undef __need_getopt
./lib/getopt.h:86:   This used to be `#if defined __GETOPT_PREFIX && !defined 
__need_getopt',
./lib/getopt.h:89:   __need_getopt.
./lib/getopt.h:93:   only if __need_getopt is not defined, so it is quite safe 
to rewrite
./lib/getopt.h:96:#if !defined __need_getopt
./lib/getopt.h:173:#ifndef __need_getopt
./lib/getopt.h:241:#ifndef __need_getopt
./lib/getopt.h:258:#undef __need_getopt
./lib/getopt.in.h:37:#ifndef __need_getopt
./lib/getopt.in.h:50:#if defined __GETOPT_PREFIX && !defined __need_getopt
./lib/getopt.in.h:56:# undef __need_getopt
./lib/getopt.in.h:85:   This used to be `#if defined __GETOPT_PREFIX && 
!defined __need_getopt',
./lib/getopt.in.h:88:   __need_getopt.
./lib/getopt.in.h:92:   only if __need_getopt is not defined, so it is quite 
safe to rewrite
./lib/getopt.in.h:95:#if !defined __need_getopt
./lib/getopt.in.h:162:#ifndef __need_getopt
./lib/getopt.in.h:230:#ifndef __need_getopt
./lib/getopt.in.h:247:#undef __need_getopt

	Jan D.

bug#7856: bug#7859: syncing Emacs from sources maintained elsewhere

[Jan Djärv]

Paul Eggert skrev 2011-01-19 11.38:
> grep -r __need_getopt . /usr/include


grep -nr __need_getopt . /usr/include
./lib/getopt.h:38:#ifndef __need_getopt
./lib/getopt.h:51:#if defined __GETOPT_PREFIX && !defined __need_getopt
./lib/getopt.h:57:# undef __need_getopt
./lib/getopt.h:86:   This used to be `#if defined __GETOPT_PREFIX && !defined 
__need_getopt',
./lib/getopt.h:89:   __need_getopt.
./lib/getopt.h:93:   only if __need_getopt is not defined, so it is quite safe 
to rewrite
./lib/getopt.h:96:#if !defined __need_getopt
./lib/getopt.h:173:#ifndef __need_getopt
./lib/getopt.h:241:#ifndef __need_getopt
./lib/getopt.h:258:#undef __need_getopt
./lib/getopt.in.h:37:#ifndef __need_getopt
./lib/getopt.in.h:50:#if defined __GETOPT_PREFIX && !defined __need_getopt
./lib/getopt.in.h:56:# undef __need_getopt
./lib/getopt.in.h:85:   This used to be `#if defined __GETOPT_PREFIX && 
!defined __need_getopt',
./lib/getopt.in.h:88:   __need_getopt.
./lib/getopt.in.h:92:   only if __need_getopt is not defined, so it is quite 
safe to rewrite
./lib/getopt.in.h:95:#if !defined __need_getopt
./lib/getopt.in.h:162:#ifndef __need_getopt
./lib/getopt.in.h:230:#ifndef __need_getopt
./lib/getopt.in.h:247:#undef __need_getopt

	Jan D.

bug#7856: bug#7859: syncing Emacs from sources maintained elsewhere

[Paul Eggert]

I see that Jan Djärv tracked the problem down:
unexmacosx.c included <unistd.h> before <config.h>,
and this caused the problem.  Generally speaking, <config.h>
should be included before all other files, since it
defines macros such as _FILE_OFFSET_BITS that may affect
the ABI, and compiling files sometimes with one ABI and
sometimes with another can lead to disaster.

I see that unexmacos.c still doesn't include <config.h> first,
though.  I suggest that it be changed to do that.

A quick scan for other files with the problem show that
the following files probably need the "#include <config.h>"
moved to the top too:

  w32.c
  w32proc.c

bug#7859: syncing Emacs from sources maintained elsewhere

[Paul Eggert]

I see that Jan Djärv tracked the problem down:
unexmacosx.c included <unistd.h> before <config.h>,
and this caused the problem.  Generally speaking, <config.h>
should be included before all other files, since it
defines macros such as _FILE_OFFSET_BITS that may affect
the ABI, and compiling files sometimes with one ABI and
sometimes with another can lead to disaster.

I see that unexmacos.c still doesn't include <config.h> first,
though.  I suggest that it be changed to do that.

A quick scan for other files with the problem show that
the following files probably need the "#include <config.h>"
moved to the top too:

  w32.c
  w32proc.c

bug#7859: syncing Emacs from sources maintained elsewhere

[Eli Zaretskii]

> Date: Wed, 19 Jan 2011 15:44:40 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: 7859@debbugs.gnu.org, bug-gnulib <bug-gnulib@gnu.org>, 7856@debbugs.gnu.org,
> 	emacs-devel@gnu.org
> 
> A quick scan for other files with the problem show that
> the following files probably need the "#include <config.h>"
> moved to the top too:
> 
>   w32.c
>   w32proc.c

These two have a comment saying that this is done on purpose:

  /* must include CRT headers *before* config.h */

If someone wants to dig into the reasons, find out why and fix that,
please do.  I had my share of fighting with (and being defeated by)
the order of included headers on Windows, so I'm not volunteering.

bug#7859: syncing Emacs from sources maintained elsewhere

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

On 01/19/2011 03:38 AM, Paul Eggert wrote:
> * unexmacosx.c includes the Gnulib <unistd.h>.

That's always a bug to include any gnulib-provided header prior to
<config.h>.

> 
> * And when Gnulib <getopt.h> declares a 'struct option', this clashes
>   with the 'struct option' declared in the system getopt.h.

And this is a known symptom of violating the above constraint.

>        
> PS.  This all seems to be a case where we should be using Gnulib's
> identifier replacement mechanism, but I don't understand it that
> well and if memory serves it doesn't work with struct tags anyway.

We've managed to modernize other headers to properly declare replacement
structs exactly once; it is merely a matter of someone having enough
time to modernize the gnulib getopt module to use modern constructs
rather than abusing config.h to define an rpl_ prefix.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

bug#7856: syncing Emacs from sources maintained elsewhere

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

On 01/19/2011 03:38 AM, Paul Eggert wrote:
> * unexmacosx.c includes the Gnulib <unistd.h>.

That's always a bug to include any gnulib-provided header prior to
<config.h>.

> 
> * And when Gnulib <getopt.h> declares a 'struct option', this clashes
>   with the 'struct option' declared in the system getopt.h.

And this is a known symptom of violating the above constraint.

>        
> PS.  This all seems to be a case where we should be using Gnulib's
> identifier replacement mechanism, but I don't understand it that
> well and if memory serves it doesn't work with struct tags anyway.

We've managed to modernize other headers to properly declare replacement
structs exactly once; it is merely a matter of someone having enough
time to modernize the gnulib getopt module to use modern constructs
rather than abusing config.h to define an rpl_ prefix.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

bug#7859: syncing Emacs from sources maintained elsewhere

[Jan Djärv]

Paul Eggert skrev 2011-01-20 00.44:
> I see that Jan Djärv tracked the problem down:
> unexmacosx.c included<unistd.h>  before<config.h>,
> and this caused the problem.  Generally speaking,<config.h>
> should be included before all other files, since it
> defines macros such as _FILE_OFFSET_BITS that may affect
> the ABI, and compiling files sometimes with one ABI and
> sometimes with another can lead to disaster.
>
> I see that unexmacos.c still doesn't include<config.h>  first,
> though.  I suggest that it be changed to do that.
>

Not possible, I added this comment:

/* config.h #define:s malloc/realloc/free and then includes stdlib.h.
    We want the undefined versions, but if config.h includes stdlib.h
    with the #define:s in place, the prototypes will be wrong and we get
    warnings.  To prevent that, include stdlib.h before config.h.  */

	Jan D.

bug#7856: bug#7859: syncing Emacs from sources maintained elsewhere

[Jan Djärv]

Paul Eggert skrev 2011-01-20 00.44:
> I see that Jan Djärv tracked the problem down:
> unexmacosx.c included<unistd.h>  before<config.h>,
> and this caused the problem.  Generally speaking,<config.h>
> should be included before all other files, since it
> defines macros such as _FILE_OFFSET_BITS that may affect
> the ABI, and compiling files sometimes with one ABI and
> sometimes with another can lead to disaster.
>
> I see that unexmacos.c still doesn't include<config.h>  first,
> though.  I suggest that it be changed to do that.
>

Not possible, I added this comment:

/* config.h #define:s malloc/realloc/free and then includes stdlib.h.
    We want the undefined versions, but if config.h includes stdlib.h
    with the #define:s in place, the prototypes will be wrong and we get
    warnings.  To prevent that, include stdlib.h before config.h.  */

	Jan D.

bug#7856: bug#7859: syncing Emacs from sources maintained elsewhere

[Paul Eggert]

On 01/19/2011 10:45 PM, Jan Djärv wrote:
> /* config.h #define:s malloc/realloc/free and then includes stdlib.h.
>    We want the undefined versions, but if config.h includes stdlib.h
>    with the #define:s in place, the prototypes will be wrong and we get
>    warnings.  To prevent that, include stdlib.h before config.h.  */

OK, I see.  There's a better way, one that doesn't potentially mess up
the ABI.  Namely, Emacs can supply a stdlib.h wrapper that #defines
malloc/realloc/free the way we like.  gnulib already does
that, but Emacs doesn't use gnulib's stdlib.h wrapper.
At some point, perhaps Emacs will, and then we can look into
this issue again.

Is stdlib.h the only include file that has the problem?
If so, I suggest modifying unexmacosx.c to include <stdlib.h>
first, <config.h> second, and everything else after that.
That should lessen the likelihood of an ABI mismatch.

If not, then what other include files have the problem?
This issue may come up if we add more wrappers from gnulib.

bug#7859: syncing Emacs from sources maintained elsewhere

[Paul Eggert]

On 01/19/2011 10:45 PM, Jan Djärv wrote:
> /* config.h #define:s malloc/realloc/free and then includes stdlib.h.
>    We want the undefined versions, but if config.h includes stdlib.h
>    with the #define:s in place, the prototypes will be wrong and we get
>    warnings.  To prevent that, include stdlib.h before config.h.  */

OK, I see.  There's a better way, one that doesn't potentially mess up
the ABI.  Namely, Emacs can supply a stdlib.h wrapper that #defines
malloc/realloc/free the way we like.  gnulib already does
that, but Emacs doesn't use gnulib's stdlib.h wrapper.
At some point, perhaps Emacs will, and then we can look into
this issue again.

Is stdlib.h the only include file that has the problem?
If so, I suggest modifying unexmacosx.c to include <stdlib.h>
first, <config.h> second, and everything else after that.
That should lessen the likelihood of an ABI mismatch.

If not, then what other include files have the problem?
This issue may come up if we add more wrappers from gnulib.

bug#7859: syncing Emacs from sources maintained elsewhere

[Jan D.]

Paul Eggert skrev 2011-01-20 08:51:
> On 01/19/2011 10:45 PM, Jan Djärv wrote:
>> /* config.h #define:s malloc/realloc/free and then includes stdlib.h.
>>     We want the undefined versions, but if config.h includes stdlib.h
>>     with the #define:s in place, the prototypes will be wrong and we get
>>     warnings.  To prevent that, include stdlib.h before config.h.  */
>
> OK, I see.  There's a better way, one that doesn't potentially mess up
> the ABI.  Namely, Emacs can supply a stdlib.h wrapper that #defines
> malloc/realloc/free the way we like.  gnulib already does
> that, but Emacs doesn't use gnulib's stdlib.h wrapper.
> At some point, perhaps Emacs will, and then we can look into
> this issue again.
>

It is just for unexmacosx.c we want undefined malloc/free/realloc.
For other files we wan't the defined ones.
We can tweek the m/darwin.h file instead where these defines are.

	Jan D.

bug#7856: bug#7859: syncing Emacs from sources maintained elsewhere

[Jan D.]

Paul Eggert skrev 2011-01-20 08:51:
> On 01/19/2011 10:45 PM, Jan Djärv wrote:
>> /* config.h #define:s malloc/realloc/free and then includes stdlib.h.
>>     We want the undefined versions, but if config.h includes stdlib.h
>>     with the #define:s in place, the prototypes will be wrong and we get
>>     warnings.  To prevent that, include stdlib.h before config.h.  */
>
> OK, I see.  There's a better way, one that doesn't potentially mess up
> the ABI.  Namely, Emacs can supply a stdlib.h wrapper that #defines
> malloc/realloc/free the way we like.  gnulib already does
> that, but Emacs doesn't use gnulib's stdlib.h wrapper.
> At some point, perhaps Emacs will, and then we can look into
> this issue again.
>

It is just for unexmacosx.c we want undefined malloc/free/realloc.
For other files we wan't the defined ones.
We can tweek the m/darwin.h file instead where these defines are.

	Jan D.