Master is broken

Master is broken

[Eli Zaretskii]

As of commit bb7c182, master is broken, at least for non-optimized
builds:

    CCLD     temacs
  keymap.o: In function `Fset_keymap_parent':
  /srv/data/home/e/eliz/git/emacs/trunk/src/keymap.c:344: undefined reference to `CHECK_IMPURE'
  keymap.o: In function `store_in_keymap':
  /srv/data/home/e/eliz/git/emacs/trunk/src/keymap.c:753: undefined reference to `PURE_P'
  /srv/data/home/e/eliz/git/emacs/trunk/src/keymap.c:801: undefined reference to `CHECK_IMPURE'
  /srv/data/home/e/eliz/git/emacs/trunk/src/keymap.c:854: undefined reference to `CHECK_IMPURE'
  /srv/data/home/e/eliz/git/emacs/trunk/src/keymap.c:898: undefined reference to `CHECK_IMPURE'
  data.o: In function `Fsetcar':
  /srv/data/home/e/eliz/git/emacs/trunk/src/data.c:563: undefined reference to `CHECK_IMPURE'
  data.o: In function `Fsetcdr':
  /srv/data/home/e/eliz/git/emacs/trunk/src/data.c:573: undefined reference to `CHECK_IMPURE'
  data.o:/srv/data/home/e/eliz/git/emacs/trunk/src/data.c:2218: more undefined references to `CHECK_IMPURE' follow
  collect2: error: ld returned 1 exit status
  make[1]: *** [temacs] Error 1

I'm not really sure what's going on here, all this INLINE stuff is too
complicated.  These 2 inline functions are clearly visible in the
preprocessed source, and still the linker barfs.  The only way I could
make it link successfully was by including puresize.h in emacs.c as
well, but then temacs crashed during loadup.

Re: Master is broken

[Andy Moreton]

On Sat 10 Oct 2015, Eli Zaretskii wrote:

> As of commit bb7c182, master is broken, at least for non-optimized
> builds:
>
>     CCLD     temacs
>   keymap.o: In function `Fset_keymap_parent':
>   /srv/data/home/e/eliz/git/emacs/trunk/src/keymap.c:344: undefined reference to `CHECK_IMPURE'
>   keymap.o: In function `store_in_keymap':
>   /srv/data/home/e/eliz/git/emacs/trunk/src/keymap.c:753: undefined reference to `PURE_P'
>   /srv/data/home/e/eliz/git/emacs/trunk/src/keymap.c:801: undefined reference to `CHECK_IMPURE'
>   /srv/data/home/e/eliz/git/emacs/trunk/src/keymap.c:854: undefined reference to `CHECK_IMPURE'
>   /srv/data/home/e/eliz/git/emacs/trunk/src/keymap.c:898: undefined reference to `CHECK_IMPURE'
>   data.o: In function `Fsetcar':
>   /srv/data/home/e/eliz/git/emacs/trunk/src/data.c:563: undefined reference to `CHECK_IMPURE'
>   data.o: In function `Fsetcdr':
>   /srv/data/home/e/eliz/git/emacs/trunk/src/data.c:573: undefined reference to `CHECK_IMPURE'
>   data.o:/srv/data/home/e/eliz/git/emacs/trunk/src/data.c:2218: more undefined references to `CHECK_IMPURE' follow
>   collect2: error: ld returned 1 exit status
>   make[1]: *** [temacs] Error 1
>
> I'm not really sure what's going on here, all this INLINE stuff is too
> complicated.  These 2 inline functions are clearly visible in the
> preprocessed source, and still the linker barfs.  The only way I could
> make it link successfully was by including puresize.h in emacs.c as
> well, but then temacs crashed during loadup.

The comments in the definition of INLINE explain how this is supposed to
work. I see the same errors with mingw64: the patch below fixed
bootstrap for me.

    AndyM

diff --git a/src/emacs.c b/src/emacs.c
index 5a6999d9b1df..3eff5a720eac 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -82,6 +82,7 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include "syntax.h"
 #include "sysselect.h"
 #include "systime.h"
+#include "puresize.h"
 
 #include "gnutls.h"
 
diff --git a/src/puresize.h b/src/puresize.h
index d0926c652135..945471e6044c 100644
--- a/src/puresize.h
+++ b/src/puresize.h
@@ -16,6 +16,11 @@ GNU General Public License for more details.
 You should have received a copy of the GNU General Public License
 along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 
+#ifndef EMACS_PURESIZE_H
+#define EMACS_PURESIZE_H
+
+INLINE_HEADER_BEGIN
+
 /* Define PURESIZE, the number of bytes of pure Lisp code to leave space for.
 
    At one point, this was defined in config.h, meaning that changing
@@ -88,3 +93,8 @@ CHECK_IMPURE (Lisp_Object obj, void *ptr)
   if (PURE_P (ptr))
     pure_write_error (obj);
 }
+
+INLINE_HEADER_END
+
+#endif /* EMACS_PURESIZE_H */
+

Re: Master is broken

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 10 Oct 2015 11:44:24 +0100
> 
> > I'm not really sure what's going on here, all this INLINE stuff is too
> > complicated.  These 2 inline functions are clearly visible in the
> > preprocessed source, and still the linker barfs.  The only way I could
> > make it link successfully was by including puresize.h in emacs.c as
> > well, but then temacs crashed during loadup.
> 
> The comments in the definition of INLINE explain how this is supposed to
> work.

They do?  Then I'm probably missing something, because I cannot find
where it says that emacs.c should include any header that uses INLINE.
(Do you mean the comments in conf_post.h?)  I arrived at that by using
"nm -A" on the various *.o files that use INLINE, looking for *.o
files that define those symbols.

> I see the same errors with mingw64: the patch below fixed
> bootstrap for me.

I installed the change in emacs.c recently.  The change in puresize.h
is not needed to fix the problem, neither on GNU/Linux nor in the
mingw32 build.  Are you saying that without the changes in puresize.h
you cannot compile/link the current master?  If so, please show the
error messages.

Thanks.

Re: Master is broken

[Andy Moreton]

On Sat 10 Oct 2015, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sat, 10 Oct 2015 11:44:24 +0100
>> 
>> > I'm not really sure what's going on here, all this INLINE stuff is too
>> > complicated.  These 2 inline functions are clearly visible in the
>> > preprocessed source, and still the linker barfs.  The only way I could
>> > make it link successfully was by including puresize.h in emacs.c as
>> > well, but then temacs crashed during loadup.
>> 
>> The comments in the definition of INLINE explain how this is supposed to
>> work.
>
> They do?  Then I'm probably missing something, because I cannot find
> where it says that emacs.c should include any header that uses INLINE.
> (Do you mean the comments in conf_post.h?)  I arrived at that by using
> "nm -A" on the various *.o files that use INLINE, looking for *.o
> files that define those symbols.

Yes, the comments in conf_post.h, which I assume come from a gnulib
module. That shows that one file should contain '#define INLINE
EXTERN_INLINE' to ensure that one translation unit contains a
non-inlined definition of each function declared INLINE.

>> I see the same errors with mingw64: the patch below fixed
>> bootstrap for me.
>
> I installed the change in emacs.c recently.  The change in puresize.h
> is not needed to fix the problem, neither on GNU/Linux nor in the
> mingw32 build.  Are you saying that without the changes in puresize.h
> you cannot compile/link the current master?  If so, please show the
> error messages.

Bootstrap of commit f655d09fd5b4 works for me, so the other changes appear
not to be needed. Paul Eggert will know what the correct fix should be.

    AndyM

Re: Master is broken

[Eli Zaretskii]

> Date: Sat, 10 Oct 2015 11:47:38 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
>   data.o: In function `Fsetcdr':
>   /srv/data/home/e/eliz/git/emacs/trunk/src/data.c:573: undefined reference to `CHECK_IMPURE'
>   data.o:/srv/data/home/e/eliz/git/emacs/trunk/src/data.c:2218: more undefined references to `CHECK_IMPURE' follow
>   collect2: error: ld returned 1 exit status
>   make[1]: *** [temacs] Error 1
> 
> I'm not really sure what's going on here, all this INLINE stuff is too
> complicated.  These 2 inline functions are clearly visible in the
> preprocessed source, and still the linker barfs.  The only way I could
> make it link successfully was by including puresize.h in emacs.c as
> well, but then temacs crashed during loadup.

With the fix by Andreas in 8f41c30, including puresize.h in emacs.c
now seems to DTRT, so I committed that change.

Btw, Andreas, don't we need a similar change (i.e. use XPNTR in the
second argument of CHECK_IMPURE) in the other places where
CHECK_IMPURE is called as well?  If not, can you explain why XVECTOR
is incorrect there, but XCONS is correct?

Thanks.

Re: Master is broken

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> Btw, Andreas, don't we need a similar change (i.e. use XPNTR in the
> second argument of CHECK_IMPURE) in the other places where
> CHECK_IMPURE is called as well?  If not, can you explain why XVECTOR
> is incorrect there, but XCONS is correct?

A CONSP is always a CONSP.  But an ARRAYP is a VECTORP or a STRINGP or a
CHAR_TABLE_P or a BOOL_VECTOR_P.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Master is broken

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Sat, 10 Oct 2015 17:59:57 +0200
> 
> A CONSP is always a CONSP.  But an ARRAYP is a VECTORP or a STRINGP or a
> CHAR_TABLE_P or a BOOL_VECTOR_P.

Light goes on.  Thanks.

Re: Master is broken

[Paul Eggert]

Eli Zaretskii wrote:
> With the fix by Andreas in 8f41c30, including puresize.h in emacs.c
> now seems to DTRT, so I committed that change.

Thanks.  The problem did not occur with my build because the function was 
inlined everywhere it was used.

Re: Master is broken

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 10 Oct 2015 09:46:58 -0700
> 
> Eli Zaretskii wrote:
> > With the fix by Andreas in 8f41c30, including puresize.h in emacs.c
> > now seems to DTRT, so I committed that change.
> 
> Thanks.  The problem did not occur with my build because the function was 
> inlined everywhere it was used.

Is it true that any header that defines INLINE functions should be
included by emacs.c (and if it is used in lib-src programs, also in
their source file that defines INLINE to EXTERN_INLINE)?  If so, I'd
like to add this to the commentary in conf_post.h.

Re: Master is broken

[Eli Zaretskii]

> Date: Sat, 10 Oct 2015 20:06:25 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: schwab@suse.de, emacs-devel@gnu.org
> 
> Is it true that any header that defines INLINE functions should be
> included by emacs.c (and if it is used in lib-src programs, also in
> their source file that defines INLINE to EXTERN_INLINE)?

Also, when exactly are INLINE_HEADER_BEGIN/END required?  puresize.h
doesn't have it, and still compiles; other headers which use INLINE do
have INLINE_HEADER_BEGIN/END.  What factor determines whether they are
absolutely required?

Thanks.

Re: Master is broken

[Paul Eggert]

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

Eli Zaretskii wrote:
> Also, when exactly are INLINE_HEADER_BEGIN/END required?  puresize.h
> doesn't have it, and still compiles; other headers which use INLINE do
> have INLINE_HEADER_BEGIN/END.  What factor determines whether they are
> absolutely required?

They're always needed, if you want to build with --enable-gcc-warnings and use 
GCC older than 5.1.  I hope we can drop them once we assume that people who want 
picky warnings are using GCC 5.1 or later.  I forgot to add them to puresize.h 
since I use GCC 5.2; nobody else has needed them yet, I guess, since nobody with 
older compilers uses --enable-gcc-warnings.  I fixed this with the attached 
additional patch.

[-- Attachment #2: 0001-Fix-enable-gcc-warnings-problem-with-older-GCC.patch --]
[-- Type: text/plain, Size: 1778 bytes --]

From 1b0ab7d132f4b6956b2061a550e533833b0bdc68 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 10 Oct 2015 11:56:20 -0700
Subject: [PATCH] Fix --enable-gcc-warnings problem with older GCC

* src/puresize.h: Add INLINE_HEADER_BEGIN, INLINE_HEADER_END.
This is for building with --enable-gcc-warnings with
GCC 4.6 through 5.0.
---
 src/conf_post.h | 5 +++++
 src/puresize.h  | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/src/conf_post.h b/src/conf_post.h
index 785e5d7..2c3eee5 100644
--- a/src/conf_post.h
+++ b/src/conf_post.h
@@ -316,6 +316,11 @@ extern int emacs_setenv_TZ (char const *);
 
    before including config.h or any other .h file.
    Other .c files should not define INLINE.
+   For Emacs, this is done by having emacs.c first '#define INLINE
+   EXTERN_INLINE' and then include every .h file that uses INLINE.
+
+   The INLINE_HEADER_BEGIN and INLINE_HEADER_END suppress bogus
+   warnings in some GCC versions; see ../m4/extern-inline.m4.
 
    C99 compilers compile functions like 'incr' as C99-style extern
    inline functions.  Buggy GCC implementations do something similar with
diff --git a/src/puresize.h b/src/puresize.h
index d0926c6..c61b31f 100644
--- a/src/puresize.h
+++ b/src/puresize.h
@@ -16,6 +16,8 @@ GNU General Public License for more details.
 You should have received a copy of the GNU General Public License
 along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 
+INLINE_HEADER_BEGIN
+
 /* Define PURESIZE, the number of bytes of pure Lisp code to leave space for.
 
    At one point, this was defined in config.h, meaning that changing
@@ -88,3 +90,5 @@ CHECK_IMPURE (Lisp_Object obj, void *ptr)
   if (PURE_P (ptr))
     pure_write_error (obj);
 }
+
+INLINE_HEADER_END
-- 
2.1.4

Re: Master is broken

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 10 Oct 2015 12:01:29 -0700
> Cc: schwab@suse.de, emacs-devel@gnu.org
> 
> Eli Zaretskii wrote:
> > Also, when exactly are INLINE_HEADER_BEGIN/END required?  puresize.h
> > doesn't have it, and still compiles; other headers which use INLINE do
> > have INLINE_HEADER_BEGIN/END.  What factor determines whether they are
> > absolutely required?
> 
> They're always needed, if you want to build with --enable-gcc-warnings and use 
> GCC older than 5.1.  I hope we can drop them once we assume that people who want 
> picky warnings are using GCC 5.1 or later.  I forgot to add them to puresize.h 
> since I use GCC 5.2; nobody else has needed them yet, I guess, since nobody with 
> older compilers uses --enable-gcc-warnings.  I fixed this with the attached 
> additional patch.

Thanks.

Don't we need to make puresize.h idempotent now?

Re: Master is broken

[Paul Eggert]

Eli Zaretskii wrote:
> Don't we need to make puresize.h idempotent now?

That's not required, since puresize.h is never included more than once.

Re: Master is broken

[Juanma Barranquero]

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

On Sat, Oct 10, 2015 at 9:01 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:

> nobody else has needed them yet, I guess, since nobody with older
compilers
> uses --enable-gcc-warnings.

Depend on what you mean by "older". I build with 4.8.1 and
--enable-gcc-warnings.

[-- Attachment #2: Type: text/html, Size: 400 bytes --]

Re: Master is broken

[Paul Eggert]

Juanma Barranquero wrote:
> I build with 4.8.1 and --enable-gcc-warnings.

I was doing the same until a couple of years ago.  This combination should still 
work.  That being said, it's not important for --enable-gcc-warnings to support 
older GCCs, and I don't plan to spend much time supporting them.  Another thing: 
I don't bother with --enable-gcc-warnings on platforms other than x86-64, as 
there are just too many false alarms.