7:58 a.m.
When libvirt is build with sanitizers enabled, in debug mode, on clang,
virNWFilterRuleDefFixup exceeds the maximum stack frame size of 8192 bytes,
as specified in meson.build:
../src/conf/nwfilter_conf.c:2190:1: error: stack frame size of 10616
bytes in function 'virNWFilterRuleDefFixup' [-Werror,-Wframe-larger-than=]
virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
^
1 error generated.
This series reworks the function a bit to bring the frame size below 8192.
Regards,
Tim
Tim Wiederhake (3):
virNWFilterRuleDefFixup: Factor out ethHdr as variable
virNWFilterRuleDefFixup: Factor out ipHdr as variable
virNWFilterRuleDefFixup: Factor out portData as variable
src/conf/nwfilter_conf.c | 179 ++++++++++++++++-----------------------
1 file changed, 73 insertions(+), 106 deletions(-)
--
2.31.1
7:58 a.m.
New subject: [libvirt PATCH 1/3] virNWFilterRuleDefFixup: Factor out ethHdr as variable
This helps to bring down the frame size of virNWFilterRuleDefFixup, as it
exceeds 8192 bytes when libvirt is build with sanitizers enabled, in debug
mode, on clang.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/nwfilter_conf.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index a3109962af..62334edeec 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2189,23 +2189,23 @@ virNWFilterRuleValidate(virNWFilterRuleDef *rule)
static void
virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
{
+ ethHdrDataDef *ethHdr;
+
#define COPY_NEG_SIGN(A, B) \
(A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \
((B).flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG);
switch (rule->prtclType) {
case VIR_NWFILTER_RULE_PROTOCOL_MAC:
- COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataSrcMACMask,
- rule->p.ethHdrFilter.ethHdr.dataSrcMACAddr);
- COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataDstMACMask,
- rule->p.ethHdrFilter.ethHdr.dataDstMACAddr);
+ ethHdr = &rule->p.ethHdrFilter.ethHdr;
+ COPY_NEG_SIGN(ethHdr->dataSrcMACMask, ethHdr->dataSrcMACAddr);
+ COPY_NEG_SIGN(ethHdr->dataDstMACMask, ethHdr->dataDstMACAddr);
break;
case VIR_NWFILTER_RULE_PROTOCOL_VLAN:
- COPY_NEG_SIGN(rule->p.vlanHdrFilter.ethHdr.dataSrcMACMask,
- rule->p.vlanHdrFilter.ethHdr.dataSrcMACAddr);
- COPY_NEG_SIGN(rule->p.vlanHdrFilter.ethHdr.dataDstMACMask,
- rule->p.vlanHdrFilter.ethHdr.dataDstMACAddr);
+ ethHdr = &rule->p.vlanHdrFilter.ethHdr;
+ COPY_NEG_SIGN(ethHdr->dataSrcMACMask, ethHdr->dataSrcMACAddr);
+ COPY_NEG_SIGN(ethHdr->dataDstMACMask, ethHdr->dataDstMACAddr);
break;
case VIR_NWFILTER_RULE_PROTOCOL_STP:
--
2.31.1
8:08 a.m.
New subject: [libvirt PATCH 1/3] virNWFilterRuleDefFixup: Factor out ethHdr as variable
On Fri, Sep 17, 2021 at 02:58:09PM +0200, Tim Wiederhake wrote:
This helps to bring down the frame size of virNWFilterRuleDefFixup,
as it
exceeds 8192 bytes when libvirt is build with sanitizers enabled, in debug
mode, on clang.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/nwfilter_conf.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index a3109962af..62334edeec 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2189,23 +2189,23 @@ virNWFilterRuleValidate(virNWFilterRuleDef *rule)
static void
virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
{
+ ethHdrDataDef *ethHdr;
+
#define COPY_NEG_SIGN(A, B) \
(A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \
((B).flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG);
switch (rule->prtclType) {
case VIR_NWFILTER_RULE_PROTOCOL_MAC:
- COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataSrcMACMask,
- rule->p.ethHdrFilter.ethHdr.dataSrcMACAddr);
- COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataDstMACMask,
- rule->p.ethHdrFilter.ethHdr.dataDstMACAddr);
+ ethHdr = &rule->p.ethHdrFilter.ethHdr;
+ COPY_NEG_SIGN(ethHdr->dataSrcMACMask, ethHdr->dataSrcMACAddr);
+ COPY_NEG_SIGN(ethHdr->dataDstMACMask, ethHdr->dataDstMACAddr);
I don't get why this reduces stack size at all ?
COPY_NEG_SIGN is just doing a bitwise and + or + assignment
which doesn't require any intermediate variables.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
8:37 a.m.
New subject: [libvirt PATCH 1/3] virNWFilterRuleDefFixup: Factor out ethHdr as variable
On Fri, 2021-09-17 at 14:08 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 17, 2021 at 02:58:09PM +0200, Tim Wiederhake wrote:
> This helps to bring down the frame size of virNWFilterRuleDefFixup,
> as it
> exceeds 8192 bytes when libvirt is build with sanitizers enabled,
> in debug
> mode, on clang.
>
> Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
> ---
> src/conf/nwfilter_conf.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
> index a3109962af..62334edeec 100644
> --- a/src/conf/nwfilter_conf.c
> +++ b/src/conf/nwfilter_conf.c
> @@ -2189,23 +2189,23 @@ virNWFilterRuleValidate(virNWFilterRuleDef
> *rule)
> static void
> virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
> {
> + ethHdrDataDef *ethHdr;
> +
> #define COPY_NEG_SIGN(A, B) \
> (A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \
> ((B).flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG);
>
> switch (rule->prtclType) {
> case VIR_NWFILTER_RULE_PROTOCOL_MAC:
> - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataSrcMACMask,
> - rule->p.ethHdrFilter.ethHdr.dataSrcMACAddr);
> - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataDstMACMask,
> - rule->p.ethHdrFilter.ethHdr.dataDstMACAddr);
> + ethHdr = &rule->p.ethHdrFilter.ethHdr;
> + COPY_NEG_SIGN(ethHdr->dataSrcMACMask, ethHdr-
> >dataSrcMACAddr);
> + COPY_NEG_SIGN(ethHdr->dataDstMACMask, ethHdr-
> >dataDstMACAddr);
I don't get why this reduces stack size at all ?
COPY_NEG_SIGN is just doing a bitwise and + or + assignment
which doesn't require any intermediate variables.
Regards,
Daniel
I believe it's debug information for something that gets optimized away
in non-debug (i.e. release) builds.
Do you know of a way to ask clang to explain what takes up the space in
a stack frame?
Regards,
Tim
9:02 a.m.
New subject: [libvirt PATCH 1/3] virNWFilterRuleDefFixup: Factor out ethHdr as variable
On Fri, Sep 17, 2021 at 03:37:25PM +0200, Tim Wiederhake wrote:
On Fri, 2021-09-17 at 14:08 +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 17, 2021 at 02:58:09PM +0200, Tim Wiederhake wrote:
> > This helps to bring down the frame size of virNWFilterRuleDefFixup,
> > as it
> > exceeds 8192 bytes when libvirt is build with sanitizers enabled,
> > in debug
> > mode, on clang.
> >
> > Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
> > ---
> > src/conf/nwfilter_conf.c | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
> > index a3109962af..62334edeec 100644
> > --- a/src/conf/nwfilter_conf.c
> > +++ b/src/conf/nwfilter_conf.c
> > @@ -2189,23 +2189,23 @@ virNWFilterRuleValidate(virNWFilterRuleDef
> > *rule)
> > static void
> > virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
> > {
> > + ethHdrDataDef *ethHdr;
> > +
> > #define COPY_NEG_SIGN(A, B) \
> > (A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \
> > ((B).flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG);
> >
> > switch (rule->prtclType) {
> > case VIR_NWFILTER_RULE_PROTOCOL_MAC:
> > - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataSrcMACMask,
> > - rule->p.ethHdrFilter.ethHdr.dataSrcMACAddr);
> > - COPY_NEG_SIGN(rule->p.ethHdrFilter.ethHdr.dataDstMACMask,
> > - rule->p.ethHdrFilter.ethHdr.dataDstMACAddr);
> > + ethHdr = &rule->p.ethHdrFilter.ethHdr;
> > + COPY_NEG_SIGN(ethHdr->dataSrcMACMask, ethHdr-
> > >dataSrcMACAddr);
> > + COPY_NEG_SIGN(ethHdr->dataDstMACMask, ethHdr-
> > >dataDstMACAddr);
>
> I don't get why this reduces stack size at all ?
>
> COPY_NEG_SIGN is just doing a bitwise and + or + assignment
> which doesn't require any intermediate variables.
>
>
> Regards,
> Daniel
I believe it's debug information for something that gets optimized away
in non-debug (i.e. release) builds.
Do you know of a way to ask clang to explain what takes up the space in
a stack frame?
Not unless -save-temps shows it in the intermediate files
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
7:58 a.m.
New subject: [libvirt PATCH 2/3] virNWFilterRuleDefFixup: Factor out ipHdr as variable
This helps to bring down the frame size of virNWFilterRuleDefFixup, as it
exceeds 8192 bytes when libvirt is build with sanitizers enabled, in debug
mode, on clang.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/nwfilter_conf.c | 132 +++++++++++++++------------------------
1 file changed, 52 insertions(+), 80 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 62334edeec..2448e4b70b 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2190,6 +2190,7 @@ static void
virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
{
ethHdrDataDef *ethHdr;
+ ipHdrDataDef *ipHdr;
#define COPY_NEG_SIGN(A, B) \
(A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \
@@ -2234,18 +2235,16 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
break;
case VIR_NWFILTER_RULE_PROTOCOL_IP:
- COPY_NEG_SIGN(rule->p.ipHdrFilter.ipHdr.dataSrcIPMask,
- rule->p.ipHdrFilter.ipHdr.dataSrcIPAddr);
- COPY_NEG_SIGN(rule->p.ipHdrFilter.ipHdr.dataDstIPMask,
- rule->p.ipHdrFilter.ipHdr.dataDstIPAddr);
+ ipHdr = &rule->p.ipHdrFilter.ipHdr;
+ COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
virNWFilterRuleDefFixupIPSet(&rule->p.ipHdrFilter.ipHdr);
break;
case VIR_NWFILTER_RULE_PROTOCOL_IPV6:
- COPY_NEG_SIGN(rule->p.ipv6HdrFilter.ipHdr.dataSrcIPMask,
- rule->p.ipv6HdrFilter.ipHdr.dataSrcIPAddr);
- COPY_NEG_SIGN(rule->p.ipv6HdrFilter.ipHdr.dataDstIPMask,
- rule->p.ipv6HdrFilter.ipHdr.dataDstIPAddr);
+ ipHdr = &rule->p.ipv6HdrFilter.ipHdr;
+ COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
COPY_NEG_SIGN(rule->p.ipv6HdrFilter.dataICMPTypeEnd,
rule->p.ipv6HdrFilter.dataICMPTypeStart);
COPY_NEG_SIGN(rule->p.ipv6HdrFilter.dataICMPCodeStart,
@@ -2262,14 +2261,11 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
case VIR_NWFILTER_RULE_PROTOCOL_TCP:
case VIR_NWFILTER_RULE_PROTOCOL_TCPoIPV6:
- COPY_NEG_SIGN(rule->p.tcpHdrFilter.ipHdr.dataSrcIPMask,
- rule->p.tcpHdrFilter.ipHdr.dataSrcIPAddr);
- COPY_NEG_SIGN(rule->p.tcpHdrFilter.ipHdr.dataDstIPMask,
- rule->p.tcpHdrFilter.ipHdr.dataDstIPAddr);
- COPY_NEG_SIGN(rule->p.tcpHdrFilter.ipHdr.dataSrcIPTo,
- rule->p.tcpHdrFilter.ipHdr.dataSrcIPFrom);
- COPY_NEG_SIGN(rule->p.tcpHdrFilter.ipHdr.dataDstIPTo,
- rule->p.tcpHdrFilter.ipHdr.dataDstIPFrom);
+ ipHdr = &rule->p.tcpHdrFilter.ipHdr;
+ COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
+ COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
COPY_NEG_SIGN(rule->p.tcpHdrFilter.portData.dataSrcPortEnd,
rule->p.tcpHdrFilter.portData.dataSrcPortStart);
COPY_NEG_SIGN(rule->p.tcpHdrFilter.portData.dataDstPortStart,
@@ -2281,14 +2277,11 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
case VIR_NWFILTER_RULE_PROTOCOL_UDP:
case VIR_NWFILTER_RULE_PROTOCOL_UDPoIPV6:
- COPY_NEG_SIGN(rule->p.udpHdrFilter.ipHdr.dataSrcIPMask,
- rule->p.udpHdrFilter.ipHdr.dataSrcIPAddr);
- COPY_NEG_SIGN(rule->p.udpHdrFilter.ipHdr.dataDstIPMask,
- rule->p.udpHdrFilter.ipHdr.dataDstIPAddr);
- COPY_NEG_SIGN(rule->p.udpHdrFilter.ipHdr.dataSrcIPTo,
- rule->p.udpHdrFilter.ipHdr.dataSrcIPFrom);
- COPY_NEG_SIGN(rule->p.udpHdrFilter.ipHdr.dataDstIPTo,
- rule->p.udpHdrFilter.ipHdr.dataDstIPFrom);
+ ipHdr = &rule->p.udpHdrFilter.ipHdr;
+ COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
+ COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
COPY_NEG_SIGN(rule->p.udpHdrFilter.portData.dataSrcPortEnd,
rule->p.udpHdrFilter.portData.dataSrcPortStart);
COPY_NEG_SIGN(rule->p.udpHdrFilter.portData.dataDstPortStart,
@@ -2300,53 +2293,41 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
case VIR_NWFILTER_RULE_PROTOCOL_UDPLITE:
case VIR_NWFILTER_RULE_PROTOCOL_UDPLITEoIPV6:
- COPY_NEG_SIGN(rule->p.udpliteHdrFilter.ipHdr.dataSrcIPMask,
- rule->p.udpliteHdrFilter.ipHdr.dataSrcIPAddr);
- COPY_NEG_SIGN(rule->p.udpliteHdrFilter.ipHdr.dataDstIPMask,
- rule->p.udpliteHdrFilter.ipHdr.dataDstIPAddr);
- COPY_NEG_SIGN(rule->p.udpliteHdrFilter.ipHdr.dataSrcIPTo,
- rule->p.udpliteHdrFilter.ipHdr.dataSrcIPFrom);
- COPY_NEG_SIGN(rule->p.udpliteHdrFilter.ipHdr.dataDstIPTo,
- rule->p.udpliteHdrFilter.ipHdr.dataDstIPFrom);
+ ipHdr = &rule->p.udpliteHdrFilter.ipHdr;
+ COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
+ COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
virNWFilterRuleDefFixupIPSet(&rule->p.udpliteHdrFilter.ipHdr);
break;
case VIR_NWFILTER_RULE_PROTOCOL_ESP:
case VIR_NWFILTER_RULE_PROTOCOL_ESPoIPV6:
- COPY_NEG_SIGN(rule->p.espHdrFilter.ipHdr.dataSrcIPMask,
- rule->p.espHdrFilter.ipHdr.dataSrcIPAddr);
- COPY_NEG_SIGN(rule->p.espHdrFilter.ipHdr.dataDstIPMask,
- rule->p.espHdrFilter.ipHdr.dataDstIPAddr);
- COPY_NEG_SIGN(rule->p.espHdrFilter.ipHdr.dataSrcIPTo,
- rule->p.espHdrFilter.ipHdr.dataSrcIPFrom);
- COPY_NEG_SIGN(rule->p.espHdrFilter.ipHdr.dataDstIPTo,
- rule->p.espHdrFilter.ipHdr.dataDstIPFrom);
+ ipHdr = &rule->p.espHdrFilter.ipHdr;
+ COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
+ COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
virNWFilterRuleDefFixupIPSet(&rule->p.espHdrFilter.ipHdr);
break;
case VIR_NWFILTER_RULE_PROTOCOL_AH:
case VIR_NWFILTER_RULE_PROTOCOL_AHoIPV6:
- COPY_NEG_SIGN(rule->p.ahHdrFilter.ipHdr.dataSrcIPMask,
- rule->p.ahHdrFilter.ipHdr.dataSrcIPAddr);
- COPY_NEG_SIGN(rule->p.ahHdrFilter.ipHdr.dataDstIPMask,
- rule->p.ahHdrFilter.ipHdr.dataDstIPAddr);
- COPY_NEG_SIGN(rule->p.ahHdrFilter.ipHdr.dataSrcIPTo,
- rule->p.ahHdrFilter.ipHdr.dataSrcIPFrom);
- COPY_NEG_SIGN(rule->p.ahHdrFilter.ipHdr.dataDstIPTo,
- rule->p.ahHdrFilter.ipHdr.dataDstIPFrom);
+ ipHdr = &rule->p.ahHdrFilter.ipHdr;
+ COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
+ COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
virNWFilterRuleDefFixupIPSet(&rule->p.ahHdrFilter.ipHdr);
break;
case VIR_NWFILTER_RULE_PROTOCOL_SCTP:
case VIR_NWFILTER_RULE_PROTOCOL_SCTPoIPV6:
- COPY_NEG_SIGN(rule->p.sctpHdrFilter.ipHdr.dataSrcIPMask,
- rule->p.sctpHdrFilter.ipHdr.dataSrcIPAddr);
- COPY_NEG_SIGN(rule->p.sctpHdrFilter.ipHdr.dataDstIPMask,
- rule->p.sctpHdrFilter.ipHdr.dataDstIPAddr);
- COPY_NEG_SIGN(rule->p.sctpHdrFilter.ipHdr.dataSrcIPTo,
- rule->p.sctpHdrFilter.ipHdr.dataSrcIPFrom);
- COPY_NEG_SIGN(rule->p.sctpHdrFilter.ipHdr.dataDstIPTo,
- rule->p.sctpHdrFilter.ipHdr.dataDstIPFrom);
+ ipHdr = &rule->p.sctpHdrFilter.ipHdr;
+ COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
+ COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
COPY_NEG_SIGN(rule->p.sctpHdrFilter.portData.dataSrcPortEnd,
rule->p.sctpHdrFilter.portData.dataSrcPortStart);
COPY_NEG_SIGN(rule->p.sctpHdrFilter.portData.dataDstPortStart,
@@ -2358,14 +2339,11 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
case VIR_NWFILTER_RULE_PROTOCOL_ICMP:
case VIR_NWFILTER_RULE_PROTOCOL_ICMPV6:
- COPY_NEG_SIGN(rule->p.icmpHdrFilter.ipHdr.dataSrcIPMask,
- rule->p.icmpHdrFilter.ipHdr.dataSrcIPAddr);
- COPY_NEG_SIGN(rule->p.icmpHdrFilter.ipHdr.dataDstIPMask,
- rule->p.icmpHdrFilter.ipHdr.dataDstIPAddr);
- COPY_NEG_SIGN(rule->p.icmpHdrFilter.ipHdr.dataSrcIPTo,
- rule->p.icmpHdrFilter.ipHdr.dataSrcIPFrom);
- COPY_NEG_SIGN(rule->p.icmpHdrFilter.ipHdr.dataDstIPTo,
- rule->p.icmpHdrFilter.ipHdr.dataDstIPFrom);
+ ipHdr = &rule->p.icmpHdrFilter.ipHdr;
+ COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
+ COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
COPY_NEG_SIGN(rule->p.icmpHdrFilter.dataICMPCode,
rule->p.icmpHdrFilter.dataICMPType);
virNWFilterRuleDefFixupIPSet(&rule->p.icmpHdrFilter.ipHdr);
@@ -2373,25 +2351,19 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
case VIR_NWFILTER_RULE_PROTOCOL_ALL:
case VIR_NWFILTER_RULE_PROTOCOL_ALLoIPV6:
- COPY_NEG_SIGN(rule->p.allHdrFilter.ipHdr.dataSrcIPMask,
- rule->p.allHdrFilter.ipHdr.dataSrcIPAddr);
- COPY_NEG_SIGN(rule->p.allHdrFilter.ipHdr.dataDstIPMask,
- rule->p.allHdrFilter.ipHdr.dataDstIPAddr);
- COPY_NEG_SIGN(rule->p.allHdrFilter.ipHdr.dataSrcIPTo,
- rule->p.allHdrFilter.ipHdr.dataSrcIPFrom);
- COPY_NEG_SIGN(rule->p.allHdrFilter.ipHdr.dataDstIPTo,
- rule->p.allHdrFilter.ipHdr.dataDstIPFrom);
+ ipHdr = &rule->p.allHdrFilter.ipHdr;
+ COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
+ COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
break;
case VIR_NWFILTER_RULE_PROTOCOL_IGMP:
- COPY_NEG_SIGN(rule->p.igmpHdrFilter.ipHdr.dataSrcIPMask,
- rule->p.igmpHdrFilter.ipHdr.dataSrcIPAddr);
- COPY_NEG_SIGN(rule->p.igmpHdrFilter.ipHdr.dataDstIPMask,
- rule->p.igmpHdrFilter.ipHdr.dataDstIPAddr);
- COPY_NEG_SIGN(rule->p.igmpHdrFilter.ipHdr.dataSrcIPTo,
- rule->p.igmpHdrFilter.ipHdr.dataSrcIPFrom);
- COPY_NEG_SIGN(rule->p.igmpHdrFilter.ipHdr.dataDstIPTo,
- rule->p.igmpHdrFilter.ipHdr.dataDstIPFrom);
+ ipHdr = &rule->p.igmpHdrFilter.ipHdr;
+ COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
+ COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
+ COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
virNWFilterRuleDefFixupIPSet(&rule->p.igmpHdrFilter.ipHdr);
break;
--
2.31.1
7:58 a.m.
New subject: [libvirt PATCH 3/3] virNWFilterRuleDefFixup: Factor out portData as variable
This helps to bring down the frame size of virNWFilterRuleDefFixup, as it
exceeds 8192 bytes when libvirt is build with sanitizers enabled, in debug
mode, on clang.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/nwfilter_conf.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 2448e4b70b..ad1edc98dd 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2191,6 +2191,7 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
{
ethHdrDataDef *ethHdr;
ipHdrDataDef *ipHdr;
+ portDataDef *portData;
#define COPY_NEG_SIGN(A, B) \
(A).flags = ((A).flags & ~NWFILTER_ENTRY_ITEM_FLAG_IS_NEG) | \
@@ -2262,32 +2263,28 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
case VIR_NWFILTER_RULE_PROTOCOL_TCP:
case VIR_NWFILTER_RULE_PROTOCOL_TCPoIPV6:
ipHdr = &rule->p.tcpHdrFilter.ipHdr;
+ portData = &rule->p.tcpHdrFilter.portData;
COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
- COPY_NEG_SIGN(rule->p.tcpHdrFilter.portData.dataSrcPortEnd,
- rule->p.tcpHdrFilter.portData.dataSrcPortStart);
- COPY_NEG_SIGN(rule->p.tcpHdrFilter.portData.dataDstPortStart,
- rule->p.tcpHdrFilter.portData.dataSrcPortStart);
- COPY_NEG_SIGN(rule->p.tcpHdrFilter.portData.dataDstPortEnd,
- rule->p.tcpHdrFilter.portData.dataSrcPortStart);
+ COPY_NEG_SIGN(portData->dataSrcPortEnd, portData->dataSrcPortStart);
+ COPY_NEG_SIGN(portData->dataDstPortStart, portData->dataSrcPortStart);
+ COPY_NEG_SIGN(portData->dataDstPortEnd, portData->dataSrcPortStart);
virNWFilterRuleDefFixupIPSet(&rule->p.tcpHdrFilter.ipHdr);
break;
case VIR_NWFILTER_RULE_PROTOCOL_UDP:
case VIR_NWFILTER_RULE_PROTOCOL_UDPoIPV6:
ipHdr = &rule->p.udpHdrFilter.ipHdr;
+ portData = &rule->p.udpHdrFilter.portData;
COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
- COPY_NEG_SIGN(rule->p.udpHdrFilter.portData.dataSrcPortEnd,
- rule->p.udpHdrFilter.portData.dataSrcPortStart);
- COPY_NEG_SIGN(rule->p.udpHdrFilter.portData.dataDstPortStart,
- rule->p.udpHdrFilter.portData.dataSrcPortStart);
- COPY_NEG_SIGN(rule->p.udpHdrFilter.portData.dataDstPortEnd,
- rule->p.udpHdrFilter.portData.dataSrcPortStart);
+ COPY_NEG_SIGN(portData->dataSrcPortEnd, portData->dataSrcPortStart);
+ COPY_NEG_SIGN(portData->dataDstPortStart, portData->dataSrcPortStart);
+ COPY_NEG_SIGN(portData->dataDstPortEnd, portData->dataSrcPortStart);
virNWFilterRuleDefFixupIPSet(&rule->p.udpHdrFilter.ipHdr);
break;
@@ -2324,16 +2321,14 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
case VIR_NWFILTER_RULE_PROTOCOL_SCTP:
case VIR_NWFILTER_RULE_PROTOCOL_SCTPoIPV6:
ipHdr = &rule->p.sctpHdrFilter.ipHdr;
+ portData = &rule->p.sctpHdrFilter.portData;
COPY_NEG_SIGN(ipHdr->dataSrcIPMask, ipHdr->dataSrcIPAddr);
COPY_NEG_SIGN(ipHdr->dataDstIPMask, ipHdr->dataDstIPAddr);
COPY_NEG_SIGN(ipHdr->dataSrcIPTo, ipHdr->dataSrcIPFrom);
COPY_NEG_SIGN(ipHdr->dataDstIPTo, ipHdr->dataDstIPFrom);
- COPY_NEG_SIGN(rule->p.sctpHdrFilter.portData.dataSrcPortEnd,
- rule->p.sctpHdrFilter.portData.dataSrcPortStart);
- COPY_NEG_SIGN(rule->p.sctpHdrFilter.portData.dataDstPortStart,
- rule->p.sctpHdrFilter.portData.dataSrcPortStart);
- COPY_NEG_SIGN(rule->p.sctpHdrFilter.portData.dataDstPortEnd,
- rule->p.sctpHdrFilter.portData.dataSrcPortStart);
+ COPY_NEG_SIGN(portData->dataSrcPortEnd, portData->dataSrcPortStart);
+ COPY_NEG_SIGN(portData->dataDstPortStart, portData->dataSrcPortStart);
+ COPY_NEG_SIGN(portData->dataDstPortEnd, portData->dataSrcPortStart);
virNWFilterRuleDefFixupIPSet(&rule->p.sctpHdrFilter.ipHdr);
break;
--
2.31.1
8:10 a.m.
On Fri, Sep 17, 2021 at 02:58:08PM +0200, Tim Wiederhake wrote:
When libvirt is build with sanitizers enabled, in debug mode, on
clang,
virNWFilterRuleDefFixup exceeds the maximum stack frame size of 8192 bytes,
as specified in meson.build:
../src/conf/nwfilter_conf.c:2190:1: error: stack frame size of 10616
bytes in function 'virNWFilterRuleDefFixup' [-Werror,-Wframe-larger-than=]
virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
^
1 error generated.
This series reworks the function a bit to bring the frame size below 8192.
Why don't we just enlarge stack size limit for building with
sanitizers ?
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
8:37 a.m.
On Fri, 2021-09-17 at 14:10 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 17, 2021 at 02:58:08PM +0200, Tim Wiederhake wrote:
> When libvirt is build with sanitizers enabled, in debug mode, on
> clang,
> virNWFilterRuleDefFixup exceeds the maximum stack frame size of
> 8192 bytes,
> as specified in meson.build:
>
> ../src/conf/nwfilter_conf.c:2190:1: error: stack frame size of
> 10616
> bytes in function 'virNWFilterRuleDefFixup' [-Werror,-Wframe-
> larger-than=]
> virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
> ^
> 1 error generated.
>
> This series reworks the function a bit to bring the frame size
> below 8192.
Why don't we just enlarge stack size limit for building with
sanitizers ?
Regards,
Daniel
We already double the stack size limit for debug builds, and increasing
it further seemed excessive.
Note that there is one more function that exceeds the limit,
virDomainDefParseXML, for which I have a patch ready. I will send it
once this series lands.
Regards,
Tim
9:03 a.m.
On Fri, Sep 17, 2021 at 03:37:28PM +0200, Tim Wiederhake wrote:
On Fri, 2021-09-17 at 14:10 +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 17, 2021 at 02:58:08PM +0200, Tim Wiederhake wrote:
> > When libvirt is build with sanitizers enabled, in debug mode, on
> > clang,
> > virNWFilterRuleDefFixup exceeds the maximum stack frame size of
> > 8192 bytes,
> > as specified in meson.build:
> >
> > ../src/conf/nwfilter_conf.c:2190:1: error: stack frame size of
> > 10616
> > bytes in function 'virNWFilterRuleDefFixup' [-Werror,-Wframe-
> > larger-than=]
> > virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
> > ^
> > 1 error generated.
> >
> > This series reworks the function a bit to bring the frame size
> > below 8192.
>
> Why don't we just enlarge stack size limit for building with
> sanitizers ?
>
>
> Regards,
> Daniel
We already double the stack size limit for debug builds, and increasing
it further seemed excessive.
Note that there is one more function that exceeds the limit,
virDomainDefParseXML, for which I have a patch ready. I will send it
once this series lands.
We're not using sanitizers in production builds though, so IMHO we could
even just run with no stack size checking entirely for such builds.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
10:01 a.m.
On Fri, 2021-09-17 at 15:03 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 17, 2021 at 03:37:28PM +0200, Tim Wiederhake wrote:
> On Fri, 2021-09-17 at 14:10 +0100, Daniel P. Berrangé wrote:
> > On Fri, Sep 17, 2021 at 02:58:08PM +0200, Tim Wiederhake wrote:
> > > When libvirt is build with sanitizers enabled, in debug mode, on
> > > clang,
> > > virNWFilterRuleDefFixup exceeds the maximum stack frame size of
> > > 8192 bytes,
> > > as specified in meson.build:
> > >
> > > ../src/conf/nwfilter_conf.c:2190:1: error: stack frame size of
> > > 10616
> > > bytes in function 'virNWFilterRuleDefFixup' [-Werror,-Wframe-
> > > larger-than=]
> > > virNWFilterRuleDefFixup(virNWFilterRuleDef *rule)
> > > ^
> > > 1 error generated.
> > >
> > > This series reworks the function a bit to bring the frame size
> > > below 8192.
> >
> > Why don't we just enlarge stack size limit for building with
> > sanitizers ?
> >
> >
> > Regards,
> > Daniel
>
> We already double the stack size limit for debug builds, and
> increasing
> it further seemed excessive.
s/debug builds/sanitizer builds/ -- my mistake.
>
> Note that there is one more function that exceeds the limit,
> virDomainDefParseXML, for which I have a patch ready. I will send it
> once this series lands.
We're not using sanitizers in production builds though, so IMHO we
could
even just run with no stack size checking entirely for such builds.
My local builds have "-Dbuildtype=debug". In gitlab's CI, the buildtype
is not explictly set. This difference is what triggers the stack frame
size warnings:
$ export CC=clang
$ meson -Db_sanitize=address,undefined -Db_lundef=false buildtype-
default &> /dev/null
$ ninja -C buildtype-default &>/dev/null && echo "ok" ||
echo
"fail"
ok
$ meson -Db_sanitize=address,undefined -Db_lundef=false -
Dbuildtype=debug buildtype-debug &> /dev/null
$ ninja -C buildtype-debug >/dev/null && echo "ok" || echo
"fail"
fail
Which is weird, as meson's default build type is "debug".
> > > When libvirt is build with sanitizers enabled, in
debug mode,
> > > on clang, virNWFilterRuleDefFixup exceeds the maximum stack
> > > frame size of 8192 bytes, as specified in meson.build:
s/in debug mode/buildtype explicitly set to debug/
Regards,
Tim
1162
days inactive
1162
days old
10 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Tim Wiederhake