bug#27346: module tests fail to compile with gcc 4.8.5

bug#27346: module tests fail to compile with gcc 4.8.5

[Glenn Morris]

Package: emacs
Version: 26.0.50

Current master on rhel7.3.
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-11)

After building with-modules:
    cd test/data/emacs-module
    make
    
    gcc -std=gnu11 -I../../../src -fPIC -g3 -O2 -c -o mod-test.o mod-test.c
    In file included from mod-test.c:23:0:
    ../../../src/emacs-module.h:38:47: error: missing binary operator before token "("
     #if defined __has_attribute && __has_attribute(__nonnull__)
                                                   ^
    make: *** [mod-test.o] Error 1

bug#27346: module tests fail to compile with gcc 4.8.5

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Mon, 12 Jun 2017 13:37:18 -0400
> 
> Current master on rhel7.3.
> gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-11)
> 
> After building with-modules:
>     cd test/data/emacs-module
>     make
>     
>     gcc -std=gnu11 -I../../../src -fPIC -g3 -O2 -c -o mod-test.o mod-test.c
>     In file included from mod-test.c:23:0:
>     ../../../src/emacs-module.h:38:47: error: missing binary operator before token "("
>      #if defined __has_attribute && __has_attribute(__nonnull__)

I think the "if defined __has_attribute &&" part is not needed, since
conf_post.h defines it fr compilers that lack it.  Can you try that?

bug#27346: module tests fail to compile with gcc 4.8.5

[Glenn Morris]

Eli Zaretskii wrote:

> I think the "if defined __has_attribute &&" part is not needed, since
> conf_post.h defines it fr compilers that lack it.  Can you try that?

emacs-module.h does not include config.h, and I don't know if it should.
(I see no src/*.h file includes config.h, except cygw32.h.
Should it? It could easily be changed not to.)

If not, then I guess something like the following is needed:

--- a/src/emacs-module.h
+++ b/src/emacs-module.h
@@ -35,9 +35,12 @@
 # define EMACS_NOEXCEPT
 #endif
 
-#if defined __has_attribute && __has_attribute(__nonnull__)
+#ifdef __has_attribute
+#if __has_attribute(__nonnull__)
 # define EMACS_ATTRIBUTE_NONNULL(...)
 __attribute__((__nonnull__(__VA_ARGS__)))
-#else
+#endif
+#endif
+#ifndef EMACS_ATTRIBUTE_NONNULL
 # define EMACS_ATTRIBUTE_NONNULL(...)
 #endif


(Even with this change or your suggestion, I then get "Module initialization
failed" when running the tests, but that's probably a different issue.)

bug#27346: module tests fail to compile with gcc 4.8.5

[Philipp Stephani]

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

Glenn Morris <rgm@gnu.org> schrieb am Mo., 12. Juni 2017 um 22:30 Uhr:

> Eli Zaretskii wrote:
>
> > I think the "if defined __has_attribute &&" part is not needed, since
> > conf_post.h defines it fr compilers that lack it.  Can you try that?
>
> emacs-module.h does not include config.h, and I don't know if it should.
>
>
No, emacs-module.h cannot include any headers except standard C headers.


>
> If not, then I guess something like the following is needed:
>
> --- a/src/emacs-module.h
> +++ b/src/emacs-module.h
> @@ -35,9 +35,12 @@
>  # define EMACS_NOEXCEPT
>  #endif
>
> -#if defined __has_attribute && __has_attribute(__nonnull__)
> +#ifdef __has_attribute
> +#if __has_attribute(__nonnull__)
>  # define EMACS_ATTRIBUTE_NONNULL(...)
>  __attribute__((__nonnull__(__VA_ARGS__)))
> -#else
> +#endif
> +#endif
> +#ifndef EMACS_ATTRIBUTE_NONNULL
>  # define EMACS_ATTRIBUTE_NONNULL(...)
>  #endif
>
>
>
Probably yes, thanks. (I don't know why the && expression doesn't work.)


> (Even with this change or your suggestion, I then get "Module
> initialization
> failed" when running the tests, but that's probably a different issue.)
>

Do you get a more detailed error message?

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

bug#27346: module tests fail to compile with gcc 4.8.5

[Glenn Morris]

Philipp Stephani wrote:

>> +#ifdef __has_attribute
>> +#if __has_attribute(__nonnull__)
>>  # define EMACS_ATTRIBUTE_NONNULL(...)
>>  __attribute__((__nonnull__(__VA_ARGS__)))
>> -#else
>> +#endif
>> +#endif
>> +#ifndef EMACS_ATTRIBUTE_NONNULL
>>  # define EMACS_ATTRIBUTE_NONNULL(...)
>>  #endif

Applied as 69899d4.

> Probably yes, thanks. (I don't know why the && expression doesn't work.)

I could not find a good reference, but see eg
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36453#c3

>> (Even with this change or your suggestion, I then get "Module
>> initialization
>> failed" when running the tests, but that's probably a different issue.)
>
> Do you get a more detailed error message?

It was the off-by-one thing you just fixed.

bug#27346: module tests fail to compile with gcc 4.8.5

[Philipp Stephani]

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

Glenn Morris <rgm@gnu.org> schrieb am Mo., 12. Juni 2017 um 23:46 Uhr:

> Philipp Stephani wrote:
>
> >> (Even with this change or your suggestion, I then get "Module
> >> initialization
> >> failed" when running the tests, but that's probably a different issue.)
> >
> > Do you get a more detailed error message?
>
> It was the off-by-one thing you just fixed.
>

Thanks. I've now also added some more informational messages.
Two smaller things:
- After 5f25857f619f87c59e8e888fde5e66280a185123 dependency tracking
doesn't seem to work correctly any more on my macOS system (i.e.
mod-test.so isn't rebuilt if the module source has changed). `make' is GNU
Make 3.81; maybe there was a bug that has been fixed in the meantime? If
so, should we check for this in configure?
- Regarding the core dumps, it would probably better to use `ulimit -c 0'
to prevent them during tests where aborts are expected. Not sure how to do
that portably though.

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

bug#27346: module tests fail to compile with gcc 4.8.5

[Glenn Morris]

Philipp Stephani wrote:

> - After 5f25857f619f87c59e8e888fde5e66280a185123 dependency tracking
> doesn't seem to work correctly any more on my macOS system (i.e.
> mod-test.so isn't rebuilt if the module source has changed).
> `make' is GNU Make 3.81; maybe there was a bug that has been fixed in
> the meantime? If so, should we check for this in configure?

The previous version used "$srcdir", which wasn't defined in that
Makefile, and would have been wrong even if it was, so I can't see how
it could ever have worked for you. It works for me now with make 3.82 if
either mod-test.c or emacs-module.h is changed. I don't see how this
could be specific to some version of make.

> - Regarding the core dumps, it would probably better to use `ulimit -c 0'
> to prevent them during tests where aborts are expected. Not sure how to do
> that portably though.

Me neither. :)

bug#27346: module tests fail to compile with gcc 4.8.5

[Philipp Stephani]

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

Glenn Morris <rgm@gnu.org> schrieb am Di., 13. Juni 2017 um 00:10 Uhr:

> Philipp Stephani wrote:
>
> > - After 5f25857f619f87c59e8e888fde5e66280a185123 dependency tracking
> > doesn't seem to work correctly any more on my macOS system (i.e.
> > mod-test.so isn't rebuilt if the module source has changed).
> > `make' is GNU Make 3.81; maybe there was a bug that has been fixed in
> > the meantime? If so, should we check for this in configure?
>
> The previous version used "$srcdir", which wasn't defined in that
> Makefile, and would have been wrong even if it was, so I can't see how
> it could ever have worked for you.


Indeed. I think what matters for 'make -C test src/emacs-module-tests' is
the dependency declaration in test/Makefile.in.


> It works for me now with make 3.82 if
> either mod-test.c or emacs-module.h is changed. I don't see how this
> could be specific to some version of make.
>

Does it also work if you run 'make src/emacs-module-tests' from 'test'?
That's the case that apparently doesn't work any more for me (probably
because of some unrelated change).

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

bug#27346: module tests fail to compile with gcc 4.8.5

[Glenn Morris]

Philipp Stephani wrote:

> Does it also work if you run 'make src/emacs-module-tests' from 'test'?

Yes, it does.

Although I did change test/data/emacs-module/mod-test.o to not directly
depend on src/emacs-module.c (only on .h), because it doesn't. Depending
on the .c versions seems to me a bit like adding a depending on
"src/emacs", which is strictly true, but would probably be annoying in
practice. But put back the [ch] thing if that's the issue you mean.

BTW, the relevant rules in test/Makefile are duplicating some of the
logic of those in data/emacs-module/Makefile.in. Perhaps the latter
could be removed. At least, test/Makefile should use FORCE to defer to
data/emacs-module/Makefile as to whether things are up-to-date.

bug#27346: module tests fail to compile with gcc 4.8.5

[Philipp Stephani]

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

Glenn Morris <rgm@gnu.org> schrieb am Di., 13. Juni 2017 um 19:04 Uhr:

> Philipp Stephani wrote:
>
> > Does it also work if you run 'make src/emacs-module-tests' from 'test'?
>
> Yes, it does.
>
> Although I did change test/data/emacs-module/mod-test.o to not directly
> depend on src/emacs-module.c (only on .h), because it doesn't. Depending
> on the .c versions seems to me a bit like adding a depending on
> "src/emacs", which is strictly true, but would probably be annoying in
> practice. But put back the [ch] thing if that's the issue you mean.
>

No, that dependency would indeed be bogus. I don't remember why I put it
into test/Makefile.in in the first place.


>
> BTW, the relevant rules in test/Makefile are duplicating some of the
> logic of those in data/emacs-module/Makefile.in. Perhaps the latter
> could be removed.
>

Yes, I've done that now and at least for me the dependency tracking now
works fine.

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

bug#27346: module tests fail to compile with gcc 4.8.5

[Philipp Stephani]

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

Glenn Morris <rgm@gnu.org> schrieb am Mo., 12. Juni 2017 um 23:46 Uhr:

> Philipp Stephani wrote:
>
> >> +#ifdef __has_attribute
> >> +#if __has_attribute(__nonnull__)
> >>  # define EMACS_ATTRIBUTE_NONNULL(...)
> >>  __attribute__((__nonnull__(__VA_ARGS__)))
> >> -#else
> >> +#endif
> >> +#endif
> >> +#ifndef EMACS_ATTRIBUTE_NONNULL
> >>  # define EMACS_ATTRIBUTE_NONNULL(...)
> >>  #endif
>
> Applied as 69899d4.
>

Thanks!


>
> > Probably yes, thanks. (I don't know why the && expression doesn't work.)
>
> I could not find a good reference, but see eg
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36453#c3
>
>
I've looked it up in the (C++) standard, and the explanation makes sense.
What happens is that the preprocessor first replaces the 'define' forms and
expands macros, then replaces all leftover identifiers (including C
keywords) with 0. So

#if defined __has_attribute && __has_attribute ((__nonnull__))

first gets translated to

#if 0 && 0 ((0))

which is then evaluated, but it's a syntax error, which fails even if the
LHS of && is false.

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