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 <bogorods...@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
