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 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)
diff --git a/src/shared/macro.h b/src/shared/macro.h
index 5619c32..ac0f3a6 100644
--- a/src/shared/macro.h
+++ b/src/shared/macro.h
@@ -133,12 +133,15 @@ static inline unsigned long ALIGN_POWER2(unsigned long u) {
})
#undef MAX
-#define MAX(a,b) \
- __extension__ ({ \
- typeof(a) _a = (a); \
- typeof(b) _b = (b); \
- _a > _b ? _a : _b; \
- })
+#define MAX(_A, _B) \
+ __extension__ (__builtin_choose_expr( \
+ (__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__ ({ \
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();
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
