Patchwork arm,rx: don't ICE on naked functions with local vars

login
register
mail settings
Submitter DJ Delorie
Date July 26, 2011, 7:52 p.m.
Message ID <201107261952.p6QJqA8s007635@greed.delorie.com>
Download mbox | patch
Permalink /patch/106931/
State New
Headers show

Comments

DJ Delorie - July 26, 2011, 7:52 p.m.
This patch tests for at least one user-caused reason for this
assertion failing - requiring a local frame in a naked function.  For
this case at least, it would be better to trigger an error than to
ICE.  OK?

static int bar;
void __attribute__((naked)) function(void) {
   int foo, result;
   result = subFunction(&foo, &bar);   // ICE here
}

	* expr.c (expand_expr_addr_expr_1): Detect a user request for
	a local frame in a naked function, and produce a suitable
	error for that specific case.
Richard Henderson - July 28, 2011, 7:26 p.m.
On 07/26/2011 12:52 PM, DJ Delorie wrote:
> This patch tests for at least one user-caused reason for this
> assertion failing - requiring a local frame in a naked function.  For
> this case at least, it would be better to trigger an error than to
> ICE.  OK?
> 
> static int bar;
> void __attribute__((naked)) function(void) {
>    int foo, result;
>    result = subFunction(&foo, &bar);   // ICE here
> }
> 
> 	* expr.c (expand_expr_addr_expr_1): Detect a user request for
> 	a local frame in a naked function, and produce a suitable
> 	error for that specific case.

Err... why would naked affect anything at all this early in the
compilation path?

Without an answer to that, this seems like an odd place to patch.



r~
DJ Delorie - July 28, 2011, 7:38 p.m.
Naked is related to TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS - ARM and RX
set the latter based on the former, and nobody else uses that target
hook.  So, naked functions don't have stack slots for args.  Without
stack slots, args can't be assigned to memory locations - even if
they're TREE_ADDRESSABLE.
Richard Henderson - July 28, 2011, 7:57 p.m.
On 07/28/2011 12:38 PM, DJ Delorie wrote:
> Naked is related to TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS - ARM and RX
> set the latter based on the former, and nobody else uses that target
> hook.  So, naked functions don't have stack slots for args.  Without
> stack slots, args can't be assigned to memory locations - even if
> they're TREE_ADDRESSABLE.

But your test case has a local variable, not an argument at all.
So I don't immediately see the connection.


r~
DJ Delorie - July 28, 2011, 8:30 p.m.
Fails the same way with a parameter as with a local:

int result;
void __attribute__((naked)) ISRFunction(int x) {
   result = subFunction(&x);
}

but I'm certainly open to a better explanation of how a user program
can trigger an ICE that way.

Hmmm... the only use of targetm.calls.allocate_stack_slots_for_args()
is in use_register_for_decl() - which is used by expand_decl() for
automatic variables.

Maybe the hook should have been called allocate_stack_slots_for_decls() ?
Richard Henderson - July 28, 2011, 9:41 p.m.
On 07/28/2011 01:30 PM, DJ Delorie wrote:
> Fails the same way with a parameter as with a local:
> 
> int result;
> void __attribute__((naked)) ISRFunction(int x) {
>    result = subFunction(&x);
> }
> 
> but I'm certainly open to a better explanation of how a user program
> can trigger an ICE that way.
> 
> Hmmm... the only use of targetm.calls.allocate_stack_slots_for_args()
> is in use_register_for_decl() - which is used by expand_decl() for
> automatic variables.
> 
> Maybe the hook should have been called allocate_stack_slots_for_decls() ?

Probably.  Thanks for the leg-work.  I'll approve the patch as-is.


r~
DJ Delorie - July 28, 2011, 10:26 p.m.
Thanks!  Committed.
DJ Delorie - July 29, 2011, 3:31 p.m.
> Probably.  Thanks for the leg-work.  I'll approve the patch as-is.

May I apply it to the 4.5 and 4.6 branches?  The same patch applies
as-is to both.

Patch

Index: expr.c
===================================================================
--- expr.c	(revision 176766)
+++ expr.c	(working copy)
@@ -6943,13 +6943,22 @@  expand_expr_addr_expr_1 (tree exp, rtx t
 				modifier == EXPAND_INITIALIZER
 				? EXPAND_INITIALIZER : EXPAND_CONST_ADDRESS);
 
 	  /* If the DECL isn't in memory, then the DECL wasn't properly
 	     marked TREE_ADDRESSABLE, which will be either a front-end
 	     or a tree optimizer bug.  */
-	  gcc_assert (MEM_P (result));
+
+	  if (TREE_ADDRESSABLE (exp)
+	      && ! MEM_P (result)
+	      && ! targetm.calls.allocate_stack_slots_for_args())
+	    {
+	      error ("local frame unavailable (naked function?)");
+	      return result;
+	    }
+	  else
+	    gcc_assert (MEM_P (result));
 	  result = XEXP (result, 0);
 
 	  /* ??? Is this needed anymore?  */
 	  if (DECL_P (exp) && !TREE_USED (exp) == 0)
 	    {
 	      assemble_external (exp);