[libvirt] [PATCH 0/2] Attempt, and fail, to fix build with Clang 3.9 / Linux
Martin asked for this a while ago. I was surprised, at first, because according to my experience the only issue with Clang builds were a couple of test cases failing to run, but trying again on Clang 3.9 / Fedora rawhide caused me to run into at least three separate compilation issues. Two of them I take care with this series, even though I'm fairly unconvinced the bugs are actually in libvirt rather than Clang or glibc; more details in the relevant commit messages. The last one I was unable to work around in any sensible manner: when linking, clang fails with ../gnulib/lib/.libs/libgnu.a(mgetgroups.o): In function `realloc_groupbuf': .../libvirt/gnulib/lib/mgetgroups.c:45: undefined reference to `__muloti4' This seems to be a well-known issue[1] with no solution in sight; passing CFLAGS='-rtlib=compiler-rt' to configure allows the compilation to succeed but, yeah, way gross. So basically I've spent way too much time trying to get this to work and I'm stuck now, so I'm posting whatever I have so far in hope others will pitch in and help me out :) [1] https://llvm.org/bugs/show_bug.cgi?id=16404 Andrea Bolognani (2): util: Turn virFirewallAddRule() into a macro util: Cast to 'long double' when calling isnan() src/libvirt_private.syms | 1 - src/util/virfirewall.c | 23 ----------------------- src/util/virfirewall.h | 16 ++++++++++++---- src/util/virxml.c | 10 +++++----- 4 files changed, 17 insertions(+), 33 deletions(-) -- 2.11.0
Clang 3.9 refuses to compile the existing code with the following error: util/virfirewall.c:425:20: error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Werror,-Wvarargs] va_start(args, layer); ^ util/virfirewall.c:420:37: note: parameter of type 'virFirewallLayer' is declared here virFirewallLayer layer, ^ This happens because 'layer' is of type virFirewallLayer, which is an enum type and not a standard type such as eg. void* or int. To solve the issue, turn virFirewallAddRule() from a very thin wrapper around virFirewallAddRuleFullV() to a macro that expands to a call to virFirewallAddRuleFull() - itself a very thin wrapper around the aforementioned virFirewallAddRuleFullV() - with no loss of functionality or type safety. --- This only seems to be required on very specific combinations of Clang and host OS, eg. I need it on Clang 3.9 / Fedora rawhide but not on Clang 3.8 or 4.0 / Debian sid. src/libvirt_private.syms | 1 - src/util/virfirewall.c | 23 ----------------------- src/util/virfirewall.h | 16 ++++++++++++---- 3 files changed, 12 insertions(+), 28 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2d23e462d..01118730b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1624,7 +1624,6 @@ virFindFileInPath; # util/virfirewall.h -virFirewallAddRule; virFirewallAddRuleFull; virFirewallApply; virFirewallFree; diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 3f976186a..4de38d592 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -405,29 +405,6 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, return NULL; } -/** - * virFirewallAddRule: - * @firewall: firewall ruleset to add to - * @layer: the firewall layer to change - * @...: NULL terminated list of strings for the rule - * - * Add any type of rule to the firewall ruleset. - * - * Returns the new rule - */ -virFirewallRulePtr -virFirewallAddRule(virFirewallPtr firewall, - virFirewallLayer layer, - ...) -{ - virFirewallRulePtr rule; - va_list args; - va_start(args, layer); - rule = virFirewallAddRuleFullV(firewall, layer, false, NULL, NULL, args); - va_end(args); - return rule; -} - /** * virFirewallAddRuleFull: diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index dbf397537..5248d6003 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -44,10 +44,18 @@ virFirewallPtr virFirewallNew(void); void virFirewallFree(virFirewallPtr firewall); -virFirewallRulePtr virFirewallAddRule(virFirewallPtr firewall, - virFirewallLayer layer, - ...) - ATTRIBUTE_SENTINEL; +/** + * virFirewallAddRule: + * @firewall: firewall ruleset to add to + * @layer: the firewall layer to change + * @...: NULL terminated list of strings for the rule + * + * Add any type of rule to the firewall ruleset. + * + * Returns the new rule + */ +#define virFirewallAddRule(firewall, layer, ...) \ + virFirewallAddRuleFull(firewall, layer, false, NULL, NULL, __VA_ARGS__) typedef int (*virFirewallQueryCallback)(virFirewallPtr firewall, const char *const *lines, -- 2.11.0
On Mon, Jan 02, 2017 at 07:15:30PM +0100, Andrea Bolognani wrote:
Clang 3.9 refuses to compile the existing code with the following error:
util/virfirewall.c:425:20: error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Werror,-Wvarargs] va_start(args, layer); ^ util/virfirewall.c:420:37: note: parameter of type 'virFirewallLayer' is declared here virFirewallLayer layer, ^
This happens because 'layer' is of type virFirewallLayer, which is an enum type and not a standard type such as eg. void* or int.
To solve the issue, turn virFirewallAddRule() from a very thin wrapper around virFirewallAddRuleFullV() to a macro that expands to a call to virFirewallAddRuleFull() - itself a very thin wrapper around the aforementioned virFirewallAddRuleFullV() - with no loss of functionality or type safety. --- This only seems to be required on very specific combinations of Clang and host OS, eg. I need it on Clang 3.9 / Fedora rawhide but not on Clang 3.8 or 4.0 / Debian sid.
We sent various patches for this (me, Jan and maybe other people as well). I never realized it's not a problem with different versions of clang. I would say it's not a problem for us to solve it in this case, however, as I wrote in my solution, it works, but it's undefined from the specification point of view. Can it work just because virFirewallAddRule() gets optimized into inline function? it shouldn't be, though... I don't know.
On Tue, 2017-01-03 at 13:46 +0100, Martin Kletzander wrote:
To solve the issue, turn virFirewallAddRule() from a very thin wrapper around virFirewallAddRuleFullV() to a macro that expands to a call to virFirewallAddRuleFull() - itself a very thin wrapper around the aforementioned virFirewallAddRuleFullV() - with no loss of functionality or type safety. --- This only seems to be required on very specific combinations of Clang and host OS, eg. I need it on Clang 3.9 / Fedora rawhide but not on Clang 3.8 or 4.0 / Debian sid. We sent various patches for this (me, Jan and maybe other people as well). I never realized it's not a problem with different versions of clang.
Oh, must have missed the previous attempts to fix this.
I would say it's not a problem for us to solve it in this case, however, as I wrote in my solution, it works, but it's undefined from the specification point of view. Can it work just because virFirewallAddRule() gets optimized into inline function? it shouldn't be, though... I don't know.
I lean towards merging this or a comparable solution. It's true that we aren't currently hitting this on our main targets, but relying on undefined behavior is definitely something we want to avoid, plus I don't see any real drawbacks in changing this to a macro. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Jan 03, 2017 at 02:17:38PM +0100, Andrea Bolognani wrote:
On Tue, 2017-01-03 at 13:46 +0100, Martin Kletzander wrote:
To solve the issue, turn virFirewallAddRule() from a very thin wrapper around virFirewallAddRuleFullV() to a macro that expands to a call to virFirewallAddRuleFull() - itself a very thin wrapper around the aforementioned virFirewallAddRuleFullV() - with no loss of functionality or type safety. --- This only seems to be required on very specific combinations of Clang and host OS, eg. I need it on Clang 3.9 / Fedora rawhide but not on Clang 3.8 or 4.0 / Debian sid. We sent various patches for this (me, Jan and maybe other people as well). I never realized it's not a problem with different versions of clang.
Oh, must have missed the previous attempts to fix this.
I would say it's not a problem for us to solve it in this case, however, as I wrote in my solution, it works, but it's undefined from the specification point of view. Can it work just because virFirewallAddRule() gets optimized into inline function? it shouldn't be, though... I don't know.
I lean towards merging this or a comparable solution. It's true that we aren't currently hitting this on our main targets, but relying on undefined behavior is definitely something we want to avoid, plus I don't see any real drawbacks in changing this to a macro.
Agreed, I think this patch is fine - the original code could easily have been done this way even ignoring the clang issue. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Tue, Jan 03, 2017 at 02:17:38PM +0100, Andrea Bolognani wrote:
On Tue, 2017-01-03 at 13:46 +0100, Martin Kletzander wrote:
To solve the issue, turn virFirewallAddRule() from a very thin wrapper around virFirewallAddRuleFullV() to a macro that expands to a call to virFirewallAddRuleFull() - itself a very thin wrapper around the aforementioned virFirewallAddRuleFullV() - with no loss of functionality or type safety. --- This only seems to be required on very specific combinations of Clang and host OS, eg. I need it on Clang 3.9 / Fedora rawhide but not on Clang 3.8 or 4.0 / Debian sid. We sent various patches for this (me, Jan and maybe other people as well). I never realized it's not a problem with different versions of clang.
Oh, must have missed the previous attempts to fix this.
I would say it's not a problem for us to solve it in this case, however, as I wrote in my solution, it works, but it's undefined from the specification point of view. Can it work just because virFirewallAddRule() gets optimized into inline function? it shouldn't be, though... I don't know.
I lean towards merging this or a comparable solution. It's true that we aren't currently hitting this on our main targets, but relying on undefined behavior is definitely something we want to avoid, plus I don't see any real drawbacks in changing this to a macro.
Feel free to have a look at the other approaches (and whole threads) and see what you like: - https://www.redhat.com/archives/libvir-list/2016-June/msg02173.html - https://www.redhat.com/archives/libvir-list/2016-December/msg00379.html Yeah, it goes a long way back, and I know about even longer standing clang problems that we're just not dealing with. Martin
-- Andrea Bolognani / Red Hat / Virtualization
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2017-01-03 at 14:48 +0100, Martin Kletzander wrote:
I lean towards merging this or a comparable solution. It's true that we aren't currently hitting this on our main targets, but relying on undefined behavior is definitely something we want to avoid, plus I don't see any real drawbacks in changing this to a macro. Feel free to have a look at the other approaches (and whole threads) and see what you like: - https://www.redhat.com/archives/libvir-list/2016-June/msg02173.html - https://www.redhat.com/archives/libvir-list/2016-December/msg00379.html Yeah, it goes a long way back, and I know about even longer standing clang problems that we're just not dealing with.
I like my own solution better than either yours or Jano's :) And given Dan's okay with it as well, I will push the patch in a while unless someone disagrees. Jano, does the other stuff you fixed with your series back in June still apply? Going through the thread, it looks like I tried and failed to reproduce the build failure on my own setup at the time. -- Andrea Bolognani / Red Hat / Virtualization
Clang 3.9 chokes when calling isnan() on a double variable: util/virxml.c:153:21: error: implicit conversion increases floating-point precision: 'double' to 'long double' [-Werror,-Wdouble-promotion] (isnan(obj->floatval))) { ~~~~~~~~~~~^~~~~~~~~ /usr/include/math.h:360:46: note: expanded from macro 'isnan' # define isnan(x) __MATH_TG ((x), __isnan, (x)) ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~ /usr/include/math.h:295:16: note: expanded from macro '__MATH_TG' : FUNC ## l ARGS) ~~~~~~~~~ ^~~~ Note how the wrong version of isnan() is being called: isnanl() is for 'long double's, but obj->floatval is a double and a suitable version should be called instead. Cast the value to 'long double' to make the compiler happy. --- Clang seems to be tripping on the specific way the isnan() macro is defined in recent glibc versions; more specifically, if I replace the current definition in <math.h> with the one that predates the introduction of the __MATH_TG() macro, I can get the current code to compile. I was not able to find anything wrong with the __MATH_TG() macro though. src/util/virxml.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/virxml.c b/src/util/virxml.c index 666024809..076c23c03 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -150,7 +150,7 @@ virXPathNumber(const char *xpath, obj = xmlXPathEval(BAD_CAST xpath, ctxt); ctxt->node = relnode; if ((obj == NULL) || (obj->type != XPATH_NUMBER) || - (isnan(obj->floatval))) { + (isnan((long double) obj->floatval))) { xmlXPathFreeObject(obj); return -1; } @@ -183,7 +183,7 @@ virXPathLongBase(const char *xpath, if (virStrToLong_l((char *) obj->stringval, NULL, base, value) < 0) ret = -2; } else if ((obj != NULL) && (obj->type == XPATH_NUMBER) && - (!(isnan(obj->floatval)))) { + (!(isnan((long double) obj->floatval)))) { *value = (long) obj->floatval; if (*value != obj->floatval) ret = -2; @@ -288,7 +288,7 @@ virXPathULongBase(const char *xpath, if (virStrToLong_ul((char *) obj->stringval, NULL, base, value) < 0) ret = -2; } else if ((obj != NULL) && (obj->type == XPATH_NUMBER) && - (!(isnan(obj->floatval)))) { + (!(isnan((long double) obj->floatval)))) { *value = (unsigned long) obj->floatval; if (*value != obj->floatval) ret = -2; @@ -404,7 +404,7 @@ virXPathULongLong(const char *xpath, if (virStrToLong_ull((char *) obj->stringval, NULL, 10, value) < 0) ret = -2; } else if ((obj != NULL) && (obj->type == XPATH_NUMBER) && - (!(isnan(obj->floatval)))) { + (!(isnan((long double) obj->floatval)))) { *value = (unsigned long long) obj->floatval; if (*value != obj->floatval) ret = -2; @@ -450,7 +450,7 @@ virXPathLongLong(const char *xpath, if (virStrToLong_ll((char *) obj->stringval, NULL, 10, value) < 0) ret = -2; } else if ((obj != NULL) && (obj->type == XPATH_NUMBER) && - (!(isnan(obj->floatval)))) { + (!(isnan((long double) obj->floatval)))) { *value = (long long) obj->floatval; if (*value != obj->floatval) ret = -2; -- 2.11.0
On Mon, Jan 02, 2017 at 07:15:31PM +0100, Andrea Bolognani wrote:
Clang 3.9 chokes when calling isnan() on a double variable:
util/virxml.c:153:21: error: implicit conversion increases floating-point precision: 'double' to 'long double' [-Werror,-Wdouble-promotion] (isnan(obj->floatval))) { ~~~~~~~~~~~^~~~~~~~~ /usr/include/math.h:360:46: note: expanded from macro 'isnan' # define isnan(x) __MATH_TG ((x), __isnan, (x)) ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~ /usr/include/math.h:295:16: note: expanded from macro '__MATH_TG' : FUNC ## l ARGS) ~~~~~~~~~ ^~~~
Note how the wrong version of isnan() is being called: isnanl() is for 'long double's, but obj->floatval is a double and a suitable version should be called instead.
I don't know where do you see that ^^. Good eyes, I guess =)
Cast the value to 'long double' to make the compiler happy. --- Clang seems to be tripping on the specific way the isnan() macro is defined in recent glibc versions; more specifically, if I replace the current definition in <math.h> with the one that predates the introduction of the __MATH_TG() macro, I can get the current code to compile. I was not able to find anything wrong with the __MATH_TG() macro though.
This sounds like a glibc <=> clang problem that we shoudn't introduce more complexity for. Also *I* don't see this error, for a change =) Martin
On Tue, 2017-01-03 at 13:54 +0100, Martin Kletzander wrote:
On Mon, Jan 02, 2017 at 07:15:31PM +0100, Andrea Bolognani wrote:
Clang 3.9 chokes when calling isnan() on a double variable: util/virxml.c:153:21: error: implicit conversion increases floating-point precision: 'double' to 'long double' [-Werror,-Wdouble-promotion] (isnan(obj->floatval))) { ~~~~~~~~~~~^~~~~~~~~ /usr/include/math.h:360:46: note: expanded from macro 'isnan' # define isnan(x) __MATH_TG ((x), __isnan, (x)) ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~ /usr/include/math.h:295:16: note: expanded from macro '__MATH_TG' : FUNC ## l ARGS) ~~~~~~~~~ ^~~~ Note how the wrong version of isnan() is being called: isnanl() is for 'long double's, but obj->floatval is a double and a suitable version should be called instead. I don't know where do you see that ^^. Good eyes, I guess =)
'FUNC ## l' turns into '__isnanl' when the macro is expanded, and that's the isnan() variant to be used for 'long double's.
Cast the value to 'long double' to make the compiler happy. --- Clang seems to be tripping on the specific way the isnan() macro is defined in recent glibc versions; more specifically, if I replace the current definition in <math.h> with the one that predates the introduction of the __MATH_TG() macro, I can get the current code to compile. I was not able to find anything wrong with the __MATH_TG() macro though. This sounds like a glibc <=> clang problem that we shoudn't introduce more complexity for. Also *I* don't see this error, for a change =)
Definitely, and I would SNACK any attempt to actually merge this :) It's just a dirty hack that I threw together in an attempt to get the compiler to move on with the next error: its real value is not in the code itself but in the commit message. I believe we should look for existing Clang / glibc bugs about this issue and, if none is found, file one along with a minimal reproducer, which should be fairly easy to create. -- Andrea Bolognani / Red Hat / Virtualization
On 01/02/2017 12:15 PM, Andrea Bolognani wrote:
The last one I was unable to work around in any sensible manner: when linking, clang fails with
../gnulib/lib/.libs/libgnu.a(mgetgroups.o): In function `realloc_groupbuf': .../libvirt/gnulib/lib/mgetgroups.c:45: undefined reference to `__muloti4'
This seems to be a well-known issue[1] with no solution in sight; passing CFLAGS='-rtlib=compiler-rt' to configure allows the compilation to succeed but, yeah, way gross.
And gnulib just fixed things to avoid tickling that clang bug: http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=04441fd8 Work around LLVM bug 16404, which is still not fixed. https://llvm.org/bugs/show_bug.cgi?id=16404 Problem reported by Nelson H. F. Beebe. Shall I go ahead and update the gnulib submodule to the latest, and see if you can get further with your clang tests as a result? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, 2017-01-06 at 15:48 -0600, Eric Blake wrote:
The last one I was unable to work around in any sensible manner: when linking, clang fails with ../gnulib/lib/.libs/libgnu.a(mgetgroups.o): In function `realloc_groupbuf': .../libvirt/gnulib/lib/mgetgroups.c:45: undefined reference to `__muloti4' This seems to be a well-known issue[1] with no solution in sight; passing CFLAGS='-rtlib=compiler-rt' to configure allows the compilation to succeed but, yeah, way gross. And gnulib just fixed things to avoid tickling that clang bug: http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=04441fd8 Work around LLVM bug 16404, which is still not fixed. https://llvm.org/bugs/show_bug.cgi?id=16404 Problem reported by Nelson H. F. Beebe.
Wow, that's great!
Shall I go ahead and update the gnulib submodule to the latest, and see if you can get further with your clang tests as a result?
I might not be able to play with Clang again in the immediate future, but I don't see why that should prevent us from paving the way for other parties who might be interested. Please go right ahead :) -- Andrea Bolognani / Red Hat / Virtualization
participants (4)
-
Andrea Bolognani -
Daniel P. Berrange -
Eric Blake -
Martin Kletzander