[PATCH] build: clang stack frame size handling improvement

Roman Bogorodskiy posted 1 patch 10 months, 1 week ago
Failed in applying to current master (apply log)
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] build: clang stack frame size handling improvement
Posted by Roman Bogorodskiy 10 months, 1 week ago
The 'plain' optimization type also triggers the clang stack frame size
issues, so increase limit for it as well.

Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 56823ca25b..0a402a19a2 100644
--- a/meson.build
+++ b/meson.build
@@ -259,7 +259,7 @@ alloc_max = run_command(
 stack_frame_size = 2048
 
 # clang without optimization enlarges stack frames in certain corner cases
-if cc.get_id() == 'clang' and get_option('optimization') == '0'
+if cc.get_id() == 'clang' and get_option('optimization') in ['plain', '0']
     stack_frame_size = 4096
 endif
 
-- 
2.49.0
Re: [PATCH] build: clang stack frame size handling improvement
Posted by Michal Prívozník via Devel 10 months, 1 week ago
On 4/2/25 19:24, Roman Bogorodskiy wrote:
> The 'plain' optimization type also triggers the clang stack frame size
> issues, so increase limit for it as well.
> 
> Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
> ---
>  meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 56823ca25b..0a402a19a2 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -259,7 +259,7 @@ alloc_max = run_command(
>  stack_frame_size = 2048
>  
>  # clang without optimization enlarges stack frames in certain corner cases
> -if cc.get_id() == 'clang' and get_option('optimization') == '0'
> +if cc.get_id() == 'clang' and get_option('optimization') in ['plain', '0']
>      stack_frame_size = 4096
>  endif
>  

Funny, with clang I hit this issue for all possible values of
--optimization {plain,0,g,1,2,3,s}.

I worry this is clang version dependent. Should we just drop check for
'optimization' argument altogether?

Michal
Re: [PATCH] build: clang stack frame size handling improvement
Posted by Roman Bogorodskiy 10 months, 1 week ago
  Michal Prívozník wrote:

> On 4/2/25 19:24, Roman Bogorodskiy wrote:
> > The 'plain' optimization type also triggers the clang stack frame size
> > issues, so increase limit for it as well.
> > 
> > Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
> > ---
> >  meson.build | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/meson.build b/meson.build
> > index 56823ca25b..0a402a19a2 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -259,7 +259,7 @@ alloc_max = run_command(
> >  stack_frame_size = 2048
> >  
> >  # clang without optimization enlarges stack frames in certain corner cases
> > -if cc.get_id() == 'clang' and get_option('optimization') == '0'
> > +if cc.get_id() == 'clang' and get_option('optimization') in ['plain', '0']
> >      stack_frame_size = 4096
> >  endif
> >  
> 
> Funny, with clang I hit this issue for all possible values of
> --optimization {plain,0,g,1,2,3,s}.

That's interesting indeed.

Currently, "plain" is the only value that triggers the issue for me:

Problematic file is the following:

../src/remote/remote_driver.c:790:1: error: stack frame size (2344) exceeds limit (2048) in 'doRemoteOpen' [-Werror,-Wframe-larger-than]
  790 | doRemoteOpen(virConnectPtr conn,
      | ^
1 error generated.

Clang version is 19.1.7. 

Roman
Re: [PATCH] build: clang stack frame size handling improvement
Posted by Michal Prívozník via Devel 10 months, 1 week ago
On 4/3/25 18:28, Roman Bogorodskiy wrote:
>   Michal Prívozník wrote:
> 
>> On 4/2/25 19:24, Roman Bogorodskiy wrote:
>>> The 'plain' optimization type also triggers the clang stack frame size
>>> issues, so increase limit for it as well.
>>>
>>> Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
>>> ---
>>>  meson.build | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 56823ca25b..0a402a19a2 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -259,7 +259,7 @@ alloc_max = run_command(
>>>  stack_frame_size = 2048
>>>  
>>>  # clang without optimization enlarges stack frames in certain corner cases
>>> -if cc.get_id() == 'clang' and get_option('optimization') == '0'
>>> +if cc.get_id() == 'clang' and get_option('optimization') in ['plain', '0']
>>>      stack_frame_size = 4096
>>>  endif
>>>  
>>
>> Funny, with clang I hit this issue for all possible values of
>> --optimization {plain,0,g,1,2,3,s}.
> 
> That's interesting indeed.
> 
> Currently, "plain" is the only value that triggers the issue for me:
> 
> Problematic file is the following:
> 
> ../src/remote/remote_driver.c:790:1: error: stack frame size (2344) exceeds limit (2048) in 'doRemoteOpen' [-Werror,-Wframe-larger-than]
>   790 | doRemoteOpen(virConnectPtr conn,
>       | ^
> 1 error generated.
> 
> Clang version is 19.1.7. 

Same here:

clang version 19.1.7
Target: x86_64-pc-linux-gnu

except I'm running Linux and I assume you're running *BSD. Given what
also Dan said, are you willing to post a v2 where the condition is
simplified to just 'clang' and no get_option('optimization')?

Actually, give me a minute or to - maybe there's something I can do
about doRemoteOpen() so that its stack isn't that huge.

Michal
Re: [PATCH] build: clang stack frame size handling improvement
Posted by Michal Prívozník via Devel 10 months, 1 week ago
On 4/4/25 08:46, Michal Prívozník wrote:
> On 4/3/25 18:28, Roman Bogorodskiy wrote:
>>   Michal Prívozník wrote:
>>
>>> On 4/2/25 19:24, Roman Bogorodskiy wrote:
>>>> The 'plain' optimization type also triggers the clang stack frame size
>>>> issues, so increase limit for it as well.
>>>>
>>>> Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
>>>> ---
>>>>  meson.build | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/meson.build b/meson.build
>>>> index 56823ca25b..0a402a19a2 100644
>>>> --- a/meson.build
>>>> +++ b/meson.build
>>>> @@ -259,7 +259,7 @@ alloc_max = run_command(
>>>>  stack_frame_size = 2048
>>>>  
>>>>  # clang without optimization enlarges stack frames in certain corner cases
>>>> -if cc.get_id() == 'clang' and get_option('optimization') == '0'
>>>> +if cc.get_id() == 'clang' and get_option('optimization') in ['plain', '0']
>>>>      stack_frame_size = 4096
>>>>  endif
>>>>  
>>>
>>> Funny, with clang I hit this issue for all possible values of
>>> --optimization {plain,0,g,1,2,3,s}.
>>
>> That's interesting indeed.
>>
>> Currently, "plain" is the only value that triggers the issue for me:
>>
>> Problematic file is the following:
>>
>> ../src/remote/remote_driver.c:790:1: error: stack frame size (2344) exceeds limit (2048) in 'doRemoteOpen' [-Werror,-Wframe-larger-than]
>>   790 | doRemoteOpen(virConnectPtr conn,
>>       | ^
>> 1 error generated.
>>
>> Clang version is 19.1.7. 
> 
> Same here:
> 
> clang version 19.1.7
> Target: x86_64-pc-linux-gnu
> 
> except I'm running Linux and I assume you're running *BSD. Given what
> also Dan said, are you willing to post a v2 where the condition is
> simplified to just 'clang' and no get_option('optimization')?
> 
> Actually, give me a minute or to - maybe there's something I can do
> about doRemoteOpen() so that its stack isn't that huge.

Alight, I think I know what's going on and why we're getting 2300+ bytes
long stack even with just a few variables declared.

Shortly: it's because of glib.

Longer version:
glib declares plenty of helper variables (mostly for type checking). For
instance:

g_clear_pointer() is declared as:

#define g_clear_pointer(pp, destroy)                     \
  G_STMT_START                                           \
  {                                                      \
    G_STATIC_ASSERT (sizeof *(pp) == sizeof (gpointer)); \
    glib_typeof ((pp)) _pp = (pp);                       \
    glib_typeof (*(pp)) _ptr = *_pp;                     \
    *_pp = NULL;                                         \
    if (_ptr)                                            \
      (destroy) (_ptr);                                  \
  }                                                      \
  G_STMT_END                                             \

And our VIR_FREE() is defined to be g_clear_pointer().

Then, g_strdup() is defined to be g_strdup_inline(), which: a) is
declared as always inline, and b) declares two additional variables:

G_ALWAYS_INLINE static inline char *
g_strdup_inline (const char *str)
{
  if (__builtin_constant_p (!str) && !str)
    return NULL;

  if (__builtin_constant_p (!!str) && !!str && __builtin_constant_p
(strlen (str)))
    {
      const size_t len = strlen (str) + 1;
      char *dup_str = (char *) g_malloc (len);
      return (char *) memcpy (dup_str, str, len);
    }

  return g_strdup (str);
}


And finally, for some reason, attribute cleanup also increases stack
size (observed by playing with godbolt.org) - but this is for both gcc
and clang and isn't glib related. Honestly, I do not understand why
attribute cleanup would need to increase stack size, but whatever. I
think now I have all the ingredients needed to break the function down
into smaller pieces. So hopefully, in the end, this whole workaround
might be dropped.

NB: there's still vboxSnapshotRedefine() which has even bigger stack:

../src/vbox/vbox_common.c:4557:1: error: stack frame size (2824) exceeds
limit (2048) in 'vboxSnapshotRedefine' [-Werror,-Wframe-larger-than]

but I think (wishfully) the same strategy can be applied there.

Michal
Re: [PATCH] build: clang stack frame size handling improvement
Posted by Roman Bogorodskiy 10 months, 1 week ago
  Michal Prívozník wrote:

> On 4/4/25 08:46, Michal Prívozník wrote:
> > On 4/3/25 18:28, Roman Bogorodskiy wrote:
> >>   Michal Prívozník wrote:
> >>
> >>> On 4/2/25 19:24, Roman Bogorodskiy wrote:
> >>>> The 'plain' optimization type also triggers the clang stack frame size
> >>>> issues, so increase limit for it as well.
> >>>>
> >>>> Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
> >>>> ---
> >>>>  meson.build | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/meson.build b/meson.build
> >>>> index 56823ca25b..0a402a19a2 100644
> >>>> --- a/meson.build
> >>>> +++ b/meson.build
> >>>> @@ -259,7 +259,7 @@ alloc_max = run_command(
> >>>>  stack_frame_size = 2048
> >>>>  
> >>>>  # clang without optimization enlarges stack frames in certain corner cases
> >>>> -if cc.get_id() == 'clang' and get_option('optimization') == '0'
> >>>> +if cc.get_id() == 'clang' and get_option('optimization') in ['plain', '0']
> >>>>      stack_frame_size = 4096
> >>>>  endif
> >>>>  
> >>>
> >>> Funny, with clang I hit this issue for all possible values of
> >>> --optimization {plain,0,g,1,2,3,s}.
> >>
> >> That's interesting indeed.
> >>
> >> Currently, "plain" is the only value that triggers the issue for me:
> >>
> >> Problematic file is the following:
> >>
> >> ../src/remote/remote_driver.c:790:1: error: stack frame size (2344) exceeds limit (2048) in 'doRemoteOpen' [-Werror,-Wframe-larger-than]
> >>   790 | doRemoteOpen(virConnectPtr conn,
> >>       | ^
> >> 1 error generated.
> >>
> >> Clang version is 19.1.7. 
> > 
> > Same here:
> > 
> > clang version 19.1.7
> > Target: x86_64-pc-linux-gnu
> > 
> > except I'm running Linux and I assume you're running *BSD. Given what
> > also Dan said, are you willing to post a v2 where the condition is
> > simplified to just 'clang' and no get_option('optimization')?
> > 
> > Actually, give me a minute or to - maybe there's something I can do
> > about doRemoteOpen() so that its stack isn't that huge.
> 
> Alight, I think I know what's going on and why we're getting 2300+ bytes
> long stack even with just a few variables declared.
> 
> Shortly: it's because of glib.
> 
> Longer version:
> glib declares plenty of helper variables (mostly for type checking). For
> instance:
> 
> g_clear_pointer() is declared as:
> 
> #define g_clear_pointer(pp, destroy)                     \
>   G_STMT_START                                           \
>   {                                                      \
>     G_STATIC_ASSERT (sizeof *(pp) == sizeof (gpointer)); \
>     glib_typeof ((pp)) _pp = (pp);                       \
>     glib_typeof (*(pp)) _ptr = *_pp;                     \
>     *_pp = NULL;                                         \
>     if (_ptr)                                            \
>       (destroy) (_ptr);                                  \
>   }                                                      \
>   G_STMT_END                                             \
> 
> And our VIR_FREE() is defined to be g_clear_pointer().
> 
> Then, g_strdup() is defined to be g_strdup_inline(), which: a) is
> declared as always inline, and b) declares two additional variables:
> 
> G_ALWAYS_INLINE static inline char *
> g_strdup_inline (const char *str)
> {
>   if (__builtin_constant_p (!str) && !str)
>     return NULL;
> 
>   if (__builtin_constant_p (!!str) && !!str && __builtin_constant_p
> (strlen (str)))
>     {
>       const size_t len = strlen (str) + 1;
>       char *dup_str = (char *) g_malloc (len);
>       return (char *) memcpy (dup_str, str, len);
>     }
> 
>   return g_strdup (str);
> }
> 
> 
> And finally, for some reason, attribute cleanup also increases stack
> size (observed by playing with godbolt.org) - but this is for both gcc
> and clang and isn't glib related. Honestly, I do not understand why
> attribute cleanup would need to increase stack size, but whatever. I
> think now I have all the ingredients needed to break the function down
> into smaller pieces. So hopefully, in the end, this whole workaround
> might be dropped.
> 
> NB: there's still vboxSnapshotRedefine() which has even bigger stack:
> 
> ../src/vbox/vbox_common.c:4557:1: error: stack frame size (2824) exceeds
> limit (2048) in 'vboxSnapshotRedefine' [-Werror,-Wframe-larger-than]
> 
> but I think (wishfully) the same strategy can be applied there.

IIRC, the first time I've encountered this issue was actually a test
file. So I guess it would still make sense to keep the workaround for
clang for now, dropping the optimization condition?

Roman
Re: [PATCH] build: clang stack frame size handling improvement
Posted by Michal Prívozník via Devel 10 months, 1 week ago
On 4/4/25 11:02, Roman Bogorodskiy wrote:
>   Michal Prívozník wrote:
> 
>>  <snip/>
> 
> IIRC, the first time I've encountered this issue was actually a test
> file. So I guess it would still make sense to keep the workaround for
> clang for now, dropping the optimization condition?

Maybe, but I haven't found anything there. I've tested these
successfully on my FreeBSD VM:

https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/XT2HHWXNM64FLLVOHECB3DE72JMYQXKQ/

Michal
Re: [PATCH] build: clang stack frame size handling improvement
Posted by Daniel P. Berrangé via Devel 10 months, 1 week ago
On Thu, Apr 03, 2025 at 02:08:28PM +0200, Michal Prívozník via Devel wrote:
> On 4/2/25 19:24, Roman Bogorodskiy wrote:
> > The 'plain' optimization type also triggers the clang stack frame size
> > issues, so increase limit for it as well.
> > 
> > Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
> > ---
> >  meson.build | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/meson.build b/meson.build
> > index 56823ca25b..0a402a19a2 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -259,7 +259,7 @@ alloc_max = run_command(
> >  stack_frame_size = 2048
> >  
> >  # clang without optimization enlarges stack frames in certain corner cases
> > -if cc.get_id() == 'clang' and get_option('optimization') == '0'
> > +if cc.get_id() == 'clang' and get_option('optimization') in ['plain', '0']
> >      stack_frame_size = 4096
> >  endif
> >  
> 
> Funny, with clang I hit this issue for all possible values of
> --optimization {plain,0,g,1,2,3,s}.
> 
> I worry this is clang version dependent. Should we just drop check for
> 'optimization' argument altogether?

We originally picked 2k default in

  commit 42bc76cdb8486ef502200f3bce9e3faebdd78103
  Author: Peter Krempa <pkrempa@redhat.com>
  Date:   Mon Sep 5 14:38:09 2022 +0200

    build: Decrease maximum stack frame size to 2048
    
    After recent cleanups we can now restrict the maximum stack frame size
    to 2k.

guess we must be just a bit too aggressive with certain compiler
scenarios - with various hardening countermeasures compilers may
choose to apply, stack size can be bigger than we might otherwise
expect.

With 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 :|