public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Added information about inline assembler in stack calculations (.su files)
@ 2018-11-26 14:03 Torbjorn SVENSSON
  2018-11-27 19:03 ` Segher Boessenkool
  2018-12-01  0:16 ` Jeff Law
  0 siblings, 2 replies; 9+ messages in thread
From: Torbjorn SVENSSON @ 2018-11-26 14:03 UTC (permalink / raw)
  To: gcc-patches
  Cc: Joey.Ye, Niklas DAHLQUIST, Samuel HULTGREN, Christophe LYON,
	Christophe MONAT

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

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







[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Added-information-about-inline-assembler-in-stack-ca.patch --]
[-- Type: text/x-patch; name="0001-Added-information-about-inline-assembler-in-stack-ca.patch", Size: 7964 bytes --]

From c7149f9be7c408c6355e085d0d2d6700b9938d08 Mon Sep 17 00:00:00 2001
From: Niklas DAHLQUIST <niklas.dahlquist@st.com>
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 <niklas.dahlquist@st.com>

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 %<asm%>");
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 00000000000..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 00000000000..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-*-* } { "*" } { "" } } */
+
+void foo(int size)
+{
+    int i;
+    int v[size];
+    i = 1;
+    v[0] = 1;
+}
+
+void bar(int size)
+{
+    int i;
+    int v[size];
+    i = 1;
+    v[0] = 1;
+    asm("nop");
+}
+
+/* { dg-final { scan-stack-usage "foo\t\[1-9\]\[0-9\]*\tdynamic" } } */
+/* { dg-final { scan-stack-usage "bar\t\[1-9\]\[0-9\]*\tdynamic,ignoring_inline_asm" } } */
+/* { dg-final { cleanup-stack-usage } } */
diff --git a/gcc/testsuite/gcc.target/arm/stack-usage-naked.c b/gcc/testsuite/gcc.target/arm/stack-usage-naked.c
new file mode 100644
index 00000000000..72e7124c5e3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/stack-usage-naked.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options -fstack-usage } */
+
+
+void __attribute__((naked)) foo()
+{
+}
+
+void __attribute__((naked)) bar()
+{
+    asm("nop");
+}
+
+/* { dg-final { scan-stack-usage "foo\t0\tstatic" } } */
+/* { dg-final { scan-stack-usage "bar\t0\tstatic,ignoring_inline_asm" } } */
+/* { dg-final { cleanup-stack-usage } } */
diff --git a/gcc/toplev.c b/gcc/toplev.c
index ab20cd98969..14c930be9f3 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -937,11 +937,16 @@ void
 output_stack_usage (void)
 {
   static bool warning_issued = false;
-  enum stack_usage_kind_type { STATIC = 0, DYNAMIC, DYNAMIC_BOUNDED };
+  enum stack_usage_kind_type { STATIC = 0, DYNAMIC, DYNAMIC_BOUNDED,
+    STATIC_IGNORING_INLINE_ASM,DYNAMIC_IGNORING_INLINE_ASM,
+    DYNAMIC_BOUNDED_IGNORING_INLINE_ASM };
   const char *stack_usage_kind_str[] = {
     "static",
     "dynamic",
-    "dynamic,bounded"
+    "dynamic,bounded",
+    "static,ignoring_inline_asm",
+    "dynamic,ignoring_inline_asm",
+    "dynamic,bounded,ignoring_inline_asm"
   };
   HOST_WIDE_INT stack_usage = current_function_static_stack_size;
   enum stack_usage_kind_type stack_usage_kind;
@@ -990,6 +995,26 @@ output_stack_usage (void)
       stack_usage += current_function_dynamic_stack_size;
     }
 
+  /* Add info regarding inline assembler (not part of stack calculations) */
+  if (current_function_has_inline_assembler)
+    {
+	switch (stack_usage_kind)
+	  {
+	case STATIC:
+	  stack_usage_kind = STATIC_IGNORING_INLINE_ASM;
+	  break;
+	case DYNAMIC:
+	  stack_usage_kind = DYNAMIC_IGNORING_INLINE_ASM;
+	  break;
+	case DYNAMIC_BOUNDED:
+	  stack_usage_kind = DYNAMIC_BOUNDED_IGNORING_INLINE_ASM;
+	  break;
+	default:
+	  stack_usage_kind = STATIC_IGNORING_INLINE_ASM;
+	  break;
+	}
+    }
+
   if (stack_usage_file)
     {
       expanded_location loc
@@ -1031,11 +1056,13 @@ output_stack_usage (void)
     {
       const location_t loc = DECL_SOURCE_LOCATION (current_function_decl);
 
-      if (stack_usage_kind == DYNAMIC)
+      if (stack_usage_kind == DYNAMIC
+	  || stack_usage_kind == DYNAMIC_IGNORING_INLINE_ASM)
 	warning_at (loc, OPT_Wstack_usage_, "stack usage might be unbounded");
       else if (stack_usage > warn_stack_usage)
 	{
-	  if (stack_usage_kind == DYNAMIC_BOUNDED)
+	  if (stack_usage_kind == DYNAMIC_BOUNDED
+	      || stack_usage_kind == DYNAMIC_BOUNDED_IGNORING_INLINE_ASM)
 	    warning_at (loc,
 			OPT_Wstack_usage_, "stack usage might be %wu bytes",
 			stack_usage);
-- 
2.18.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
  2018-11-26 14:03 [PATCH] Added information about inline assembler in stack calculations (.su files) Torbjorn SVENSSON
@ 2018-11-27 19:03 ` Segher Boessenkool
  2018-11-27 19:46   ` Torbjorn SVENSSON
  2018-12-01  0:16 ` Jeff Law
  1 sibling, 1 reply; 9+ messages in thread
From: Segher Boessenkool @ 2018-11-27 19:03 UTC (permalink / raw)
  To: Torbjorn SVENSSON
  Cc: gcc-patches, Joey.Ye, Niklas DAHLQUIST, Samuel HULTGREN,
	Christophe LYON, Christophe MONAT

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
  2018-11-27 19:03 ` Segher Boessenkool
@ 2018-11-27 19:46   ` Torbjorn SVENSSON
  2018-12-07 23:28     ` Segher Boessenkool
  0 siblings, 1 reply; 9+ messages in thread
From: Torbjorn SVENSSON @ 2018-11-27 19:46 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: gcc-patches, Joey.Ye, Niklas DAHLQUIST, Samuel HULTGREN,
	Christophe LYON, Christophe MONAT

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Added-information-about-inline-assembler-in-stack-ca.patch --]
[-- Type: text/x-patch; name="0001-Added-information-about-inline-assembler-in-stack-ca.patch", Size: 8004 bytes --]

From e8af10ab24c7e095604b16206c0bd544e8699b93 Mon Sep 17 00:00:00 2001
From: Niklas DAHLQUIST <niklas.dahlquist@st.com>
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 <niklas.dahlquist@st.com>

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 %<asm%>");
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 00000000000..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 b/gcc/testsuite/gcc.dg/stack-usage-4.c
new file mode 100644
index 00000000000..c599729a8da
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/stack-usage-4.c
@@ -0,0 +1,27 @@
+/* { 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 size)
+{
+    int i;
+    int v[size];
+    i = 1;
+    v[0] = 1;
+}
+
+void bar(int size)
+{
+    int i;
+    int v[size];
+    i = 1;
+    v[0] = 1;
+    asm(NOP);
+}
+
+/* { dg-final { scan-stack-usage "foo\t\[1-9\]\[0-9\]*\tdynamic" } } */
+/* { dg-final { scan-stack-usage "bar\t\[1-9\]\[0-9\]*\tdynamic,ignoring_inline_asm" } } */
+/* { dg-final { cleanup-stack-usage } } */
diff --git a/gcc/testsuite/gcc.target/arm/stack-usage-naked.c b/gcc/testsuite/gcc.target/arm/stack-usage-naked.c
new file mode 100644
index 00000000000..72e7124c5e3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/stack-usage-naked.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options -fstack-usage } */
+
+
+void __attribute__((naked)) foo()
+{
+}
+
+void __attribute__((naked)) bar()
+{
+    asm("nop");
+}
+
+/* { dg-final { scan-stack-usage "foo\t0\tstatic" } } */
+/* { dg-final { scan-stack-usage "bar\t0\tstatic,ignoring_inline_asm" } } */
+/* { dg-final { cleanup-stack-usage } } */
diff --git a/gcc/toplev.c b/gcc/toplev.c
index ab20cd98969..14c930be9f3 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -937,11 +937,16 @@ void
 output_stack_usage (void)
 {
   static bool warning_issued = false;
-  enum stack_usage_kind_type { STATIC = 0, DYNAMIC, DYNAMIC_BOUNDED };
+  enum stack_usage_kind_type { STATIC = 0, DYNAMIC, DYNAMIC_BOUNDED,
+    STATIC_IGNORING_INLINE_ASM,DYNAMIC_IGNORING_INLINE_ASM,
+    DYNAMIC_BOUNDED_IGNORING_INLINE_ASM };
   const char *stack_usage_kind_str[] = {
     "static",
     "dynamic",
-    "dynamic,bounded"
+    "dynamic,bounded",
+    "static,ignoring_inline_asm",
+    "dynamic,ignoring_inline_asm",
+    "dynamic,bounded,ignoring_inline_asm"
   };
   HOST_WIDE_INT stack_usage = current_function_static_stack_size;
   enum stack_usage_kind_type stack_usage_kind;
@@ -990,6 +995,26 @@ output_stack_usage (void)
       stack_usage += current_function_dynamic_stack_size;
     }
 
+  /* Add info regarding inline assembler (not part of stack calculations) */
+  if (current_function_has_inline_assembler)
+    {
+	switch (stack_usage_kind)
+	  {
+	case STATIC:
+	  stack_usage_kind = STATIC_IGNORING_INLINE_ASM;
+	  break;
+	case DYNAMIC:
+	  stack_usage_kind = DYNAMIC_IGNORING_INLINE_ASM;
+	  break;
+	case DYNAMIC_BOUNDED:
+	  stack_usage_kind = DYNAMIC_BOUNDED_IGNORING_INLINE_ASM;
+	  break;
+	default:
+	  stack_usage_kind = STATIC_IGNORING_INLINE_ASM;
+	  break;
+	}
+    }
+
   if (stack_usage_file)
     {
       expanded_location loc
@@ -1031,11 +1056,13 @@ output_stack_usage (void)
     {
       const location_t loc = DECL_SOURCE_LOCATION (current_function_decl);
 
-      if (stack_usage_kind == DYNAMIC)
+      if (stack_usage_kind == DYNAMIC
+	  || stack_usage_kind == DYNAMIC_IGNORING_INLINE_ASM)
 	warning_at (loc, OPT_Wstack_usage_, "stack usage might be unbounded");
       else if (stack_usage > warn_stack_usage)
 	{
-	  if (stack_usage_kind == DYNAMIC_BOUNDED)
+	  if (stack_usage_kind == DYNAMIC_BOUNDED
+	      || stack_usage_kind == DYNAMIC_BOUNDED_IGNORING_INLINE_ASM)
 	    warning_at (loc,
 			OPT_Wstack_usage_, "stack usage might be %wu bytes",
 			stack_usage);
-- 
2.18.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
  2018-11-26 14:03 [PATCH] Added information about inline assembler in stack calculations (.su files) Torbjorn SVENSSON
  2018-11-27 19:03 ` Segher Boessenkool
@ 2018-12-01  0:16 ` Jeff Law
  2018-12-07  7:51   ` Niklas DAHLQUIST
  1 sibling, 1 reply; 9+ messages in thread
From: Jeff Law @ 2018-12-01  0:16 UTC (permalink / raw)
  To: Torbjorn SVENSSON, gcc-patches
  Cc: Joey.Ye, Niklas DAHLQUIST, Samuel HULTGREN, Christophe LYON,
	Christophe MONAT

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
  2018-12-01  0:16 ` Jeff Law
@ 2018-12-07  7:51   ` Niklas DAHLQUIST
  2018-12-08 12:05     ` Segher Boessenkool
  0 siblings, 1 reply; 9+ messages in thread
From: Niklas DAHLQUIST @ 2018-12-07  7:51 UTC (permalink / raw)
  To: Jeff Law, Torbjorn SVENSSON, gcc-patches
  Cc: Joey.Ye, Samuel HULTGREN, Christophe LYON, Christophe MONAT

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
  2018-11-27 19:46   ` Torbjorn SVENSSON
@ 2018-12-07 23:28     ` Segher Boessenkool
  0 siblings, 0 replies; 9+ messages in thread
From: Segher Boessenkool @ 2018-12-07 23:28 UTC (permalink / raw)
  To: Torbjorn SVENSSON
  Cc: gcc-patches, Joey.Ye, Niklas DAHLQUIST, Samuel HULTGREN,
	Christophe LYON, Christophe MONAT

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
  2018-12-07  7:51   ` Niklas DAHLQUIST
@ 2018-12-08 12:05     ` Segher Boessenkool
  2018-12-14  0:31       ` Jeff Law
  0 siblings, 1 reply; 9+ messages in thread
From: Segher Boessenkool @ 2018-12-08 12:05 UTC (permalink / raw)
  To: Niklas DAHLQUIST
  Cc: Jeff Law, Torbjorn SVENSSON, gcc-patches, Joey.Ye,
	Samuel HULTGREN, Christophe LYON, Christophe MONAT

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
  2018-12-08 12:05     ` Segher Boessenkool
@ 2018-12-14  0:31       ` Jeff Law
  2018-12-14  8:58         ` Segher Boessenkool
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff Law @ 2018-12-14  0:31 UTC (permalink / raw)
  To: Segher Boessenkool, Niklas DAHLQUIST
  Cc: Torbjorn SVENSSON, gcc-patches, Joey.Ye, Samuel HULTGREN,
	Christophe LYON, Christophe MONAT

On 12/7/18 4:35 PM, Segher Boessenkool wrote:
> 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 ;-)
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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Added information about inline assembler in stack calculations (.su files)
  2018-12-14  0:31       ` Jeff Law
@ 2018-12-14  8:58         ` Segher Boessenkool
  0 siblings, 0 replies; 9+ messages in thread
From: Segher Boessenkool @ 2018-12-14  8:58 UTC (permalink / raw)
  To: Jeff Law
  Cc: Niklas DAHLQUIST, Torbjorn SVENSSON, gcc-patches, Joey.Ye,
	Samuel HULTGREN, Christophe LYON, Christophe MONAT

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-12-14  8:58 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-26 14:03 [PATCH] Added information about inline assembler in stack calculations (.su files) Torbjorn SVENSSON
2018-11-27 19:03 ` Segher Boessenkool
2018-11-27 19:46   ` Torbjorn SVENSSON
2018-12-07 23:28     ` Segher Boessenkool
2018-12-01  0:16 ` Jeff Law
2018-12-07  7:51   ` Niklas DAHLQUIST
2018-12-08 12:05     ` Segher Boessenkool
2018-12-14  0:31       ` Jeff Law
2018-12-14  8:58         ` Segher Boessenkool

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).