[systemd-devel] compile with clang broken
Thomas H.P. Andersen
phomes at gmail.com
Fri Aug 15 03:29:36 PDT 2014
On Fri, Aug 15, 2014 at 11:49 AM, David Herrmann <dh.herrmann at gmail.com> wrote:
> 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?
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));
> 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)
More information about the systemd-devel
mailing list