Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
On Thu, Dec 13, 2018 at 05:31:28PM -0700, Jeff Law wrote: > On 12/7/18 4:35 PM, Segher Boessenkool wrote: > > 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 ;-) > What I'm questioning is whether or not this is at all useful. ie, if > I've written a something like task switching in C+asms, then I would > fully expect any data related to stack usage in that function to be > totally bogus. Telling me it's bogus in the assembly output really > isn't of value. It's telling me something I should clearly already know. > > And in the common case of an asm that doesn't mess with the stack at > all, the stack usage info is valid and warning me that it may not be is > just a huge annoyance. Yes, good points. I was thinking that if the warning only triggers for naked functions it wouldn't misfire so often, but that does not take away from that it is pretty useless in the first place. Segher
Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
On 12/7/18 4:35 PM, Segher Boessenkool wrote: > Hi! > > On Fri, Dec 07, 2018 at 07:51:35AM +, 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 ;-) What I'm questioning is whether or not this is at all useful. ie, if I've written a something like task switching in C+asms, then I would fully expect any data related to stack usage in that function to be totally bogus. Telling me it's bogus in the assembly output really isn't of value. It's telling me something I should clearly already know. And in the common case of an asm that doesn't mess with the stack at all, the stack usage info is valid and warning me that it may not be is just a huge annoyance. jeff
Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
Hi! On Fri, Dec 07, 2018 at 07:51:35AM +, 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
Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
Hi Torbjorn,
Just some formatting nitpicking:
On Tue, Nov 27, 2018 at 07:45:59PM +, 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
Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
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
Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
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
Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
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 +, 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
From e8af10ab24c7e095604b16206c0bd544e8699b93 Mon Sep 17 00:00:00 2001
From: Niklas DAHLQUIST
Date: Fri, 9 Nov 2018 18:48:34 +0100
Subject: [PATCH] Added information about inline assembler in stack
calculations
The stack usage calculation in GCC ignores any possible stack usage that
any inline assembly might contribute, and this is not reflected in the
.su file currently.
Since inline assembly will add to the uncertainty of the actual stack
usage for a function, this should be shown in the .su file as well.
This changeset will add "ignoring_inline_assembly" to the .su-file "type"
information of functions containing inline assembly.
The resulting stack usage type for functions containing inline assembly
will be according to the following table:
Static | Dynamic | Inline asm() | Resulting stack usage type
* | 0 | False| static
* | 0 | True | static,ignoring_inline_assembler
* | 0 < x | False| dynamic
* | 0 < x | True | dynamic,ignoring_inline_assembler
* | 0 < x < MAX | False| dynamic,bounded
* | 0 < x < MAX | True | dynamic,bounded,ignoring_inline_assembler
Added test case for ignore inline assembler in stack analysis files (.su)
Signed-off-by: Niklas DAHLQUIST
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.
The message is in keeping with the output of @option{-fstack-usage}.
diff --git a/gcc/function.c b/gcc/function.c
index 69523c1d723..197f80c0df3 100644
--- 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;
+ }
if (!check_asm_operands (PATTERN (insn)))
{
error_for_asm (insn, "impossible constraint in %");
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;
};
#define current_function_static_stack_size (cfun->su->static_stack_size)
#define current_function_dynamic_stack_size (cfun->su->dynamic_stack_size)
#define current_function_pushed_stack_size (cfun->su->pushed_stack_size)
+#define current_function_has_inline_assembler \
+ (cfun->su->has_inline_assembler)
#define current_function_has_unbounded_dynamic_stack_size \
(cfun->su->has_unbounded_dynamic_stack_size)
#define current_function_allocates_dynamic_stack_space\
diff --git a/gcc/testsuite/gcc.dg/stack-usage-3.c b/gcc/testsuite/gcc.dg/stack-usage-3.c
new file mode 100644
index 000..ccb935f358e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/stack-usage-3.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-fstack-usage" } */
+/* nvptx doesn't have a reg allocator, and hence no stack usage data. */
+/* { dg-skip-if "" { nvptx-*-* } { "*" } { "" } } */
+
+#include "nop.h"
+
+void foo()
+{
+int i;
+i = 1;
+}
+
+void bar()
+{
+int i;
+i = 1;
+asm(NOP);
+}
+
+/* { dg-final { scan-stack-usage "foo\t\[1-9\]\[0-9\]*\tstatic" } } */
+/* { dg-final { scan-stack-usage "bar\t\[1-9\]\[0-9\]*\tstatic,ignoring_inline_asm" } } */
+/* { dg-final { cleanup-stack-usage } } */
+
diff --git a/gcc/testsuite/gcc.dg/stack-usage-4.c
Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
Hi! On Mon, Nov 26, 2018 at 02:02:49PM +, 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
[PATCH] Added information about inline assembler in stack calculations (.su files)
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
From c7149f9be7c408c6355e085d0d2d6700b9938d08 Mon Sep 17 00:00:00 2001
From: Niklas DAHLQUIST
Date: Fri, 9 Nov 2018 18:48:34 +0100
Subject: [PATCH] Added information about inline assembler in stack
calculations
The stack usage calculation in GCC ignores any possible stack usage that
any inline assembly might contribute, and this is not reflected in the
.su file currently.
Since inline assembly will add to the uncertainty of the actual stack
usage for a function, this should be shown in the .su file as well.
This changeset will add "ignoring_inline_assembly" to the .su-file "type"
information of functions containing inline assembly.
The resulting stack usage type for functions containing inline assembly
will be according to the following table:
Static | Dynamic | Inline asm() | Resulting stack usage type
* | 0 | False| static
* | 0 | True | static,ignoring_inline_assembler
* | 0 < x | False| dynamic
* | 0 < x | True | dynamic,ignoring_inline_assembler
* | 0 < x < MAX | False| dynamic,bounded
* | 0 < x < MAX | True | dynamic,bounded,ignoring_inline_assembler
Added test case for ignore inline assembler in stack analysis files (.su)
Signed-off-by: Niklas DAHLQUIST
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.
The message is in keeping with the output of @option{-fstack-usage}.
diff --git a/gcc/function.c b/gcc/function.c
index 69523c1d723..197f80c0df3 100644
--- 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;
+ }
if (!check_asm_operands (PATTERN (insn)))
{
error_for_asm (insn, "impossible constraint in %");
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;
};
#define current_function_static_stack_size (cfun->su->static_stack_size)
#define current_function_dynamic_stack_size (cfun->su->dynamic_stack_size)
#define current_function_pushed_stack_size (cfun->su->pushed_stack_size)
+#define current_function_has_inline_assembler \
+ (cfun->su->has_inline_assembler)
#define current_function_has_unbounded_dynamic_stack_size \
(cfun->su->has_unbounded_dynamic_stack_size)
#define current_function_allocates_dynamic_stack_space\
diff --git a/gcc/testsuite/gcc.dg/stack-usage-3.c b/gcc/testsuite/gcc.dg/stack-usage-3.c
new file mode 100644
index 000..dfd92058a40
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/stack-usage-3.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-fstack-usage" } */
+/* nvptx doesn't have a reg allocator, and hence no stack usage data. */
+/* { dg-skip-if "" { nvptx-*-* } { "*" } { "" } } */
+
+void foo()
+{
+int i;
+i = 1;
+}
+
+void bar()
+{
+int i;
+i = 1;
+asm("nop");
+}
+
+/* { dg-final { scan-stack-usage "foo\t\[1-9\]\[0-9\]*\tstatic" } } */
+/* { dg-final { scan-stack-usage "bar\t\[1-9\]\[0-9\]*\tstatic,ignoring_inline_asm" } } */
+/* { dg-final { cleanup-stack-usage } } */
+
diff --git a/gcc/testsuite/gcc.dg/stack-usage-4.c b/gcc/testsuite/gcc.dg/stack-usage-4.c
new file mode 100644
index 000..6f0065006ec
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/stack-usage-4.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-fstack-usage" } */
+/* nvptx doesn't have a reg allocator, and hence no stack usage data. */
+/* { dg-skip-if "" { nvptx-*-* } { "*" } { ""
