Bug 34982 - [4.3 regression] calling a function with undefined parameters causes segmentation fault at -O1 or higher
Summary: [4.3 regression] calling a function with undefined parameters causes segmenta...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.3.0
: P1 normal
Target Milestone: 4.3.0
Assignee: Not yet assigned to anyone
URL:
Keywords: patch, wrong-code
Depends on:
Blocks:
 
Reported: 2008-01-26 17:27 UTC by bero
Modified: 2008-01-31 08:21 UTC (History)
9 users (show)

See Also:
Host: i586-pc-linux-gnu
Target: i586-pc-linux-gnu
Build: i586-pc-linux-gnu
Known to work:
Known to fail:
Last reconfirmed: 2008-01-26 18:50:39


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description bero 2008-01-26 17:27:30 UTC
The following (admittedly ugly) piece of code works on older gccs, and segfaults on 4.3:


#include <assert.h>
#include <string.h>

static void something();

int main(int argc, char **argv) {
        something("test");
}

static void something(const char *whatever) {
        assert(!strcmp(whatever, "test"));
}



Changing the initial declaration of something() to static void something(char *whatever) makes the problem go away.
Comment 1 Richard Biener 2008-01-26 18:50:38 UTC
We seem to use local calling conventions for emitting the body of something, but
at the call site we pass arguments via the stack.

P1 until we know more about this.

Reduced testcase:

extern void abort (void);

static void something();

int main()
{
  something(-1);
}

static void something(int i)
{
  if (i != -1)
    abort ();
}

The asm shows it:

something:
        subl    $12, %esp
        cmpl    $-1, %eax
        je      .L3
        call    abort
.L3:
        addl    $12, %esp
        ret

but:

main:
        leal    4(%esp), %ecx
        andl    $-16, %esp
        pushl   -4(%ecx)
        pushl   %ecx
        subl    $8, %esp
        movl    $-1, (%esp)
        call    something
Comment 2 Richard Biener 2008-01-26 19:07:01 UTC
The first time we ask, cgraph_local_info ()->local is zero, the second time it
is one.

Honza, Uros?
Comment 3 Richard Biener 2008-01-26 19:09:56 UTC
Janis, can you hunt this?  Thanks.
Comment 4 Richard Biener 2008-01-26 19:23:47 UTC
So we use the local info before it is available and thus the following will ICE:

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 131861)
+++ config/i386/i386.c	(working copy)
@@ -3199,6 +3199,7 @@ ix86_function_regparm (const_tree type, 
     {
       /* FIXME: remove this CONST_CAST when cgraph.[ch] is constified.  */
       struct cgraph_local_info *i = cgraph_local_info (CONST_CAST_TREE(decl));
+      gcc_assert (cgraph_node (CONST_CAST_TREE(decl))->analyzed);
       if (i && i->local)
 	{
 	  int local_regparm, globals = 0, regno;


and if it is just not available (i == NULL) might give inconsistent
answers.
Comment 5 Jan Hubicka 2008-01-26 20:19:19 UTC
Subject: Re:  [4.3 regression] calling a function with undefined parameters causes segmentation fault at -O1 or higher

> and if it is just not available (i == NULL) might give inconsistent
> answers.

I will look into this.  cgraph_local_info used to trap when asked for
unavailable local info, looks like someone fixed the bug by removing the
assert.

Honza
Comment 6 Jan Hubicka 2008-01-27 13:54:54 UTC
Subject: Re:  [4.3 regression] calling a function with undefined parameters causes segmentation fault at -O1 or higher

cgraph_local_info still behaves as expected returning NULL when info is
not computed yet. Unfortunately check to simply ignore it when not
available has been added to ix86_function_regparm that makes this bug
lead to wrong code. (revision 123146)

There are two occurences where we can ix86_function_regparm. First one
is for compatibility checking, I would just declare it invalid - we
don't want the type comatiblity to depend on backend decision and I
think it is perfectly sane to reject any types specifying different
REGPARM values or where one specify and other doesn't.

I am testing attached patch and will commit it if passes.

Other case is from gimplifier, I am looking into it.  This definitly has
to go or we need to drop the feature :(

Honza

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 131882)
+++ config/i386/i386.c	(working copy)
@@ -3148,6 +3148,7 @@ ix86_comp_type_attributes (const_tree ty
 {
   /* Check for mismatch of non-default calling convention.  */
   const char *const rtdstr = TARGET_RTD ? "cdecl" : "stdcall";
+  tree attr1, attr2;
 
   if (TREE_CODE (type1) != FUNCTION_TYPE
       && TREE_CODE (type1) != METHOD_TYPE)
@@ -3155,11 +3156,27 @@ ix86_comp_type_attributes (const_tree ty
 
   /* Check for mismatched fastcall/regparm types.  */
   if ((!lookup_attribute ("fastcall", TYPE_ATTRIBUTES (type1))
-       != !lookup_attribute ("fastcall", TYPE_ATTRIBUTES (type2)))
-      || (ix86_function_regparm (type1, NULL)
-	  != ix86_function_regparm (type2, NULL)))
+       != !lookup_attribute ("fastcall", TYPE_ATTRIBUTES (type2))))
     return 0;
 
+  /* We don't want to use ix86_function_regparm here: it's decision depends
+     on middle end information, like localness of functions.  Here we only want
+     to know if types are declared compatible.  */
+  attr1 = lookup_attribute ("regparm", TYPE_ATTRIBUTES (type1));
+  attr2 = lookup_attribute ("regparm", TYPE_ATTRIBUTES (type2));
+
+  if ((attr1 != NULL_TREE) != (attr2 != NULL_TREE))
+    return 0;
+
+  if (attr1)
+    {
+      int val1 = TREE_INT_CST_LOW (TREE_VALUE (TREE_VALUE (attr1)));
+      int val2 = TREE_INT_CST_LOW (TREE_VALUE (TREE_VALUE (attr2)));
+
+      if (val1 != val2)
+	return 0;
+    }
+
   /* Check for mismatched sseregparm types.  */
   if (!lookup_attribute ("sseregparm", TYPE_ATTRIBUTES (type1))
       != !lookup_attribute ("sseregparm", TYPE_ATTRIBUTES (type2)))
Comment 7 Richard Biener 2008-01-27 14:19:22 UTC
One more reason to gimplify unit-at-a-time...
Comment 8 Jan Hubicka 2008-01-27 18:10:00 UTC
Subject: Re:  [4.3 regression] calling a function with undefined parameters causes segmentation fault at -O1 or higher

> One more reason to gimplify unit-at-a-time...

Yep, on the other hand there is probably not much need to get that
amount of architectural detail so easy.  I am looking into what makes
the compilation to diverge.

Honza
Comment 9 Jan Hubicka 2008-01-27 19:24:29 UTC
Subject: Re:  [4.3 regression] calling a function with undefined parameters causes segmentation fault at -O1 or higher

However the failure here is not early calling of cgraph_local_info (it
is ugly, but harmless, we are just looking for target promoting rules
that we don't change). 

The problem is good old type system broken scenario: the forward
declaration has no prorotype and thus might be vararg and thus it is not
regparmized, however the definition is correct. When expanding the call
we use type of the call, so the wrong type.

I am testing the attached patch. My type merging code fixes this too and
obvioiusly we should work harder on maybe_vaarg rule for local
functions, this should make lot of difference on K&R code (I wonder if
any is still around in usual distro)

Honza

Index: config/i386/i386.c
===================================================================
*** config/i386/i386.c	(revision 131882)
--- config/i386/i386.c	(working copy)
*************** init_cumulative_args (CUMULATIVE_ARGS *c
*** 3432,3437 ****
--- 3449,3455 ----
  		      rtx libname,	/* SYMBOL_REF of library name or 0 */
  		      tree fndecl)
  {
+   struct cgraph_local_info *i = fndecl ? cgraph_local_info (fndecl) : NULL;
    memset (cum, 0, sizeof (*cum));
  
    /* Set up the number of registers to use for passing arguments.  */
*************** init_cumulative_args (CUMULATIVE_ARGS *c
*** 3442,3447 ****
--- 3460,3474 ----
      cum->mmx_nregs = MMX_REGPARM_MAX;
    cum->warn_sse = true;
    cum->warn_mmx = true;
+ 
+   /* Because type might mismatch in between caller and callee, we need to
+      use actual type of function for local calls.
+      FIXME: cgraph_analyze can be told to actually record if function uses
+      va_start so for local functions maybe_vaarg can be made aggressive
+      helping K&R code.
+      FIXME: once typesytem is fixed, we won't need this code anymore.  */
+   if (i && i->local)
+     fntype = TREE_TYPE (fndecl);
    cum->maybe_vaarg = (fntype
  		      ? (!prototype_p (fntype) || stdarg_p (fntype))
  		      : !libname);
Comment 10 bero 2008-01-27 19:36:44 UTC
> this should make lot of difference on K&R code (I wonder if
> any is still around in usual distro)

Some parts of xorg still follow K&R conventions, few parts of teTeX have K&R code in them, cdrtools is fully K&R (I "fixed" that in the dvdrtools fork, not sure if any of the other cdrtools forks in circulation copied that) -- other than that, I'm not aware of any commonly used K&R bits and pieces in a modern system.
Comment 11 Jan Hubicka 2008-01-29 17:51:36 UTC
Subject: Re:  [4.3 regression] calling a function with undefined parameters causes segmentation fault at -O1 or higher

Hi,
the patch seems to pass my local testing, but on Zdenek's tester I get
curious results on i686:

Tests that now fail, but worked before:                                                                                                                     
                                                                                                                                                            
libmudflap.cth/pass37-frag.c (-O2) (rerun 14) execution test                                                                                                
libmudflap.cth/pass37-frag.c (-O2) (rerun 18) execution test                                                                                                
libmudflap.cth/pass37-frag.c (-O2) (rerun 18) output pattern test                                                                                           
libmudflap.cth/pass37-frag.c (-O3) (rerun 2) execution test                                                                                                 
libmudflap.cth/pass37-frag.c (-O3) (rerun 2) output pattern test                                                                                            
libmudflap.cth/pass37-frag.c (-O3) (rerun 3) execution test                                                                                                 
libmudflap.cth/pass37-frag.c (-O3) (rerun 3) output pattern test                                                                                            
libmudflap.cth/pass37-frag.c (-static -DSTATIC) (rerun 10) execution test                                                                                   
libmudflap.cth/pass37-frag.c (-static -DSTATIC) (rerun 16) execution test                                                                                   
libmudflap.cth/pass37-frag.c (-static -DSTATIC) (rerun 16) output pattern test                                                                              
libmudflap.cth/pass37-frag.c (rerun 10) execution test                                                                                                      
libmudflap.cth/pass37-frag.c (rerun 10) output pattern test                                                                                                 
libmudflap.cth/pass37-frag.c (rerun 12) execution test                                                                                                      libmudflap.cth/pass37-frag.c (rerun 12) output pattern test                                                                                                 
libmudflap.cth/pass37-frag.c (rerun 13) execution test                                                                                                      
libmudflap.cth/pass37-frag.c (rerun 14) execution test                                                                                                      
libmudflap.cth/pass37-frag.c (rerun 14) output pattern test                                                                                                 
libmudflap.cth/pass37-frag.c (rerun 15) execution test                                                                                                      
libmudflap.cth/pass37-frag.c (rerun 17) execution test                                                                                                      
libmudflap.cth/pass37-frag.c (rerun 17) output pattern test                                                                                                 
libmudflap.cth/pass37-frag.c (rerun 2) execution test                                                                                                       
libmudflap.cth/pass37-frag.c (rerun 2) output pattern test                                                                                                  
libmudflap.cth/pass37-frag.c (rerun 4) execution test                                                                                                       
libmudflap.cth/pass37-frag.c (rerun 4) output pattern test                                                                                                  
libmudflap.cth/pass39-frag.c (-O2) (rerun 11) execution test                                                                                                
libmudflap.cth/pass39-frag.c (-O2) (rerun 4) execution test                                                                                                 
libmudflap.cth/pass39-frag.c (-O3) (rerun 13) execution test                                                                                                
libmudflap.cth/pass39-frag.c (-O3) (rerun 13) output pattern test                                                                                           
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 10) execution test                                                                                   
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 10) output pattern test                                                                              
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 14) execution test                                                                                   
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 14) output pattern test                                                                              
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 16) execution test                                                                                   
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 16) output pattern test                                                                              
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 4) execution test                                                                                    
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 4) output pattern test                                                                               
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 5) execution test                                                                                    
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 5) output pattern test                                                                               
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 7) execution test                                                                                    
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 7) output pattern test                                                                               
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 9) execution test                                                                                    
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 9) output pattern test                                                                               
libmudflap.cth/pass39-frag.c (rerun 1) execution test                                                                                                       
libmudflap.cth/pass39-frag.c (rerun 1) output pattern test                                                                                                  
libmudflap.cth/pass39-frag.c (rerun 15) execution test                                                                                                      
libmudflap.cth/pass39-frag.c (rerun 18) execution test                                                                                                      
libmudflap.cth/pass39-frag.c (rerun 18) output pattern test                                                                                                 
libmudflap.cth/pass39-frag.c (rerun 19) execution test                                                                                                      
libmudflap.cth/pass39-frag.c (rerun 9) execution test    
libmudflap.cth/pass39-frag.c (rerun 9) output pattern test                                                                                                  
libmudflap.cth/pass39-frag.c execution test                                                                                                                 
libmudflap.cth/pass39-frag.c output pattern test                                                                                                            
libmudflap.cth/pass40-frag.c (-O2) execution test                                                                                                           
libmudflap.cth/pass40-frag.c (-O2) output pattern test                                                                                                      
libmudflap.cth/pass40-frag.c (-static -DSTATIC) execution test                                                                                              
libmudflap.cth/pass40-frag.c (-static -DSTATIC) output pattern test                                                                                         
libmudflap.cth/pass40-frag.c execution test                                                                                                                 
libmudflap.cth/pass40-frag.c output pattern test                                                                                                            
                                                                                                                                                            
Tests that now work, but didn't before:                                                                                                                     
                                                                                                                                                            
libmudflap.cth/pass37-frag.c (-O2) (rerun 11) execution test                                                                                                
libmudflap.cth/pass37-frag.c (-O2) (rerun 11) output pattern test                                                                                           
libmudflap.cth/pass37-frag.c (-O2) (rerun 17) execution test                                                                                                
libmudflap.cth/pass37-frag.c (-O2) (rerun 19) execution test                                                                                                libmudflap.cth/pass37-frag.c (-O2) (rerun 19) output pattern test                                                                                           
libmudflap.cth/pass37-frag.c (-O2) (rerun 4) execution test                                                                                                 
libmudflap.cth/pass37-frag.c (-O2) (rerun 4) output pattern test                                                                                            
libmudflap.cth/pass37-frag.c (-O2) (rerun 7) execution test                                                                                                 
libmudflap.cth/pass37-frag.c (-O2) (rerun 7) output pattern test                                                                                            
libmudflap.cth/pass37-frag.c (-O2) (rerun 9) execution test                                                                                                 
libmudflap.cth/pass37-frag.c (-O2) (rerun 9) output pattern test                                                                                            
libmudflap.cth/pass37-frag.c (-O3) (rerun 17) execution test                                                                                                
libmudflap.cth/pass37-frag.c (-O3) (rerun 18) execution test                                                                                                
libmudflap.cth/pass37-frag.c (-O3) (rerun 18) output pattern test                                                                                           
libmudflap.cth/pass37-frag.c (-static -DSTATIC) (rerun 1) output pattern test                                                                               
libmudflap.cth/pass37-frag.c (-static -DSTATIC) (rerun 5) execution test                                                                                    
libmudflap.cth/pass37-frag.c (-static -DSTATIC) (rerun 5) output pattern test                                                                               
libmudflap.cth/pass37-frag.c (-static -DSTATIC) execution test                                                                                              
libmudflap.cth/pass37-frag.c (rerun 18) execution test                                                                                                      
libmudflap.cth/pass37-frag.c (rerun 18) output pattern test                                                                                                 
libmudflap.cth/pass37-frag.c (rerun 19) execution test                                                                                                      
libmudflap.cth/pass37-frag.c (rerun 3) execution test                                                                                                       
libmudflap.cth/pass37-frag.c (rerun 3) output pattern test                                                                                                  
libmudflap.cth/pass39-frag.c (-O2) (rerun 1) execution test                                                                                                 
libmudflap.cth/pass39-frag.c (-O2) (rerun 1) output pattern test                                                                                            
libmudflap.cth/pass39-frag.c (-O3) (rerun 5) execution test                                                                                                 
libmudflap.cth/pass39-frag.c (-O3) (rerun 5) output pattern test                                                                                            
libmudflap.cth/pass39-frag.c (-O3) (rerun 9) execution test                                                                                                 
libmudflap.cth/pass39-frag.c (-O3) execution test                                                                                                           
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 17) execution test                                                                                   
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 17) output pattern test                                                                              
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 19) execution test                                                                                   
libmudflap.cth/pass39-frag.c (-static -DSTATIC) (rerun 19) output pattern test                                                                              
libmudflap.cth/pass39-frag.c (-static -DSTATIC) execution test                                                                                              
libmudflap.cth/pass39-frag.c (rerun 11) execution test                                                                                                      
libmudflap.cth/pass39-frag.c (rerun 11) output pattern test                                                                                                 
libmudflap.cth/pass39-frag.c (rerun 12) execution test                                                                                                      
libmudflap.cth/pass39-frag.c (rerun 12) output pattern test                                                                                                 
libmudflap.cth/pass39-frag.c (rerun 13) execution test                                                                                                      
libmudflap.cth/pass39-frag.c (rerun 3) execution test         
libmudflap.cth/pass39-frag.c (rerun 3) execution test                                                                                                       
libmudflap.cth/pass39-frag.c (rerun 5) execution test                                                                                                       
libmudflap.cth/pass39-frag.c (rerun 5) output pattern test 

Those are all timeouts that does not reproduce for me on local testing
(and the tests don't even seem that slow).  I am inclined to think that
this is just random noise but will try to check before comitting.

Honza
Comment 12 Jakub Jelinek 2008-01-29 21:22:47 UTC
These tests time out from time to time when the testing box is busy, that's quite
normal.  The problem is in the use of sched_yield (), which puts the calling thread to the end of the runqueue.  If there are many processes in the runqueue,
one or more of the 10 threads might miss the 10 sec timeout in one or more of the 20 repetitions in 100 sched_yield calls.
So just ignore this.
Comment 13 Tom Tromey 2008-01-30 01:19:26 UTC
> other
> than that, I'm not aware of any commonly used K&R bits and pieces in a modern
> system.

FWIW -- Emacs is mostly K&R.
Comment 14 Steven Bosscher 2008-01-30 12:35:13 UTC
Patch in comment #9 works for me.
Comment 15 Jan Hubicka 2008-01-30 15:54:51 UTC
Fixed at mainline. I am really surprises this is 4.3 only regression since the code didn't see much changes in last few releases.
Comment 16 Jan Hubicka 2008-01-30 15:55:06 UTC
Subject: Bug 34982

Author: hubicka
Date: Wed Jan 30 15:54:14 2008
New Revision: 131966

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131966
Log:

	PR target/34982
	* i386.c (init_cumulative_args): Use real function declaration when
	calling locally.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr34982.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/testsuite/ChangeLog

Comment 17 Jan Hubicka 2008-01-30 15:56:50 UTC
Subject: Re:  [4.3 regression] calling a function with undefined parameters causes segmentation fault at -O1 or higher

> These tests time out from time to time when the testing box is busy, that's
> quite
> normal.  The problem is in the use of sched_yield (), which puts the calling
> thread to the end of the runqueue.  If there are many processes in the
> runqueue,
> one or more of the 10 threads might miss the 10 sec timeout in one or more of
> the 20 repetitions in 100 sched_yield calls.
> So just ignore this.

Thanks for explanation.  It happent few time in past to me that I
ignored mudflap failures incorrectly claiming random noise. Now at least
I know how to look for test that is supposed to have this problem.

Honza
Comment 18 Dominique d'Humieres 2008-01-30 22:59:36 UTC
On i686-apple-darwin9 (rev. 131968), the new test gcc.c-torture/execute/pr34982.c fails:

FAIL: gcc.c-torture/execute/pr34982.c execution,  -O1
 
Comment 19 Dominique d'Humieres 2008-01-30 23:05:47 UTC
Follow up to comment #18, the test pass if I run it directly or if I run
gcc/testsuite/gcc/pr34982.x1.

Any idea why the test is failing in the test suite?

Comment 20 Andrew Pinski 2008-01-30 23:08:36 UTC
(In reply to comment #19)
> Any idea why the test is failing in the test suite?

Yes because main needs a "return 0;"
so the main function should look like:

int main()
{
  something(-1);
  return 0;
}
Comment 21 Dominique d'Humieres 2008-01-30 23:15:49 UTC
> Yes because main needs a "return 0;"

but why does this happen only with -O1?

Comment 22 Andrew Pinski 2008-01-30 23:18:41 UTC
(In reply to comment #21)
> but why does this happen only with -O1?

Random value in eax register so we could put 0 in some cases but not others.
Comment 23 Jan Hubicka 2008-01-30 23:20:31 UTC
Subject: Re:  [4.3 regression] calling a function with undefined parameters causes segmentation fault at -O1 or higher

> (In reply to comment #21)
> > but why does this happen only with -O1?
> 
> Random value in eax register so we could put 0 in some cases but not others.

Oops, I am going to commit obvious fix for that. Looks like my tester
got lucky.
Comment 24 Uroš Bizjak 2008-01-31 08:21:53 UTC
Author: hubicka
Date: Wed Jan 30 23:25:35 2008
New Revision: 131969

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131969
Log:

	* gcc.c-torture/execute/pr34982.c: Add forgotten return 0.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.c-torture/execute/pr34982.c