[systemd-devel] compile with clang broken

[David Herrmann]

Hi

On Fri, Aug 15, 2014 at 11:35 AM, Thomas H.P. Andersen <phomes at gmail.com> wrote:
> 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)))
>                                           ^

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 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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: max.patch
Type: text/x-patch
Size: 2186 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20140815/19c3849a/attachment-0001.bin>