Bug 22052 - [4.0 Regression] redefinition of inline function succeeds
Summary: [4.0 Regression] redefinition of inline function succeeds
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 4.0.0
: P2 normal
Target Milestone: 4.0.3
Assignee: Andrew Pinski
URL: http://gcc.gnu.org/ml/gcc-patches/200...
Keywords: accepts-invalid, patch
: 21975 23474 24140 24656 (view as bug list)
Depends on: 23104
Blocks: 16989 21975
  Show dependency treegraph
 
Reported: 2005-06-13 19:02 UTC by Eric Christopher
Modified: 2005-11-03 15:30 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work: 3.4.4 4.1.0
Known to fail: 4.0.0
Last reconfirmed: 2005-06-15 18:25:47


Attachments
diff1 (1008 bytes, patch)
2005-06-14 01:10 UTC, Eric Christopher
Details | Diff
diff2 (1.26 KB, patch)
2005-06-15 20:50 UTC, Eric Christopher
Details | Diff
Simplier patch (503 bytes, patch)
2005-09-16 18:50 UTC, Andrew Pinski
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Christopher 2005-06-13 19:02:10 UTC
This doesn't error out:

#include <stdio.h>
static inline int foo(void) {     return 1; }
static inline int foo(void) {     return 0; }
static inline int bar(void) {     return foo(); }
int main(void)
{
    if (bar())
      printf("1\n");
    else
      printf("0\n");

    return 0;
}

It appears to be a problem with inlining since if the inline keyword is removed
it works (well, fails) as expected.
Comment 1 Andrew Pinski 2005-06-13 19:07:53 UTC
Confirmed, this started after "3.5.0 20040909".
Comment 2 Andrew Pinski 2005-06-13 19:11:56 UTC
It started to fail after "4.0.0 20041124" but before "4.0.0 20050225" (the branching).
Comment 3 Andrew Pinski 2005-06-13 19:17:50 UTC
Hmm, the most obvious patch (which is mine) is the following:
2005-01-04  Andrew Pinski  <pinskia@physics.uc.edu>

        PR c/19152
        * c-decl.c (diagnose_mismatched_decls): Accept "extern inline" declared
        after the full declaration if the are in two different TUs.
Comment 4 Andrew Pinski 2005-06-13 19:35:48 UTC
This patch should fix it (I don't have time right now, school is getting in the way, could you test it):
Index: c-decl.c
===============================================================
====
RCS file: /cvs/gcc/gcc/gcc/c-decl.c,v
retrieving revision 1.662
diff -u -p -r1.662 c-decl.c
--- c-decl.c    6 Jun 2005 19:31:24 -0000       1.662
+++ c-decl.c    13 Jun 2005 19:35:08 -0000
@@ -1318,9 +1318,7 @@ diagnose_mismatched_decls (tree newdecl,
                  return false;
                }
              /* If both decls have not extern inline, reject the new decl.  */
-             if (!DECL_DECLARED_INLINE_P (olddecl)
-                 && !DECL_EXTERNAL (olddecl)
-                 && !DECL_DECLARED_INLINE_P (newdecl)
+             if (!DECL_EXTERNAL (olddecl)
                  && !DECL_EXTERNAL (newdecl))
                {
                  error ("%Jredefinition of %qD", newdecl, newdecl);
Comment 5 Joseph S. Myers 2005-06-13 23:18:47 UTC
There is a case the multiple static inline definitions of the same function in a
translation unit violate the constraint of  6.9#3 (no more than one external
definition of an identifier with internal linkage in the same translation unit),
if you take the definition of "inline definition" in 6.7.4#6 as only applying to
functions with external linkage.

In general C99 presumes there is no more than one definition of a function in a
translation unit (in 6.7.4#6) but has no constraint requiring this to be
diagnosed.  I think it would be reasonable in C99 mode, as part of an
implementation of C99 inline semantics, to prohibit all such cases.  inline in
gnu89 mode should remain compatible with traditional GCC semantics, which in the
case of the present bug means rejecting the problem code but in some cases may
mean accepting it (in particular where one definition is extern inline).
Comment 6 Eric Christopher 2005-06-14 01:10:42 UTC
Created attachment 9083 [details]
diff1
Comment 7 Eric Christopher 2005-06-14 01:11:05 UTC
Joseph,

So, what you're saying is that we should accept this:

extern inline int foo (void) { return 0; }
inline int foo (void) { return 1; }

unless we're in strict c99 mode.

And reject this:

static inline int foo (void) { return 0; }
static inline int foo (void) { return 1; }

in all cases?

I'm trying to get a set of tests up for everything we should and shouldn't accept :)

Current diff is attached. Mostly untested except for the above testcases.

-eric
Comment 8 joseph@codesourcery.com 2005-06-14 01:19:05 UTC
Subject: Re:  [4.0/4.1 Regression] redefinition of inline function
 succeeds

On Tue, 14 Jun 2005, echristo at redhat dot com wrote:

> So, what you're saying is that we should accept this:
> 
> extern inline int foo (void) { return 0; }
> inline int foo (void) { return 1; }
> 
> unless we're in strict c99 mode.

A C99 conditional is pointless and possibly harmful for this case without 
the rest of C99 semantics, since "inline" and "extern inline" are 
(approximately) the other way round in C99 from in gnu89.  (It's only 
approximate; for example,

inline int foo (void) { return 1; }

on its own is an inline definition only in C99 but

inline int foo (void) { return 1; }
int foo(void);

yields an external definition in C99: any declaration of foo without 
inline or with extern causes it to be exported.)

So, just accept all cases GCC has traditionally accepted here without 
regard to language version for now.

> And reject this:
> 
> static inline int foo (void) { return 0; }
> static inline int foo (void) { return 1; }
> 
> in all cases?

Yes.

Comment 9 Eric Christopher 2005-06-15 18:25:47 UTC
I'll take this. I've got a patch just completing testing.
Comment 10 Eric Christopher 2005-06-15 20:49:20 UTC
OK. This diff works, and passes bootstrap with no regressions for both
--enable-intermodule and normally on x86-linux. The --enable-intermodule bits
are why we split this out in the first place, but same_translation_unit_p in the
condition seems to handle that case.

Joseph: What do you think?

-eric
Comment 11 Eric Christopher 2005-06-15 20:50:10 UTC
Created attachment 9095 [details]
diff2
Comment 12 joseph@codesourcery.com 2005-06-16 12:41:32 UTC
Subject: Re:  [4.0/4.1 Regression] redefinition of inline function
 succeeds

On Wed, 15 Jun 2005, echristo at redhat dot com wrote:

> OK. This diff works, and passes bootstrap with no regressions for both
> --enable-intermodule and normally on x86-linux. The --enable-intermodule bits
> are why we split this out in the first place, but same_translation_unit_p in the
> condition seems to handle that case.
> 
> Joseph: What do you think?

I'm more interested in the testcases here than in the front-end changes 
which make the testcases pass.

Comment 13 Eric Christopher 2005-06-17 20:21:44 UTC
Here are the 4 testcases that I've been using to test, the first two you've
seen. (and no, the dg stuff isn't done yet, but that's merely to make the
testsuite happy.)

test1:
/* { dg-do compile } */
/* This test is expected to fail with an error for the redefinition of foo.
   This violates the constraint of 6.9#3 (no more than one external definition
   of an identifier with internal linkage in the same translation unit).  */
static inline int foo(void) { return 1; }
static inline int foo(void) { return 0; } /* { dg-error "" } */

test2:
/* { dg-do compile } */
/* This test should compile successfully.  */
extern inline int foo(void) { return 0; }
inline int foo (void) { return 1; }

test3:
/* { dg-do compile } */
/* This testcase should fail since we're redefining foo in the same
   translation unit.  */
extern inline int foo(void) { return 0; }
inline int foo (void) { return 1; }
int foo (void) { return 2; } /* { dg-error "error: redefinition of 'foo'" } */

test4:
/* { dg-do compile } */
/* This testcase should fail since we're redefining foo in the same
   translation unit.  */
int foo (void) { return 2; }
extern inline int foo (void) { return 1; }
Comment 14 joseph@codesourcery.com 2005-06-17 20:48:32 UTC
Subject: Re:  [4.0/4.1 Regression] redefinition of inline function
 succeeds

On Fri, 17 Jun 2005, echristo at redhat dot com wrote:

> Here are the 4 testcases that I've been using to test, the first two you've
> seen. (and no, the dg stuff isn't done yet, but that's merely to make the
> testsuite happy.)

These results seem OK.

Comment 15 CVS Commits 2005-06-29 00:11:48 UTC
Subject: Bug 22052

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	echristo@gcc.gnu.org	2005-06-29 00:11:37

Modified files:
	gcc            : c-decl.c ChangeLog 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.dg: inline1.c inline2.c inline3.c inline4.c 
	                      inline5.c 

Log message:
	2005-06-28  Eric Christopher  <echristo@redhat.com>
	
	PR c/22052
	PR c/21975
	* c-decl.c (diagnose_mismatched_decls): Define DECL_EXTERN_INLINE.
	Use. Fix detection of invalid extern inline redefinition.
	
	2005-06-28  Eric Christopher  <echristo@redhat.com>
	
	PR c/22052
	PR c/21975
	* gcc.dg/inline1.c: New test.
	* gcc.dg/inline2.c: Ditto.
	* gcc.dg/inline3.c: Ditto.
	* gcc.dg/inline4.c: Ditto.
	* gcc.dg/inline5.c: Ditto.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/c-decl.c.diff?cvsroot=gcc&r1=1.668&r2=1.669
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.9267&r2=2.9268
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.5702&r2=1.5703
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/inline1.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/inline2.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/inline3.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/inline4.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/inline5.c.diff?cvsroot=gcc&r1=NONE&r2=1.1

Comment 16 Andrew Pinski 2005-06-29 01:59:45 UTC
This is now rejected like it should on the mainline.
Comment 17 Andrew Pinski 2005-08-19 02:36:08 UTC
*** Bug 23474 has been marked as a duplicate of this bug. ***
Comment 18 Andrew Pinski 2005-08-19 02:37:10 UTC
*** Bug 21975 has been marked as a duplicate of this bug. ***
Comment 19 Andrew Pinski 2005-09-16 18:43:49 UTC
I have a fix for the mainline which also fixes this and 23104 and reverting back to the old code which 
was added for 19152.
Comment 20 Andrew Pinski 2005-09-16 18:50:29 UTC
Created attachment 9743 [details]
Simplier patch

This is the simplier patch which should fix this on the 4.0 branch without
causing PR 23104 regression.
Comment 21 Andrew Pinski 2005-09-30 11:46:39 UTC
*** Bug 24140 has been marked as a duplicate of this bug. ***
Comment 22 Andrew Pinski 2005-10-02 19:00:08 UTC
Patch posted here:
http://gcc.gnu.org/ml/gcc-patches/2005-10/msg00050.html
Comment 23 Andrew Pinski 2005-10-04 19:41:29 UTC
Fixed the correct way in 4.0.3 which does not expose PR 23104.
Comment 24 CVS Commits 2005-10-04 19:41:53 UTC
Subject: Bug 22052

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-4_0-branch
Changes by:	pinskia@gcc.gnu.org	2005-10-04 19:41:47

Modified files:
	gcc            : ChangeLog c-decl.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.dg: inline1.c inline2.c inline3.c inline4.c 

Log message:
	2005-10-04  Andrew Pinski  <pinskia@physics.uc.edu>
	
	PR c/22052
	PR c/21975
	* c-decl.c (diagnose_mismatched_decls): Fix check for both
	decls being "extern inline".
	2005-10-04  Eric Christopher  <echristo@apple.com>
	
	PR c/22052
	PR c/21975
	* gcc.dg/inline1.c: New test.
	* gcc.dg/inline2.c: Ditto.
	* gcc.dg/inline3.c: Ditto.
	* gcc.dg/inline4.c: Ditto.
	* gcc.dg/inline5.c: Ditto.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.7592.2.445&r2=2.7592.2.446
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/c-decl.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.630.6.20&r2=1.630.6.21
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.5084.2.424&r2=1.5084.2.425
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/inline1.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.18.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/inline2.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.18.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/inline3.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.18.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/inline4.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.18.1

Comment 25 Andrew Pinski 2005-11-03 15:30:08 UTC
*** Bug 24656 has been marked as a duplicate of this bug. ***