12:15 p.m.
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
12:15 p.m.
New subject: [libvirt] [PATCH 1/2] util: Turn virFirewallAddRule() into a macro
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
6:46 a.m.
New subject: [libvirt] [PATCH 1/2] util: Turn virFirewallAddRule() into a macro
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.
7:17 a.m.
New subject: [libvirt] [PATCH 1/2] util: Turn virFirewallAddRule() into a macro
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
7:21 a.m.
New subject: [libvirt] [PATCH 1/2] util: Turn virFirewallAddRule() into a macro
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/ :|
7:48 a.m.
New subject: [libvirt] [PATCH 1/2] util: Turn virFirewallAddRule() into a macro
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:27 a.m.
New subject: [libvirt] [PATCH 1/2] util: Turn virFirewallAddRule() into a macro
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
12:15 p.m.
New subject: [libvirt] [PATCH 2/2] util: Cast to 'long double' when calling isnan()
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
6:54 a.m.
New subject: [libvirt] [PATCH 2/2] util: Cast to 'long double' when calling isnan()
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
7:27 a.m.
New subject: [libvirt] [PATCH 2/2] util: Cast to 'long double' when calling isnan()
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
3:48 p.m.
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
6:47 a.m.
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
2903
days inactive
2911
days old
11 comments
4 participants
participants (4)
-
Andrea Bolognani -
Daniel P. Berrange -
Eric Blake -
Martin Kletzander