[PATCH] Added information about inline assembler in stack calculations (.su files)

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Added information about inline assembler in stack calculations (.su files)

Torbjorn SVENSSON
Hi,

Attached is a small patch that, in case of inline assembler code,
indicates that the function stack usage is uncertain due to inline
assembler.

The test suite are using "nop" as an assembler instruction on all
targets, is this acceptable or is there a better way to test this?

Patch has been tested on gcc-arm-none-eabi-7-2018-q2-update for both
arm-none-eabi and x86_64-linux-gnu and SVN head (r266454) for
x86_64-linux-gnu.

Thanks,
Torbjörn, Samuel and Niklas







0001-Added-information-about-inline-assembler-in-stack-ca.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Added information about inline assembler in stack calculations (.su files)

Segher Boessenkool
Hi!

On Mon, Nov 26, 2018 at 02:02:49PM +0000, Torbjorn SVENSSON wrote:
> Attached is a small patch that, in case of inline assembler code,
> indicates that the function stack usage is uncertain due to inline
> assembler.
>
> The test suite are using "nop" as an assembler instruction on all
> targets, is this acceptable or is there a better way to test this?

Maybe see testsuite/gcc.dg/nop.h ?


Segher
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Added information about inline assembler in stack calculations (.su files)

Torbjorn SVENSSON
Hi!

Thanks for the feedback.
Attached is an updated patch where I switched to the NOP define instead.
I'm not sure if stack-usage-naked.c should be moved to gcc.dg-tree, or
if it should skip using the nop.h file (it feels wrong to do #include
"../../gcc.dg/nop.h" from within gcc.taget-tree).

Torbjörn

On 27/11/2018 19:40, Segher Boessenkool wrote:

> Hi!
>
> On Mon, Nov 26, 2018 at 02:02:49PM +0000, Torbjorn SVENSSON wrote:
>> Attached is a small patch that, in case of inline assembler code,
>> indicates that the function stack usage is uncertain due to inline
>> assembler.
>>
>> The test suite are using "nop" as an assembler instruction on all
>> targets, is this acceptable or is there a better way to test this?
> Maybe see testsuite/gcc.dg/nop.h ?
>
>
> Segher


0001-Added-information-about-inline-assembler-in-stack-ca.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Added information about inline assembler in stack calculations (.su files)

Jeff Law
In reply to this post by Torbjorn SVENSSON
On 11/26/18 7:02 AM, Torbjorn SVENSSON wrote:

> Hi,
>
> Attached is a small patch that, in case of inline assembler code,
> indicates that the function stack usage is uncertain due to inline
> assembler.
>
> The test suite are using "nop" as an assembler instruction on all
> targets, is this acceptable or is there a better way to test this?
>
> Patch has been tested on gcc-arm-none-eabi-7-2018-q2-update for both
> arm-none-eabi and x86_64-linux-gnu and SVN head (r266454) for
> x86_64-linux-gnu.
One could argue that allocating stack space inside an ASM is a really
bad idea.  Consider things like dwarf debugging and unwind tables.  If
you're allocating stack inside an ASM that stuff is going to be totally
wrong.

So I think my question before moving forward with something like this is
whether or not it makes sense at all to bother dumping data for a
scenario that we'd probably suggest developers avoid to begin with.

jeff
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Added information about inline assembler in stack calculations (.su files)

Niklas DAHLQUIST
On 12/1/18 1:15 AM, Jeff Law wrote:

> On 11/26/18 7:02 AM, Torbjorn SVENSSON wrote:
>> Hi,
>>
>> Attached is a small patch that, in case of inline assembler code,
>> indicates that the function stack usage is uncertain due to inline
>> assembler.
>>
>> The test suite are using "nop" as an assembler instruction on all
>> targets, is this acceptable or is there a better way to test this?
>>
>> Patch has been tested on gcc-arm-none-eabi-7-2018-q2-update for both
>> arm-none-eabi and x86_64-linux-gnu and SVN head (r266454) for
>> x86_64-linux-gnu.
> One could argue that allocating stack space inside an ASM is a really
> bad idea.  Consider things like dwarf debugging and unwind tables.  If
> you're allocating stack inside an ASM that stuff is going to be totally
> wrong.
>
> So I think my question before moving forward with something like this is
> whether or not it makes sense at all to bother dumping data for a
> scenario that we'd probably suggest developers avoid to begin with.
>
> jeff
Hi,

The purpose of the patch is to notify when the reported stack usage might be
incorrect. Even if it's bad practice to alter stack in asm, there are
use cases
in the embedded world that makes sense. A notable common use case is
FreeRTOS
task switch using ARM "naked" attribute and inline asm, which reports "0
static",
which gives a faulty stack usage. We have considered the other option to
report a warning for these cases, but that alternative hasn't appealed
to us.

Niklas
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Added information about inline assembler in stack calculations (.su files)

Segher Boessenkool
In reply to this post by Torbjorn SVENSSON
Hi Torbjorn,

Just some formatting nitpicking:

On Tue, Nov 27, 2018 at 07:45:59PM +0000, Torbjorn SVENSSON wrote:

> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 38e27a50a1e..e61ddc3260b 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -6456,7 +6456,7 @@ Warn if the stack usage of a function might exceed @var{byte-size}.
>  The computation done to determine the stack usage is conservative.
>  Any space allocated via @code{alloca}, variable-length arrays, or related
>  constructs is included by the compiler when determining whether or not to
> -issue a warning.
> +issue a warning. @code{asm} statements are ignored when computing stack usage.

Two spaces after a full stop.  Please try to start sentences with a capital
letter.

> --- a/gcc/function.c
> +++ b/gcc/function.c
> @@ -1822,6 +1822,10 @@ instantiate_virtual_regs_in_insn (rtx_insn *insn)
>  
>    if (asm_noperands (PATTERN (insn)) >= 0)
>      {
> +       if (flag_stack_usage_info)
> + {
> +   current_function_has_inline_assembler = 1;
> + }

Single statements should not normally get a { block } wrapped around it.

> diff --git a/gcc/function.h b/gcc/function.h
> index 7e59050e8a6..8c3ef49e866 100644
> --- a/gcc/function.h
> +++ b/gcc/function.h
> @@ -208,11 +208,16 @@ struct GTY(()) stack_usage
>    /* Nonzero if the amount of stack space allocated dynamically cannot
>       be bounded at compile-time.  */
>    unsigned int has_unbounded_dynamic_stack_size : 1;
> +
> +  /* NonZero if body contains asm statement (ignored in stack calculations) */
> +  unsigned int has_inline_assembler: 1;

"Nonzero", no camelcase.  I think it should be "an asm statement", or "some
asm statemement", or similar.  The line should end with a full stop and two
spaces, before the */.

> +  /* Add info regarding inline assembler (not part of stack calculations) */

Similar here.


Segher
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Added information about inline assembler in stack calculations (.su files)

Segher Boessenkool
In reply to this post by Niklas DAHLQUIST
Hi!

On Fri, Dec 07, 2018 at 07:51:35AM +0000, Niklas DAHLQUIST wrote:

> On 12/1/18 1:15 AM, Jeff Law wrote:
> > One could argue that allocating stack space inside an ASM is a really
> > bad idea.  Consider things like dwarf debugging and unwind tables.  If
> > you're allocating stack inside an ASM that stuff is going to be totally
> > wrong.
> >
> > So I think my question before moving forward with something like this is
> > whether or not it makes sense at all to bother dumping data for a
> > scenario that we'd probably suggest developers avoid to begin with.
>
> The purpose of the patch is to notify when the reported stack usage might be
> incorrect. Even if it's bad practice to alter stack in asm, there are
> use cases
> in the embedded world that makes sense. A notable common use case is
> FreeRTOS
> task switch using ARM "naked" attribute and inline asm, which reports "0
> static",
> which gives a faulty stack usage. We have considered the other option to
> report a warning for these cases, but that alternative hasn't appealed
> to us.

Would that work well?  Only warn for naked functions?  It would work
better for all users that do *not* mess with the stack in their asm ;-)


Segher