New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deal with glibc228 #75

Merged
merged 3 commits into from Sep 25, 2018

Conversation

Projects
None yet
2 participants
@cwilling
Copy link
Contributor

cwilling commented Sep 24, 2018

Fixes issue #74

Signed-off-by: Christoph Willing chris.willing@linux.com

Deal with glibc228
Fixesd issue #74

Signed-off-by: Christoph Willing <chris.willing@linux.com>
@Yamakuzure

This comment has been minimized.

Copy link
Collaborator

Yamakuzure commented Sep 25, 2018

This breaks compatibility with glibc-2.27:

[13/643] Compiling C object 'src/basic/src@basic@@basic@sta/escape.c.o'.
FAILED: src/basic/src@basic@@basic@sta/escape.c.o 
ccache cc -Isrc/basic/src@basic@@basic@sta -Isrc/basic -I../src/basic -Isrc/shared -I../src/shared -Isrc/systemd -I../src/systemd -Isrc/login -I../src/login -Isrc/core -I../src/core -I../src/libelogind/sd-bus -I../src/libelogind/sd-id128 -Isrc/sleep -I../src/sleep -Isrc/update-utmp -I../src/update-utmp -I. -I../ -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -std=gnu99 -O0 -g -Wextra -Werror=undef -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Wpointer-arith -Winit-self -Wfloat-equal -Wsuggest-attribute=noreturn -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=missing-declarations -Werror=return-type -Werror=incompatible-pointer-types -Werror=format=2 -Wstrict-prototypes -Wredundant-decls -Wmissing-noreturn -Wimplicit-fallthrough=5 -Wshadow -Wendif-labels -Wstrict-aliasing=2 -Wwrite-strings -Werror=overflow -Werror=shift-count-overflow -Werror=shift-overflow=2 -Wdate-time -Wnested-externs -ffast-math -fno-common -fdiagnostics-show-option -fno-strict-aliasing -fvisibility=hidden -fstack-protector -fstack-protector-strong --param=ssp-buffer-size=4 -fPIE -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unused-result -Wno-format-signedness -Wno-error=nonnull -Werror=shadow -include config.h -march=native -Og -g3 -ggdb -ftrapv -fPIE -Wall -Wextra -Wunused -Wno-unused-parameter -Wno-unused-result -ftree-vectorize -fPIC -pthread -fvisibility=default  -MD -MQ 'src/basic/src@basic@@basic@sta/escape.c.o' -MF 'src/basic/src@basic@@basic@sta/escape.c.o.d' -o 'src/basic/src@basic@@basic@sta/escape.c.o' -c ../src/basic/escape.c
In file included from ../src/basic/escape.h:11,
                 from ../src/basic/escape.c:8:
../src/basic/missing.h:1376:8: error: redefinition of 'struct statx_timestamp'
 struct statx_timestamp {
        ^~~~~~~~~~~~~~~
In file included from ../src/basic/missing.h:1375,
                 from ../src/basic/escape.h:11,
                 from ../src/basic/escape.c:8:
/usr/include/linux/stat.h:18:8: note: originally defined here
 struct statx_timestamp {
        ^~~~~~~~~~~~~~~
In file included from ../src/basic/escape.h:11,
                 from ../src/basic/escape.c:8:
../src/basic/missing.h:1381:8: error: redefinition of 'struct statx'
 struct statx {
        ^~~~~
In file included from ../src/basic/missing.h:1375,
                 from ../src/basic/escape.h:11,
                 from ../src/basic/escape.c:8:
/usr/include/linux/stat.h:61:8: note: originally defined here
 struct statx {
        ^~~~~
ninja: build stopped: subcommand failed.

This is bad, meson does not detect statx as it should:

 $ grep -s -R HAVE_STRUCT_STATX .
./src/basic/missing.h:#if !HAVE_STRUCT_STATX_IN_SYS_STAT_H
./build/config.h:#define HAVE_STRUCT_STATX 0
./build/config.h:#define HAVE_STRUCT_STATX_IN_SYS_STAT_H 0
./meson.build:conf.set10('HAVE_STRUCT_STATX_IN_SYS_STAT_H', cc.sizeof('struct statx', prefix : '''

And in the configuration log I can see:

Checking for size of "struct statx": -1
Checking for size of "struct statx": -1
@Yamakuzure

This comment has been minimized.

Copy link
Collaborator

Yamakuzure commented Sep 25, 2018

So you ask for statx in sys/stat.h but include linux/stat.h. That seems to be the problem.

I have extended your patch a bit:

diff --git a/meson.build b/meson.build
index 3e97f57f3..0d096d847 100644
--- a/meson.build
+++ b/meson.build
@@ -509,6 +509,13 @@ foreach decl : ['char16_t',
         conf.set10('HAVE_' + decl.underscorify().to_upper(), have)
 endforeach
 
+conf.set10('HAVE_STRUCT_STATX_IN_SYS_STAT_H', cc.sizeof('struct statx', prefix : '''
+#include <sys/stat.h>
+''', args : '-D_GNU_SOURCE') > 0)
+conf.set10('HAVE_STRUCT_STATX_IN_LINUX_STAT_H', cc.sizeof('struct statx', prefix : '''
+#include <linux/stat.h>
+''', args : '-D_GNU_SOURCE') > 0)
+
 foreach decl : [['IFLA_INET6_ADDR_GEN_MODE',         'linux/if_link.h'],
                 ['IN6_ADDR_GEN_MODE_STABLE_PRIVACY', 'linux/if_link.h'],
                 ['IFLA_VRF_TABLE',                   'linux/if_link.h'],
diff --git a/src/basic/missing.h b/src/basic/missing.h
index fb1478548..9be48eef8 100644
--- a/src/basic/missing.h
+++ b/src/basic/missing.h
@@ -1371,7 +1371,7 @@ struct fib_rule_uid_range {
 #define PF_KTHREAD 0x00200000
 #endif
 
-#if ! HAVE_STRUCT_STATX
+#if !HAVE_STRUCT_STATX_IN_SYS_STAT_H && !HAVE_STRUCT_STATX_IN_LINUX_STAT_H
 struct statx_timestamp {
         int64_t tv_sec;
         uint32_t tv_nsec;

If I apply that, compilation succeeds against glibc-2.27 and linux-headers-4.17. Could you please check whether this is still true when building against glibc-2.28 and adapt your commit accordingly?

glibc228
Take 2

Signed-off-by: Christoph Willing <chris.willing@linux.com>
@cwilling

This comment has been minimized.

Copy link
Contributor Author

cwilling commented Sep 25, 2018

Thanks for considering the PR. I have varied your variation slightly - does it work with glibc-2.27? I only have 2.23 and 2.28 systems to test with - both build OK here with this (Take 2) version of the patch.

In the meson log, I see statx not detected with 2.23; detected with 2.28 (as it should be). We can't have any unconditional linux/stat.h or sys/linux.h included with glibc-2.28 so I put them inside your new #if !HAVE_STRUCT_STATX_IN_SYS_STAT_H && !HAVE_STRUCT_STATX_IN_LINUX_STAT_H condition, so they should be included for other glibc versions.

@Yamakuzure

This comment has been minimized.

Copy link
Collaborator

Yamakuzure commented Sep 25, 2018

Oh, you were faster than my edit...

First of all: Well done! I'll check and merge tonight (and backport to the stable-branches where appropriate.)

The includes aren't necessary, so you can remove both #include lines. Enabling them unconditionally was a mistake of mine. (A stupid thinking error to be honest).

It is only necessary to declare the structs, the headers aren't needed in missing.h. And that should be true for any glibc version.

glibc228
Take 3

Signed-off-by: Christoph Willing <chris.willing@linux.com>
@cwilling

This comment has been minimized.

Copy link
Contributor Author

cwilling commented Sep 25, 2018

I just pushed a "Take 3" of the patch that removes the includes. As you pointed out, they make no difference. Tested OK with glibc's 2.23 & 2.28.

@Yamakuzure Yamakuzure merged commit 4073de9 into elogind:master Sep 25, 2018

@Yamakuzure

This comment has been minimized.

Copy link
Collaborator

Yamakuzure commented Sep 25, 2018

Great! Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment