Re: [systemd-devel] compile with clang broken
Hi
On Sat, Aug 16, 2014 at 1:29 PM, Daniele Nicolodi dani...@grinta.net wrote:
On 16/08/2014 12:35, David Herrmann wrote:
On Fri, Aug 15, 2014 at 5:22 PM, Daniele Nicolodi dani...@grinta.net wrote:
this may be completely stupid, but if the only use case you have for
CONST_MAX() is for computing the size of a data structure, I find
something like
#define MAXSIZE(A, B) sizeof(union { __typeof(A) a; __typeof(B) b;})
a little more clear and less magic, and I believe it has the same
guarantees that the solution you found.
Your MAXSIZE macro might add padding:
This union has size 8, not 5 (64bit). But CONST_MAX would return 5.
Not sure whether that really matters, though. And we could probably
add __packed__ to the definition.
Indeed it does add padding. Adding the __packed__ attribute solves the
problem:
#define MAXSIZE(A, B) sizeof( \
union __attribute__((__packed__)) { \
__typeof(A) a; __typeof(B) b;})
However, I noticed that GCC complains about using
statement-expressions to initialize static-const structure members,
even with my 'const' annotations added to MAX. *sigh*
Thus, I think I'll keep CONST_MAX, as we'd require a 3rd macro otherwise.
My proposal was based on the fact that the only use of CONST_MAX there
is (was?) in the code was about array size declarations, and I find
MAXSIZE() much easier to understand.
I've added the macro now.
Thanks!
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On Sat, Aug 16, 2014 at 5:35 AM, David Herrmann dh.herrm...@gmail.com
wrote:
Hi
On Fri, Aug 15, 2014 at 5:22 PM, Daniele Nicolodi dani...@grinta.net
wrote:
On 15/08/2014 16:30, David Herrmann wrote:
Ok, took me a while, but I now figured out how to cause compilation to
fail even in expressions that initialize types (_Static_assert is not
allowed there):
#define assert_const(expr)
((void)(__builtin_types_compatible_p(int[(expr) ? 1 : -1], int[1])))
Btw., I like that more than our current assert_cc() fallback. But I
leave it up to you to decide.
Anyhow, I found a way to make CONST_MAX work:
#define CONST_MAX(_A, _B)
(__builtin_choose_expr(__builtin_constant_p(_A)
__builtin_constant_p(_B), ((_A) (_B)) ? (_A) : (_B), (void)0))
This will return (void) in case _A or _B is not constant. Works fine
on LLVM, I now have to test it on gcc. If it works, I will commit it
and fix resolvd.
Hello,
this may be completely stupid, but if the only use case you have for
CONST_MAX() is for computing the size of a data structure, I find
something like
#define MAXSIZE(A, B) sizeof(union { __typeof(A) a; __typeof(B) b;})
a little more clear and less magic, and I believe it has the same
guarantees that the solution you found.
Your MAXSIZE macro might add padding:
union A {
int a;
char b[5];
};
This union has size 8, not 5 (64bit). But CONST_MAX would return 5.
Not sure whether that really matters, though. And we could probably
add __packed__ to the definition.
However, I noticed that GCC complains about using
statement-expressions to initialize static-const structure members,
even with my 'const' annotations added to MAX. *sigh*
Thus, I think I'll keep CONST_MAX, as we'd require a 3rd macro otherwise.
If you know a way to unify them all, please lemme know.
Thanks for getting this in! I had given up on receiving constructive
feedback on my attempt to solve this earlier [1]. Glad your patch garnered
useful feedback that got to a committable result!
-Dan
[1]
http://lists.freedesktop.org/archives/systemd-devel/2014-August/021761.html
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On Sat, 16.08.14 13:29, Daniele Nicolodi (dani...@grinta.net) wrote:
On 16/08/2014 12:35, David Herrmann wrote:
On Fri, Aug 15, 2014 at 5:22 PM, Daniele Nicolodi dani...@grinta.net
wrote:
this may be completely stupid, but if the only use case you have for
CONST_MAX() is for computing the size of a data structure, I find
something like
#define MAXSIZE(A, B) sizeof(union { __typeof(A) a; __typeof(B) b;})
a little more clear and less magic, and I believe it has the same
guarantees that the solution you found.
Your MAXSIZE macro might add padding:
This union has size 8, not 5 (64bit). But CONST_MAX would return 5.
Not sure whether that really matters, though. And we could probably
add __packed__ to the definition.
Indeed it does add padding. Adding the __packed__ attribute solves the
problem:
#define MAXSIZE(A, B) sizeof( \
union __attribute__((__packed__)) { \
__typeof(A) a; __typeof(B) b;})
I like this actually. I am also fine with CONST_MAX(). I'd also be fine
with having both... ;-)
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
Hi
On Fri, Aug 15, 2014 at 5:22 PM, Daniele Nicolodi dani...@grinta.net wrote:
On 15/08/2014 16:30, David Herrmann wrote:
Ok, took me a while, but I now figured out how to cause compilation to
fail even in expressions that initialize types (_Static_assert is not
allowed there):
#define assert_const(expr)
((void)(__builtin_types_compatible_p(int[(expr) ? 1 : -1], int[1])))
Btw., I like that more than our current assert_cc() fallback. But I
leave it up to you to decide.
Anyhow, I found a way to make CONST_MAX work:
#define CONST_MAX(_A, _B)
(__builtin_choose_expr(__builtin_constant_p(_A)
__builtin_constant_p(_B), ((_A) (_B)) ? (_A) : (_B), (void)0))
This will return (void) in case _A or _B is not constant. Works fine
on LLVM, I now have to test it on gcc. If it works, I will commit it
and fix resolvd.
Hello,
this may be completely stupid, but if the only use case you have for
CONST_MAX() is for computing the size of a data structure, I find
something like
#define MAXSIZE(A, B) sizeof(union { __typeof(A) a; __typeof(B) b;})
a little more clear and less magic, and I believe it has the same
guarantees that the solution you found.
Your MAXSIZE macro might add padding:
union A {
int a;
char b[5];
};
This union has size 8, not 5 (64bit). But CONST_MAX would return 5.
Not sure whether that really matters, though. And we could probably
add __packed__ to the definition.
However, I noticed that GCC complains about using
statement-expressions to initialize static-const structure members,
even with my 'const' annotations added to MAX. *sigh*
Thus, I think I'll keep CONST_MAX, as we'd require a 3rd macro otherwise.
If you know a way to unify them all, please lemme know.
Thanks
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On 16/08/2014 12:35, David Herrmann wrote:
On Fri, Aug 15, 2014 at 5:22 PM, Daniele Nicolodi dani...@grinta.net wrote:
this may be completely stupid, but if the only use case you have for
CONST_MAX() is for computing the size of a data structure, I find
something like
#define MAXSIZE(A, B) sizeof(union { __typeof(A) a; __typeof(B) b;})
a little more clear and less magic, and I believe it has the same
guarantees that the solution you found.
Your MAXSIZE macro might add padding:
This union has size 8, not 5 (64bit). But CONST_MAX would return 5.
Not sure whether that really matters, though. And we could probably
add __packed__ to the definition.
Indeed it does add padding. Adding the __packed__ attribute solves the
problem:
#define MAXSIZE(A, B) sizeof( \
union __attribute__((__packed__)) { \
__typeof(A) a; __typeof(B) b;})
However, I noticed that GCC complains about using
statement-expressions to initialize static-const structure members,
even with my 'const' annotations added to MAX. *sigh*
Thus, I think I'll keep CONST_MAX, as we'd require a 3rd macro otherwise.
My proposal was based on the fact that the only use of CONST_MAX there
is (was?) in the code was about array size declarations, and I find
MAXSIZE() much easier to understand.
Cheers,
Daniele
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
Hi
On Thu, Aug 14, 2014 at 8:07 PM, Lennart Poettering
lenn...@poettering.net wrote:
On Fri, 18.07.14 16:02, Thomas H.P. Andersen (pho...@gmail.com) wrote:
1716f6dcf54d4c181c2e2558e3d5414f54c8d9ca (resolved: add LLMNR support
for looking up names) broke the build on clang.
src/resolve/resolved-manager.c:553:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct in_pktinfo), sizeof(struct
in6_pktinfo)))
Moving the MAX(...) to a separate line fixes that problem but another
error then happens:
src/resolve/resolved-manager.c:554:25: error: fields must have a
constant size: 'variable length array in structure' extension will
never be supported
uint8_t buffer[CMSG_SPACE(size)
We have encountered the same problem before and Lennart was able to
write the code in a different way. Would this be possible here too?
My sugegstion here would be to maybe rewrite the MAX() macro to use
__builtin_constant_p() so that it becomes constant if the params are
constant, and only uses code block when it isn't. Or so...
http://lists.freedesktop.org/archives/systemd-devel/2014-August/021912.html
Hm, I don't know whether that works. See the description here:
https://gcc.gnu.org/onlinedocs/gcc-4.3.3/gcc/Other-Builtins.html
What you propose is something like my attached patch, I guess? Along
the lines of:
(__builtin_constant_p(A) __builtin_constant_p(B)) ?
((A) (B) ? (A) : (B)) :
({ OLD_MAX })
Thing is, the ELSE case is not considered a compile-time constant by
LLVM. Therefore, the whole expression is not considered a compile-time
constant. I don't know whether conditions with __builtin_constant_p()
are evaluated at the parser-step. The GCC example replaces the ELSE
case with -1, effectively making both compile-time constants.
I also added a test-case to make sure __builtin_constant_p doesn't
evaluate the arguments.
Can someone with LLVM set up give this a spin?
Thanks
David
diff --git a/src/shared/macro.h b/src/shared/macro.h
index 5619c32..18f5a79 100644
--- a/src/shared/macro.h
+++ b/src/shared/macro.h
@@ -140,6 +140,17 @@ static inline unsigned long ALIGN_POWER2(unsigned long u) {
_a _b ? _a : _b; \
})
+#undef MAX
+#define MAX(_A, _B) \
+__extension__ ( \
+(__builtin_constant_p(_A) __builtin_constant_p(_B)) \
+? (((_A) (_B)) ? (_A) : (_B)) \
+: ({\
+typeof(_A) _a = (_A); \
+typeof(_B) _b = (_B); \
+_a _b ? _a : _b; \
+}))
+
#define MAX3(x,y,z) \
__extension__ ({ \
typeof(x) _c = MAX(x,y); \
diff --git a/src/test/test-util.c b/src/test/test-util.c
index 1850f97..d348ac5 100644
--- a/src/test/test-util.c
+++ b/src/test/test-util.c
@@ -70,6 +70,20 @@ static void test_align_power2(void) {
}
}
+static void test_max(void) {
+/* try triggering compile-errors for non-const initializers */
+static const struct {
+int a;
+} val1 = {
+.a = MAX(10, 100),
+};
+int d = 0;
+
+assert_se(val1.a == 100);
+assert_se(MAX(++d, 0) == 1);
+assert_se(d == 1);
+}
+
static void test_first_word(void) {
assert_se(first_word(Hello, ));
assert_se(first_word(Hello, Hello));
@@ -926,6 +940,7 @@ int main(int argc, char *argv[]) {
test_streq_ptr();
test_align_power2();
+test_max();
test_first_word();
test_close_many();
test_parse_boolean();
diff --git a/src/shared/macro.h b/src/shared/macro.h
index 5619c32..18f5a79 100644
--- a/src/shared/macro.h
+++ b/src/shared/macro.h
@@ -140,6 +140,17 @@ static inline unsigned long ALIGN_POWER2(unsigned long u) {
_a _b ? _a : _b; \
})
+#undef MAX
+#define MAX(_A, _B) \
+__extension__ ( \
+(__builtin_constant_p(_A) __builtin_constant_p(_B)) \
+? (((_A) (_B)) ? (_A) : (_B)) \
+: ({\
+typeof(_A) _a = (_A); \
+typeof(_B) _b = (_B); \
+_a _b ? _a : _b; \
+}))
+
#define MAX3(x,y,z) \
Re: [systemd-devel] compile with clang broken
On Fri, Aug 15, 2014 at 10:55 AM, David Herrmann dh.herrm...@gmail.com wrote:
Hi
On Thu, Aug 14, 2014 at 8:07 PM, Lennart Poettering
lenn...@poettering.net wrote:
On Fri, 18.07.14 16:02, Thomas H.P. Andersen (pho...@gmail.com) wrote:
1716f6dcf54d4c181c2e2558e3d5414f54c8d9ca (resolved: add LLMNR support
for looking up names) broke the build on clang.
src/resolve/resolved-manager.c:553:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct in_pktinfo), sizeof(struct
in6_pktinfo)))
Moving the MAX(...) to a separate line fixes that problem but another
error then happens:
src/resolve/resolved-manager.c:554:25: error: fields must have a
constant size: 'variable length array in structure' extension will
never be supported
uint8_t buffer[CMSG_SPACE(size)
We have encountered the same problem before and Lennart was able to
write the code in a different way. Would this be possible here too?
My sugegstion here would be to maybe rewrite the MAX() macro to use
__builtin_constant_p() so that it becomes constant if the params are
constant, and only uses code block when it isn't. Or so...
http://lists.freedesktop.org/archives/systemd-devel/2014-August/021912.html
Hm, I don't know whether that works. See the description here:
https://gcc.gnu.org/onlinedocs/gcc-4.3.3/gcc/Other-Builtins.html
What you propose is something like my attached patch, I guess? Along
the lines of:
(__builtin_constant_p(A) __builtin_constant_p(B)) ?
((A) (B) ? (A) : (B)) :
({ OLD_MAX })
Thing is, the ELSE case is not considered a compile-time constant by
LLVM. Therefore, the whole expression is not considered a compile-time
constant. I don't know whether conditions with __builtin_constant_p()
are evaluated at the parser-step. The GCC example replaces the ELSE
case with -1, effectively making both compile-time constants.
I also added a test-case to make sure __builtin_constant_p doesn't
evaluate the arguments.
Can someone with LLVM set up give this a spin?
I still get:
src/resolve/resolved-manager.c:844:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct
in_pktinfo), sizeof(struct in6_pktinfo)))
^
I tried moving that to a separate line but then I also still get:
src/resolve/resolved-manager.c:845:25: error: fields must have a
constant size: 'variable length array in structure' extension will
never be supported
uint8_t buffer[CMSG_SPACE(size)
^
I got the following to compile but I have not have time to test it at
all. Too ugly to live I guess...
diff --git a/src/resolve/resolved-dns-stream.c
b/src/resolve/resolved-dns-stream.c
index eb78587..1b415ae 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -62,10 +62,14 @@ static int dns_stream_complete(DnsStream *s, int error) {
}
static int dns_stream_identify(DnsStream *s) {
+const size_t size = __builtin_constant_p(
+MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo))) ?
+MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo)) : 0;
union {
struct cmsghdr header; /* For alignment */
-uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct
in_pktinfo), sizeof(struct in6_pktinfo)))
+uint8_t buffer[CMSG_SPACE(size)
+ EXTRA_CMSG_SPACE /* kernel appears
to require extra space */];
+
} control;
struct msghdr mh = {};
struct cmsghdr *cmsg;
@@ -73,6 +77,7 @@ static int dns_stream_identify(DnsStream *s) {
int r;
assert(s);
+assert(size 0);
if (s-identified)
return 0;
diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c
index bfbdc7d..1342fb1 100644
--- a/src/resolve/resolved-manager.c
+++ b/src/resolve/resolved-manager.c
@@ -839,9 +839,12 @@ fail:
int manager_recv(Manager *m, int fd, DnsProtocol protocol, DnsPacket **ret) {
_cleanup_(dns_packet_unrefp) DnsPacket *p = NULL;
+const size_t size = __builtin_constant_p(
+MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo))) ?
+MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo)) : 0;
union {
struct cmsghdr header; /* For alignment */
-uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct
in_pktinfo), sizeof(struct in6_pktinfo)))
+uint8_t buffer[CMSG_SPACE(size)
+ CMSG_SPACE(int) /* ttl/hoplimit */
+ EXTRA_CMSG_SPACE /* kernel appears
to require extra buffer space */];
} control;
@@ -855,6 +858,7 @@ int manager_recv(Manager *m, int fd, DnsProtocol
protocol, DnsPacket **ret) {
assert(m);
assert(fd =
Re: [systemd-devel] compile with clang broken
On Fri, 15.08.14 10:55, David Herrmann (dh.herrm...@gmail.com) wrote:
Hm, I don't know whether that works. See the description here:
https://gcc.gnu.org/onlinedocs/gcc-4.3.3/gcc/Other-Builtins.html
What you propose is something like my attached patch, I guess? Along
the lines of:
(__builtin_constant_p(A) __builtin_constant_p(B)) ?
((A) (B) ? (A) : (B)) :
({ OLD_MAX })
Yes, correct.
Thing is, the ELSE case is not considered a compile-time constant by
LLVM.
In that case __builtin_constant_p() would be entirely useless on LLVM,
right? And all uses by this construct in glibc would not work, right?
Thanks for looking into this!
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
Hi
On Fri, Aug 15, 2014 at 11:35 AM, Thomas H.P. Andersen pho...@gmail.com wrote:
On Fri, Aug 15, 2014 at 10:55 AM, David Herrmann dh.herrm...@gmail.com
wrote:
Hi
On Thu, Aug 14, 2014 at 8:07 PM, Lennart Poettering
lenn...@poettering.net wrote:
On Fri, 18.07.14 16:02, Thomas H.P. Andersen (pho...@gmail.com) wrote:
1716f6dcf54d4c181c2e2558e3d5414f54c8d9ca (resolved: add LLMNR support
for looking up names) broke the build on clang.
src/resolve/resolved-manager.c:553:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct in_pktinfo), sizeof(struct
in6_pktinfo)))
Moving the MAX(...) to a separate line fixes that problem but another
error then happens:
src/resolve/resolved-manager.c:554:25: error: fields must have a
constant size: 'variable length array in structure' extension will
never be supported
uint8_t buffer[CMSG_SPACE(size)
We have encountered the same problem before and Lennart was able to
write the code in a different way. Would this be possible here too?
My sugegstion here would be to maybe rewrite the MAX() macro to use
__builtin_constant_p() so that it becomes constant if the params are
constant, and only uses code block when it isn't. Or so...
http://lists.freedesktop.org/archives/systemd-devel/2014-August/021912.html
Hm, I don't know whether that works. See the description here:
https://gcc.gnu.org/onlinedocs/gcc-4.3.3/gcc/Other-Builtins.html
What you propose is something like my attached patch, I guess? Along
the lines of:
(__builtin_constant_p(A) __builtin_constant_p(B)) ?
((A) (B) ? (A) : (B)) :
({ OLD_MAX })
Thing is, the ELSE case is not considered a compile-time constant by
LLVM. Therefore, the whole expression is not considered a compile-time
constant. I don't know whether conditions with __builtin_constant_p()
are evaluated at the parser-step. The GCC example replaces the ELSE
case with -1, effectively making both compile-time constants.
I also added a test-case to make sure __builtin_constant_p doesn't
evaluate the arguments.
Can someone with LLVM set up give this a spin?
I still get:
src/resolve/resolved-manager.c:844:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct
in_pktinfo), sizeof(struct in6_pktinfo)))
^
Thanks for trying!
Result is as I expected. Evaluation takes place _after_ validating
compile-time constants, and thus __builtin_constant_p in combination
with ?: will not work if not both cases are constant. Maybe it works
with __builtin_choose_expr()?
Can you try the attached patch?
If that still doesn't work, I guess we're left with your proposed
solution below, or we add MAX_CONST() which just does (A B)?A:B.
I got the following to compile but I have not have time to test it at
all. Too ugly to live I guess...
diff --git a/src/resolve/resolved-dns-stream.c
b/src/resolve/resolved-dns-stream.c
index eb78587..1b415ae 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -62,10 +62,14 @@ static int dns_stream_complete(DnsStream *s, int error) {
}
static int dns_stream_identify(DnsStream *s) {
+const size_t size = __builtin_constant_p(
+MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo))) ?
+MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo)) :
0;
No reason to make size constant. You can use:
size_t size = MAX();
uint8_t buffer[...];
This will be similar to alloca(), I think... or maybe I'm wrong..
Thanks
David
union {
struct cmsghdr header; /* For alignment */
-uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct
in_pktinfo), sizeof(struct in6_pktinfo)))
+uint8_t buffer[CMSG_SPACE(size)
+ EXTRA_CMSG_SPACE /* kernel appears
to require extra space */];
+
} control;
struct msghdr mh = {};
struct cmsghdr *cmsg;
@@ -73,6 +77,7 @@ static int dns_stream_identify(DnsStream *s) {
int r;
assert(s);
+assert(size 0);
if (s-identified)
return 0;
diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c
index bfbdc7d..1342fb1 100644
--- a/src/resolve/resolved-manager.c
+++ b/src/resolve/resolved-manager.c
@@ -839,9 +839,12 @@ fail:
int manager_recv(Manager *m, int fd, DnsProtocol protocol, DnsPacket **ret) {
_cleanup_(dns_packet_unrefp) DnsPacket *p = NULL;
+const size_t size = __builtin_constant_p(
+MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo))) ?
+MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo)) :
0;
union {
struct cmsghdr header; /* For alignment */
-uint8_t
Re: [systemd-devel] compile with clang broken
Hi
On Fri, Aug 15, 2014 at 11:38 AM, Lennart Poettering
lenn...@poettering.net wrote:
On Fri, 15.08.14 10:55, David Herrmann (dh.herrm...@gmail.com) wrote:
Hm, I don't know whether that works. See the description here:
https://gcc.gnu.org/onlinedocs/gcc-4.3.3/gcc/Other-Builtins.html
What you propose is something like my attached patch, I guess? Along
the lines of:
(__builtin_constant_p(A) __builtin_constant_p(B)) ?
((A) (B) ? (A) : (B)) :
({ OLD_MAX })
Yes, correct.
Thing is, the ELSE case is not considered a compile-time constant by
LLVM.
In that case __builtin_constant_p() would be entirely useless on LLVM,
right? And all uses by this construct in glibc would not work, right?
No, it's just useless for our case.
glibc uses __builtin_constant_p() heavily to validate parameters. For
instance, it's very handy to verify length restrictions and so on. And
I think it was introduced mainly to allow optimizations, not to allow
conditional compilations. But maybe __builtin_choose_expr() works like
that.
Example: strlen() can use __builtin_constant_p() to use sizeof() - 1
on constant expressions (ok, it cannot, because there might be a NUL
in the middle, but I guess you get the idea?).
Thanks
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On Fri, Aug 15, 2014 at 10:55:57AM +0200, David Herrmann wrote:
Hi
On Thu, Aug 14, 2014 at 8:07 PM, Lennart Poettering
lenn...@poettering.net wrote:
On Fri, 18.07.14 16:02, Thomas H.P. Andersen (pho...@gmail.com) wrote:
1716f6dcf54d4c181c2e2558e3d5414f54c8d9ca (resolved: add LLMNR support
for looking up names) broke the build on clang.
src/resolve/resolved-manager.c:553:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct in_pktinfo), sizeof(struct
in6_pktinfo)))
Moving the MAX(...) to a separate line fixes that problem but another
error then happens:
src/resolve/resolved-manager.c:554:25: error: fields must have a
constant size: 'variable length array in structure' extension will
never be supported
uint8_t buffer[CMSG_SPACE(size)
We have encountered the same problem before and Lennart was able to
write the code in a different way. Would this be possible here too?
My sugegstion here would be to maybe rewrite the MAX() macro to use
__builtin_constant_p() so that it becomes constant if the params are
constant, and only uses code block when it isn't. Or so...
http://lists.freedesktop.org/archives/systemd-devel/2014-August/021912.html
Hm, I don't know whether that works. See the description here:
https://gcc.gnu.org/onlinedocs/gcc-4.3.3/gcc/Other-Builtins.html
What you propose is something like my attached patch, I guess? Along
the lines of:
(__builtin_constant_p(A) __builtin_constant_p(B)) ?
((A) (B) ? (A) : (B)) :
({ OLD_MAX })
Thing is, the ELSE case is not considered a compile-time constant by
LLVM. Therefore, the whole expression is not considered a compile-time
constant. I don't know whether conditions with __builtin_constant_p()
are evaluated at the parser-step. The GCC example replaces the ELSE
case with -1, effectively making both compile-time constants.
Sorry I didn't follow the thread, but:
Actually and IIRC it is more complicated, __builtin_constant_p() can
be computed during the SSA (optimization) passes on the GIMPLE form of
the code... so it depends on the code and parameters passed to
__builitin_constant_p(), not only preprocessor constants.
https://gcc.gnu.org/onlinedocs/gccint/Tree-SSA.html
--
Djalal Harouni
http://opendz.org
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On Fri, 15.08.14 11:49, David Herrmann (dh.herrm...@gmail.com) wrote: If that still doesn't work, I guess we're left with your proposed solution below, or we add MAX_CONST() which just does (A B)?A:B. We could also just define MAX() differently if we detect we run on LLVM. There must be some macro how one can detect that... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
Hi On Fri, Aug 15, 2014 at 12:29 PM, Thomas H.P. Andersen pho...@gmail.com wrote: On Fri, Aug 15, 2014 at 11:49 AM, David Herrmann dh.herrm...@gmail.com wrote: Thanks for trying! Result is as I expected. Evaluation takes place _after_ validating compile-time constants, and thus __builtin_constant_p in combination with ?: will not work if not both cases are constant. Maybe it works with __builtin_choose_expr()? Can you try the attached patch? This patch works. It also needs the change to do the calculation to a seperate line. Also only if size is const, like so: const size_t size = MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo)); Again, thanks for trying it out! I don't understand your comment, though. You're saying this works: const size_t size = MAX(...); uint8_t buffer[CMSG_SPACE(size) +...]; ...but this doesn't work: uint8_t buffer[CMSG_SPACE(MAX(...)) +...]; ...and this doesn't work either (mind the dropped 'const'): size_t size = MAX(...); uint8_t buffer[CMSG_SPACE(size) +...]; Hm. This is weird. Maybe CMSG_SPACE does something weird. I'll see. David ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On Fri, Aug 15, 2014 at 12:35 PM, David Herrmann dh.herrm...@gmail.com wrote: Hi On Fri, Aug 15, 2014 at 12:29 PM, Thomas H.P. Andersen pho...@gmail.com wrote: On Fri, Aug 15, 2014 at 11:49 AM, David Herrmann dh.herrm...@gmail.com wrote: Thanks for trying! Result is as I expected. Evaluation takes place _after_ validating compile-time constants, and thus __builtin_constant_p in combination with ?: will not work if not both cases are constant. Maybe it works with __builtin_choose_expr()? Can you try the attached patch? This patch works. It also needs the change to do the calculation to a seperate line. Also only if size is const, like so: const size_t size = MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo)); Again, thanks for trying it out! no problem. I have inserted the relevant error messages for the two non-working cases. I don't understand your comment, though. You're saying this works: const size_t size = MAX(...); uint8_t buffer[CMSG_SPACE(size) +...]; ...but this doesn't work: uint8_t buffer[CMSG_SPACE(MAX(...)) +...]; src/resolve/resolved-dns-stream.c:67:43: error: non-const static data member must be initialized out of line uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo))) ^ ...and this doesn't work either (mind the dropped 'const'): size_t size = MAX(...); uint8_t buffer[CMSG_SPACE(size) +...]; src/resolve/resolved-dns-stream.c:68:25: error: fields must have a constant size: 'variable length array in structure' extension will never be supported uint8_t buffer[CMSG_SPACE(size) ^ Hm. This is weird. Maybe CMSG_SPACE does something weird. I'll see. David ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On Fri, 15.08.14 12:40, Thomas H.P. Andersen (pho...@gmail.com) wrote:
On Fri, Aug 15, 2014 at 12:35 PM, David Herrmann dh.herrm...@gmail.com
wrote:
Hi
On Fri, Aug 15, 2014 at 12:29 PM, Thomas H.P. Andersen pho...@gmail.com
wrote:
On Fri, Aug 15, 2014 at 11:49 AM, David Herrmann dh.herrm...@gmail.com
wrote:
Thanks for trying!
Result is as I expected. Evaluation takes place _after_ validating
compile-time constants, and thus __builtin_constant_p in combination
with ?: will not work if not both cases are constant. Maybe it works
with __builtin_choose_expr()?
Can you try the attached patch?
This patch works. It also needs the change to do the calculation to a
seperate line. Also only if size is const, like so:
const size_t size = MAX(sizeof(struct in_pktinfo), sizeof(struct
in6_pktinfo));
Again, thanks for trying it out!
no problem. I have inserted the relevant error messages for the two
non-working cases.
I don't understand your comment, though. You're saying this works:
const size_t size = MAX(...);
uint8_t buffer[CMSG_SPACE(size) +...];
...but this doesn't work:
uint8_t buffer[CMSG_SPACE(MAX(...)) +...];
src/resolve/resolved-dns-stream.c:67:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct
in_pktinfo), sizeof(struct in6_pktinfo)))
^
...and this doesn't work either (mind the dropped 'const'):
size_t size = MAX(...);
uint8_t buffer[CMSG_SPACE(size) +...];
src/resolve/resolved-dns-stream.c:68:25: error: fields must have a
constant size: 'variable length array in structure' extension will
never be supported
uint8_t buffer[CMSG_SPACE(size)
^
Hm. This is weird. Maybe CMSG_SPACE does something weird. I'll see.
David
In this case I really think we should just detect LLVM and define MAX()
and MIN() to something weaker that doesn't use ({ ... }) expressions...
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On Fri, 15.08.14 13:00, David Herrmann (dh.herrm...@gmail.com) wrote:
src/resolve/resolved-dns-stream.c:67:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct
in_pktinfo), sizeof(struct in6_pktinfo)))
^
Ok, this can be fixed by adding const to the variables inside the ({
}) else-clause. But we then end up with:
error: statement expression not allowed at file scope
I wonder if there's *any* way how to implement a double-evalutation-free
all-type MAX() on LLVM... That'd be quite a limitation in LLVM...
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
Hi
On Fri, Aug 15, 2014 at 1:22 PM, Lennart Poettering
lenn...@poettering.net wrote:
On Fri, 15.08.14 13:00, David Herrmann (dh.herrm...@gmail.com) wrote:
src/resolve/resolved-dns-stream.c:67:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct
in_pktinfo), sizeof(struct in6_pktinfo)))
^
Ok, this can be fixed by adding const to the variables inside the ({
}) else-clause. But we then end up with:
error: statement expression not allowed at file scope
I wonder if there's *any* way how to implement a double-evalutation-free
all-type MAX() on LLVM... That'd be quite a limitation in LLVM...
I looked around and it seems like there's nothing we can do. Weird
thing is, LLVM allows const-initialization but not member-definition
with that macro. I really don't understand why..
I somehow think adding MAX_CONST which uses __builtin_constant_p and
assert_cc() is the easiest way here. That is, we use MAX_CONST() for
all cases where MAX fails. I think this is the easiest way to
guarantee no-one else changes the code to use MAX() again.
Furthermore, it guarantees that MAX_CONST is *really* called with
constant arguments.
Thanks
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On Fri, 15.08.14 13:42, David Herrmann (dh.herrm...@gmail.com) wrote:
Hi
On Fri, Aug 15, 2014 at 1:22 PM, Lennart Poettering
lenn...@poettering.net wrote:
On Fri, 15.08.14 13:00, David Herrmann (dh.herrm...@gmail.com) wrote:
src/resolve/resolved-dns-stream.c:67:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct
in_pktinfo), sizeof(struct in6_pktinfo)))
^
Ok, this can be fixed by adding const to the variables inside the ({
}) else-clause. But we then end up with:
error: statement expression not allowed at file scope
I wonder if there's *any* way how to implement a double-evalutation-free
all-type MAX() on LLVM... That'd be quite a limitation in LLVM...
I looked around and it seems like there's nothing we can do. Weird
thing is, LLVM allows const-initialization but not member-definition
with that macro. I really don't understand why..
I somehow think adding MAX_CONST which uses __builtin_constant_p and
assert_cc() is the easiest way here. That is, we use MAX_CONST() for
all cases where MAX fails. I think this is the easiest way to
guarantee no-one else changes the code to use MAX() again.
Furthermore, it guarantees that MAX_CONST is *really* called with
constant arguments.
If that is what it takes, go ahead.
Let it be known though for all future: I think LLVM is stupid here.
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
Hi
On Fri, Aug 15, 2014 at 1:53 PM, Lennart Poettering
lenn...@poettering.net wrote:
On Fri, 15.08.14 13:42, David Herrmann (dh.herrm...@gmail.com) wrote:
Hi
On Fri, Aug 15, 2014 at 1:22 PM, Lennart Poettering
lenn...@poettering.net wrote:
On Fri, 15.08.14 13:00, David Herrmann (dh.herrm...@gmail.com) wrote:
src/resolve/resolved-dns-stream.c:67:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct
in_pktinfo), sizeof(struct in6_pktinfo)))
^
Ok, this can be fixed by adding const to the variables inside the ({
}) else-clause. But we then end up with:
error: statement expression not allowed at file scope
I wonder if there's *any* way how to implement a double-evalutation-free
all-type MAX() on LLVM... That'd be quite a limitation in LLVM...
I looked around and it seems like there's nothing we can do. Weird
thing is, LLVM allows const-initialization but not member-definition
with that macro. I really don't understand why..
I somehow think adding MAX_CONST which uses __builtin_constant_p and
assert_cc() is the easiest way here. That is, we use MAX_CONST() for
all cases where MAX fails. I think this is the easiest way to
guarantee no-one else changes the code to use MAX() again.
Furthermore, it guarantees that MAX_CONST is *really* called with
constant arguments.
If that is what it takes, go ahead.
Let it be known though for all future: I think LLVM is stupid here.
Meh, static_assert() is not allowed there. _Pragma(error) works, but
is always evaluated even with __builtin_choose_expr(). This is all
stupid... That means MAX_CONST really just becomes AB?A:B.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
Hi
On Fri, Aug 15, 2014 at 1:53 PM, Lennart Poettering
lenn...@poettering.net wrote:
On Fri, 15.08.14 13:42, David Herrmann (dh.herrm...@gmail.com) wrote:
Hi
On Fri, Aug 15, 2014 at 1:22 PM, Lennart Poettering
lenn...@poettering.net wrote:
On Fri, 15.08.14 13:00, David Herrmann (dh.herrm...@gmail.com) wrote:
src/resolve/resolved-dns-stream.c:67:43: error: non-const static data
member must be initialized out of line
uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct
in_pktinfo), sizeof(struct in6_pktinfo)))
^
Ok, this can be fixed by adding const to the variables inside the ({
}) else-clause. But we then end up with:
error: statement expression not allowed at file scope
I wonder if there's *any* way how to implement a double-evalutation-free
all-type MAX() on LLVM... That'd be quite a limitation in LLVM...
I looked around and it seems like there's nothing we can do. Weird
thing is, LLVM allows const-initialization but not member-definition
with that macro. I really don't understand why..
I somehow think adding MAX_CONST which uses __builtin_constant_p and
assert_cc() is the easiest way here. That is, we use MAX_CONST() for
all cases where MAX fails. I think this is the easiest way to
guarantee no-one else changes the code to use MAX() again.
Furthermore, it guarantees that MAX_CONST is *really* called with
constant arguments.
If that is what it takes, go ahead.
Let it be known though for all future: I think LLVM is stupid here.
Ok, took me a while, but I now figured out how to cause compilation to
fail even in expressions that initialize types (_Static_assert is not
allowed there):
#define assert_const(expr)
((void)(__builtin_types_compatible_p(int[(expr) ? 1 : -1], int[1])))
Btw., I like that more than our current assert_cc() fallback. But I
leave it up to you to decide.
Anyhow, I found a way to make CONST_MAX work:
#define CONST_MAX(_A, _B)
(__builtin_choose_expr(__builtin_constant_p(_A)
__builtin_constant_p(_B), ((_A) (_B)) ? (_A) : (_B), (void)0))
This will return (void) in case _A or _B is not constant. Works fine
on LLVM, I now have to test it on gcc. If it works, I will commit it
and fix resolvd.
Cheers
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
Hi On Fri, Jul 18, 2014 at 4:02 PM, Thomas H.P. Andersen pho...@gmail.com wrote: 1716f6dcf54d4c181c2e2558e3d5414f54c8d9ca (resolved: add LLMNR support for looking up names) broke the build on clang. src/resolve/resolved-manager.c:553:43: error: non-const static data member must be initialized out of line uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo))) Moving the MAX(...) to a separate line fixes that problem but another error then happens: src/resolve/resolved-manager.c:554:25: error: fields must have a constant size: 'variable length array in structure' extension will never be supported uint8_t buffer[CMSG_SPACE(size) We have encountered the same problem before and Lennart was able to write the code in a different way. Would this be possible here too? I now pushed 3 commits which should fix this. Works fine with LLVM+clang and GCC here. Thanks David ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On 15/08/2014 16:30, David Herrmann wrote:
Ok, took me a while, but I now figured out how to cause compilation to
fail even in expressions that initialize types (_Static_assert is not
allowed there):
#define assert_const(expr)
((void)(__builtin_types_compatible_p(int[(expr) ? 1 : -1], int[1])))
Btw., I like that more than our current assert_cc() fallback. But I
leave it up to you to decide.
Anyhow, I found a way to make CONST_MAX work:
#define CONST_MAX(_A, _B)
(__builtin_choose_expr(__builtin_constant_p(_A)
__builtin_constant_p(_B), ((_A) (_B)) ? (_A) : (_B), (void)0))
This will return (void) in case _A or _B is not constant. Works fine
on LLVM, I now have to test it on gcc. If it works, I will commit it
and fix resolvd.
Hello,
this may be completely stupid, but if the only use case you have for
CONST_MAX() is for computing the size of a data structure, I find
something like
#define MAXSIZE(A, B) sizeof(union { __typeof(A) a; __typeof(B) b;})
a little more clear and less magic, and I believe it has the same
guarantees that the solution you found.
In the specific case, the problematic line could then be written as:
uint8_t buffer[CMSG_SPACE(MAXSIZE(struct in_pktinfo,
struct in6_pktinfo)) \
+ EXTRA_CMSG_SPACE];
which IMHO reads a bit better.
Cheers,
Daniele
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On Fri, 15.08.14 17:22, Daniele Nicolodi (dani...@grinta.net) wrote:
On 15/08/2014 16:30, David Herrmann wrote:
Ok, took me a while, but I now figured out how to cause compilation to
fail even in expressions that initialize types (_Static_assert is not
allowed there):
#define assert_const(expr)
((void)(__builtin_types_compatible_p(int[(expr) ? 1 : -1], int[1])))
Btw., I like that more than our current assert_cc() fallback. But I
leave it up to you to decide.
Anyhow, I found a way to make CONST_MAX work:
#define CONST_MAX(_A, _B)
(__builtin_choose_expr(__builtin_constant_p(_A)
__builtin_constant_p(_B), ((_A) (_B)) ? (_A) : (_B), (void)0))
This will return (void) in case _A or _B is not constant. Works fine
on LLVM, I now have to test it on gcc. If it works, I will commit it
and fix resolvd.
Hello,
this may be completely stupid, but if the only use case you have for
CONST_MAX() is for computing the size of a data structure, I find
something like
#define MAXSIZE(A, B) sizeof(union { __typeof(A) a; __typeof(B) b;})
a little more clear and less magic, and I believe it has the same
guarantees that the solution you found.
In the specific case, the problematic line could then be written as:
uint8_t buffer[CMSG_SPACE(MAXSIZE(struct in_pktinfo,
struct in6_pktinfo)) \
+ EXTRA_CMSG_SPACE];
which IMHO reads a bit better.
I'd be fine with that, if anyone wants to put together a patch...
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] compile with clang broken
On Fri, 18.07.14 16:02, Thomas H.P. Andersen (pho...@gmail.com) wrote: 1716f6dcf54d4c181c2e2558e3d5414f54c8d9ca (resolved: add LLMNR support for looking up names) broke the build on clang. src/resolve/resolved-manager.c:553:43: error: non-const static data member must be initialized out of line uint8_t buffer[CMSG_SPACE(MAX(sizeof(struct in_pktinfo), sizeof(struct in6_pktinfo))) Moving the MAX(...) to a separate line fixes that problem but another error then happens: src/resolve/resolved-manager.c:554:25: error: fields must have a constant size: 'variable length array in structure' extension will never be supported uint8_t buffer[CMSG_SPACE(size) We have encountered the same problem before and Lennart was able to write the code in a different way. Would this be possible here too? My sugegstion here would be to maybe rewrite the MAX() macro to use __builtin_constant_p() so that it becomes constant if the params are constant, and only uses code block when it isn't. Or so... http://lists.freedesktop.org/archives/systemd-devel/2014-August/021912.html Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
