[systemd-devel] compile with clang broken

[Thomas H.P. Andersen]

On Fri, Aug 15, 2014 at 10:55 AM, David Herrmann <dh.herrmann at gmail.com> wrote:
> Hi
>
> On Thu, Aug 14, 2014 at 8:07 PM, Lennart Poettering
> <lennart at poettering.net> wrote:
>> On Fri, 18.07.14 16:02, Thomas H.P. Andersen (phomes at 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 >= 0);
         assert(ret);
+        assert(size > 0);

         r = ioctl(fd, FIONREAD, &ms);
         if (r < 0)