COMMAND_LINE_SIZE and CONFIG_FRAME_WARN

[Atsushi Nemoto]

Recently COMMAND_LINE_SIZE (CL_SIZE) was extended to 4096 from 512.
(commit 22242681 "MIPS: Extend COMMAND_LINE_SIZE")

This cause warning if something like buf[CL_SIZE] was declared as a
local variable, for example in prom_init_cmdline() on some platforms.

And since many Makefiles in arch/mips enables -Werror, this cause
build failure.

How can we avoid this error?

- do not use local array? (but dynamic allocation cannot be used in
  such an early stage.  static array?)
- are there any way to disable -Wframe-larger-than for the file or function?
- make COMMAND_LINE_SIZE customizable?
- use non default CONFIG_FRAME_WARN?

Any comments or suggestions?
---
Atsushi Nemoto

[Atsushi Nemoto]

On Sat, 07 Nov 2009 01:08:39 +0900 (JST), Atsushi Nemoto <anemo@...> wrote:
> Recently COMMAND_LINE_SIZE (CL_SIZE) was extended to 4096 from 512.
> (commit 22242681 "MIPS: Extend COMMAND_LINE_SIZE")

Excuse me, old COMMAND_LINE_SIZE value was 256, not 512.

---
Atsushi Nemoto

[Wu Zhangjin]

On Sat, 2009-11-07 at 01:08 +0900, Atsushi Nemoto wrote:

> Recently COMMAND_LINE_SIZE (CL_SIZE) was extended to 4096 from 512.
> (commit 22242681 "MIPS: Extend COMMAND_LINE_SIZE")
>
> This cause warning if something like buf[CL_SIZE] was declared as a
> local variable, for example in prom_init_cmdline() on some platforms.
>
> And since many Makefiles in arch/mips enables -Werror, this cause
> build failure.
>
> How can we avoid this error?
>
> - do not use local array? (but dynamic allocation cannot be used in
>   such an early stage.  static array?)
> - are there any way to disable -Wframe-larger-than for the file or function?
> - make COMMAND_LINE_SIZE customizable?
> - use non default CONFIG_FRAME_WARN?
>
> Any comments or suggestions?

does "static" helps this problem?

Regards,
        Wu Zhangjin

[Dmitri Vorobiev]

On Fri, Nov 6, 2009 at 6:08 PM, Atsushi Nemoto <anemo@...> wrote:

> Recently COMMAND_LINE_SIZE (CL_SIZE) was extended to 4096 from 512.
> (commit 22242681 "MIPS: Extend COMMAND_LINE_SIZE")
>
> This cause warning if something like buf[CL_SIZE] was declared as a
> local variable, for example in prom_init_cmdline() on some platforms.
>
> And since many Makefiles in arch/mips enables -Werror, this cause
> build failure.
>
> How can we avoid this error?
>
> - do not use local array? (but dynamic allocation cannot be used in
>  such an early stage.  static array?)

Maybe a static array marked with __initdata?

Dmitri

[Dmitri Vorobiev]

On Fri, Nov 6, 2009 at 6:22 PM, Dmitri Vorobiev
<dmitri.vorobiev@...> wrote:

> On Fri, Nov 6, 2009 at 6:08 PM, Atsushi Nemoto <anemo@...> wrote:
>> Recently COMMAND_LINE_SIZE (CL_SIZE) was extended to 4096 from 512.
>> (commit 22242681 "MIPS: Extend COMMAND_LINE_SIZE")
>>
>> This cause warning if something like buf[CL_SIZE] was declared as a
>> local variable, for example in prom_init_cmdline() on some platforms.
>>
>> And since many Makefiles in arch/mips enables -Werror, this cause
>> build failure.
>>
>> How can we avoid this error?
>>
>> - do not use local array? (but dynamic allocation cannot be used in
>>  such an early stage.  static array?)
>
> Maybe a static array marked with __initdata?

Also, I just thought that maybe it's possible to use a c99
variable-length array here? Like this:

int n = COMMAND_LINE_SIZE;
char buf[n];

This way, we don't put yet another variable in the .init.data section,
unlike with the static array solution.

However, this is totally untested, just a thought...

Dmitri

>
> Dmitri
>

[Maciej W. Rozycki]

On Sat, 7 Nov 2009, Atsushi Nemoto wrote:

> Recently COMMAND_LINE_SIZE (CL_SIZE) was extended to 4096 from 512.
> (commit 22242681 "MIPS: Extend COMMAND_LINE_SIZE")
>
> This cause warning if something like buf[CL_SIZE] was declared as a
> local variable, for example in prom_init_cmdline() on some platforms.
>
> And since many Makefiles in arch/mips enables -Werror, this cause
> build failure.
>
> How can we avoid this error?
>
> - do not use local array? (but dynamic allocation cannot be used in
>   such an early stage.  static array?)
> - are there any way to disable -Wframe-larger-than for the file or function?
> - make COMMAND_LINE_SIZE customizable?
> - use non default CONFIG_FRAME_WARN?
>
> Any comments or suggestions?

 Use static storage and mark it initdata?  You can certainly override
-Wframe-larger-than in per-object CFLAGS too.  You might also be able to
use __builtin_alloca(), but this sounds like a shaky ground. ;)

  Maciej

[Dmitri Vorobiev]

On Fri, Nov 6, 2009 at 6:34 PM, Dmitri Vorobiev
<dmitri.vorobiev@...> wrote:

> On Fri, Nov 6, 2009 at 6:22 PM, Dmitri Vorobiev
> <dmitri.vorobiev@...> wrote:
>> On Fri, Nov 6, 2009 at 6:08 PM, Atsushi Nemoto <anemo@...> wrote:
>>> Recently COMMAND_LINE_SIZE (CL_SIZE) was extended to 4096 from 512.
>>> (commit 22242681 "MIPS: Extend COMMAND_LINE_SIZE")
>>>
>>> This cause warning if something like buf[CL_SIZE] was declared as a
>>> local variable, for example in prom_init_cmdline() on some platforms.
>>>
>>> And since many Makefiles in arch/mips enables -Werror, this cause
>>> build failure.
>>>
>>> How can we avoid this error?
>>>
>>> - do not use local array? (but dynamic allocation cannot be used in
>>>  such an early stage.  static array?)
>>
>> Maybe a static array marked with __initdata?
>
> Also, I just thought that maybe it's possible to use a c99
> variable-length array here? Like this:
>
> int n = COMMAND_LINE_SIZE;
> char buf[n];
>
> This way, we don't put yet another variable in the .init.data section,
> unlike with the static array solution.
>
> However, this is totally untested, just a thought...

Just tried the variable-length array option, proves to be working:

dmvo@cipher:/tmp$ cat c.c
f()
{
        char buf[4096];
}
dmvo@cipher:/tmp$ cc -c -Wframe-larger-than=1024 c.c
c.c: In function ‘f’:
c.c:4: warning: the frame size of 4112 bytes is larger than 1024 bytes
dmvo@cipher:/tmp$ cat d.c
f()
{
        int n = 4096;
        char buf1[n];
}
dmvo@cipher:/tmp$ cc -c -Wframe-larger-than=1024 d.c

Dmitri

[David Daney-4]

Dmitri Vorobiev wrote:

> On Fri, Nov 6, 2009 at 6:22 PM, Dmitri Vorobiev
> <dmitri.vorobiev@...> wrote:
>> On Fri, Nov 6, 2009 at 6:08 PM, Atsushi Nemoto <anemo@...> wrote:
>>> Recently COMMAND_LINE_SIZE (CL_SIZE) was extended to 4096 from 512.
>>> (commit 22242681 "MIPS: Extend COMMAND_LINE_SIZE")
>>>
>>> This cause warning if something like buf[CL_SIZE] was declared as a
>>> local variable, for example in prom_init_cmdline() on some platforms.
>>>
>>> And since many Makefiles in arch/mips enables -Werror, this cause
>>> build failure.
>>>
>>> How can we avoid this error?
>>>
>>> - do not use local array? (but dynamic allocation cannot be used in
>>>  such an early stage.  static array?)
>> Maybe a static array marked with __initdata?
>
> Also, I just thought that maybe it's possible to use a c99
> variable-length array here? Like this:
>
> int n = COMMAND_LINE_SIZE;
> char buf[n];
>
> This way, we don't put yet another variable in the .init.data section,
> unlike with the static array solution.
>
> However, this is totally untested, just a thought...
>

It depends on your concerns.  You are still using 4096 bytes of stack,
but you are trying to trick the compiler into not warning.

If you think the warning is bogus, you should remove it for all code,
not just this file.  If you think the warning is valid, then you should
fix the code so that it doesn't use as much stack space.

David Daney

[Dmitri Vorobiev]

On Fri, Nov 6, 2009 at 6:44 PM, David Daney <ddaney@...> wrote:

> Dmitri Vorobiev wrote:
>>
>> On Fri, Nov 6, 2009 at 6:22 PM, Dmitri Vorobiev
>> <dmitri.vorobiev@...> wrote:
>>>
>>> On Fri, Nov 6, 2009 at 6:08 PM, Atsushi Nemoto <anemo@...>
>>> wrote:
>>>>
>>>> Recently COMMAND_LINE_SIZE (CL_SIZE) was extended to 4096 from 512.
>>>> (commit 22242681 "MIPS: Extend COMMAND_LINE_SIZE")
>>>>
>>>> This cause warning if something like buf[CL_SIZE] was declared as a
>>>> local variable, for example in prom_init_cmdline() on some platforms.
>>>>
>>>> And since many Makefiles in arch/mips enables -Werror, this cause
>>>> build failure.
>>>>
>>>> How can we avoid this error?
>>>>
>>>> - do not use local array? (but dynamic allocation cannot be used in
>>>>  such an early stage.  static array?)
>>>
>>> Maybe a static array marked with __initdata?
>>
>> Also, I just thought that maybe it's possible to use a c99
>> variable-length array here? Like this:
>>
>> int n = COMMAND_LINE_SIZE;
>> char buf[n];
>>
>> This way, we don't put yet another variable in the .init.data section,
>> unlike with the static array solution.
>>
>> However, this is totally untested, just a thought...
>>
>
> It depends on your concerns.  You are still using 4096 bytes of stack, but
> you are trying to trick the compiler into not warning.
>
> If you think the warning is bogus, you should remove it for all code, not
> just this file.  If you think the warning is valid, then you should fix the
> code so that it doesn't use as much stack space.

Frankly, I don't think that the warning is bogus, especially if we're
talking about the case of the 4K page and 32-bit kernel, and I started
thinking htat probably the static array solution would be the safest
bet here.

Dmitri

[Maciej W. Rozycki]

On Fri, 6 Nov 2009, David Daney wrote:

> It depends on your concerns.  You are still using 4096 bytes of stack,
> but you are trying to trick the compiler into not warning.
>
> If you think the warning is bogus, you should remove it for all code,
> not just this file.  If you think the warning is valid, then you should
> fix the code so that it doesn't use as much stack space.

 My understanding is the option is for detecting unintended excessive use
of stack space.  If a place is known to require more space, it is
justified and harmless (such as in an early bootstrap call), then I see no
reason to forbid it or to imply it should be allowed globally then.

 Note that stack space of 4096 bytes required by a single call is
functionally equivalent to four nested calls requiring 1024 bytes each, so
from the practical point of view they are equivalent and during kernel
startup we may know that nesting is less extensive than, say, when a
syscall or an interrupt handler is being executed, where such stack
consumption would be of much more concern.

  Maciej

[Maciej W. Rozycki]

On Fri, 6 Nov 2009, Dmitri Vorobiev wrote:

> > It depends on your concerns.  You are still using 4096 bytes of stack, but
> > you are trying to trick the compiler into not warning.
> >
> > If you think the warning is bogus, you should remove it for all code, not
> > just this file.  If you think the warning is valid, then you should fix the
> > code so that it doesn't use as much stack space.
>
> Frankly, I don't think that the warning is bogus, especially if we're
> talking about the case of the 4K page and 32-bit kernel, and I started
> thinking htat probably the static array solution would be the safest
> bet here.

 KSEG space is not paged, so who cares about the page size?  You're not
making additional stack page allocations, although you can overflow the
space available at some point (but that's avoided if you know a priori
your backtrace is not going to be deep).  Static allocation has its
drawbacks, for example it takes storage space (if it's initialised data)
or memory space (if it's BSS) indefinitely.

  Maciej

[Dmitri Vorobiev]

On Fri, Nov 6, 2009 at 7:16 PM, Maciej W. Rozycki <macro@...> wrote:

> On Fri, 6 Nov 2009, Dmitri Vorobiev wrote:
>
>> > It depends on your concerns.  You are still using 4096 bytes of stack, but
>> > you are trying to trick the compiler into not warning.
>> >
>> > If you think the warning is bogus, you should remove it for all code, not
>> > just this file.  If you think the warning is valid, then you should fix the
>> > code so that it doesn't use as much stack space.
>>
>> Frankly, I don't think that the warning is bogus, especially if we're
>> talking about the case of the 4K page and 32-bit kernel, and I started
>> thinking htat probably the static array solution would be the safest
>> bet here.
>
>  KSEG space is not paged, so who cares about the page size?  You're not
> making additional stack page allocations, although you can overflow the
> space available at some point (but that's avoided if you know a priori
> your backtrace is not going to be deep).  Static allocation has its
> drawbacks, for example it takes storage space (if it's initialised data)
> or memory space (if it's BSS) indefinitely.

Thanks for the explanation. Then a variable-size array, I guess.

Dmitri

>
>  Maciej
>

[Maciej W. Rozycki]

On Fri, 6 Nov 2009, Dmitri Vorobiev wrote:

> >  KSEG space is not paged, so who cares about the page size?  You're not
> > making additional stack page allocations, although you can overflow the
> > space available at some point (but that's avoided if you know a priori
> > your backtrace is not going to be deep).  Static allocation has its
> > drawbacks, for example it takes storage space (if it's initialised data)
> > or memory space (if it's BSS) indefinitely.
>
> Thanks for the explanation. Then a variable-size array, I guess.

 Note that MIPS is at an advantage here and other architectures may have
to page the kernel space, so the observation is valid for our platform
code only -- for generic code (anything that goes outside arch/mips) you
may have to change the assumptions.

  Maciej

[Dmitri Vorobiev]

On Fri, Nov 6, 2009 at 7:30 PM, Maciej W. Rozycki <macro@...> wrote:

> On Fri, 6 Nov 2009, Dmitri Vorobiev wrote:
>
>> >  KSEG space is not paged, so who cares about the page size?  You're not
>> > making additional stack page allocations, although you can overflow the
>> > space available at some point (but that's avoided if you know a priori
>> > your backtrace is not going to be deep).  Static allocation has its
>> > drawbacks, for example it takes storage space (if it's initialised data)
>> > or memory space (if it's BSS) indefinitely.
>>
>> Thanks for the explanation. Then a variable-size array, I guess.
>
>  Note that MIPS is at an advantage here and other architectures may have
> to page the kernel space, so the observation is valid for our platform
> code only -- for generic code (anything that goes outside arch/mips) you
> may have to change the assumptions.

I believe that Atsushi-san was talking about the MIPS code only.
Indeed, he mentioned CL_SIZE, which used to be a MIPS-specific alias
to COMMAND_LINE_SIZE.

Dmitri

>
>  Maciej
>

[Atsushi Nemoto]

On Fri, 6 Nov 2009 20:17:06 +0200, Dmitri Vorobiev <dmitri.vorobiev@...> wrote:
> I believe that Atsushi-san was talking about the MIPS code only.
> Indeed, he mentioned CL_SIZE, which used to be a MIPS-specific alias
> to COMMAND_LINE_SIZE.

Yes, and I think static array is safest option now.

MIPS might be able to use large stack on early bootstrap stage, but
this assumption looks slightly fragile for me.

And excessive zero-initialized __initdata size will not be a problem
especially if vmlinux is compressed.

Thank you everyone, I will post a patch soon.

---
Atsushi Nemoto