Bug 39242 - [4.4 Regression] Inconsistent reject / accept of code
Summary: [4.4 Regression] Inconsistent reject / accept of code
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.4.0
: P1 normal
Target Milestone: 4.4.0
Assignee: Richard Biener
URL: http://gcc.gnu.org/ml/gcc-patches/200...
Keywords: rejects-valid
Depends on:
Blocks:
 
Reported: 2009-02-19 13:38 UTC by Richard Biener
Modified: 2009-02-24 14:52 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.3.3
Known to fail:
Last reconfirmed: 2009-02-19 16:59:20


Attachments
original testcase (15.97 KB, text/plain)
2009-02-19 13:40 UTC, Richard Biener
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Biener 2009-02-19 13:38:22 UTC
With -O0 the following is accepted, with -O1 it is rejected with

Parser.3.ii: In member function ‘void RepPtrStore<_Tp, _Bt>::_assign(_Tp*) [with _Tp = YStatement, _Bt = YCode]’:
Parser.3.ii:11:   instantiated from ‘RepPtrStore<_Tp, _Bt>::~RepPtrStore() [with _Tp = YStatement, _Bt = YCode]’
Parser.3.ii:21:   instantiated from here
Parser.3.ii:16: error: no matching function for call to ‘Rep::unref(YStatement*&)’
Parser.3.ii:3: note: candidates are: void Rep::unref() const
Parser.3.ii:4: note:                 static void Rep::unref(const Rep*)


testcase:

class Rep {
public:
    void unref() const { }
    static void unref (const Rep * obj_r) { obj_r->unref(); }
};
template<typename _Tp, typename _Bt = _Tp>
class RepPtrStore {
    _Tp * _obj;
    void _assign( _Tp * new_r );
public:
    ~RepPtrStore() { _assign( 0 ); }
};
template<typename _Tp,typename _Bt>
void RepPtrStore<_Tp,_Bt>::_assign( _Tp * new_r )
{
  Rep::unref( _obj );
}
class RepPtrBase { };
template<typename _Bt> class PtrBase : public RepPtrBase { };
template<typename _Tp, typename _Bt = _Tp>
class Ptr : public PtrBase<_Bt> {
    RepPtrStore<_Tp,_Bt> _ptr;
};
class YCode;
class YStatement;
typedef Ptr<YStatement,YCode> YStatementPtr;
extern template class RepPtrStore<YStatement,YCode>;
class ExecutionEnvironment {
    YStatementPtr m_statement;
    ~ExecutionEnvironment() { };
};

GCC 4.3 accepts the code unconditionally, so does EDG.
Comment 1 Richard Biener 2009-02-19 13:40:04 UTC
Created attachment 17329 [details]
original testcase

Original testcase from Yast YCP.  Requires -std=c++0x.
Comment 2 Richard Biener 2009-02-19 13:46:14 UTC
Randomly poking around the tree shows

Index: cp/ChangeLog
===================================================================
--- cp/ChangeLog        (revision 138149)
+++ cp/ChangeLog        (revision 138150)
@@ -1,3 +1,20 @@
+2008-07-25  Jan Hubicka  <jh@suse.cz>
+
+       * typeck.c (inline_conversion): Remove.
+       (cp_build_function_call): Do not use inline_conversion.
+       * decl.c (duplicate_decls): Do not insist on inline being declared
+       early.
+       (start_cleanup_fn): Do not assume that INLINE flags prevent function
+       from being output.  We now remove static functions always.
+       (finish_function): Do return warning on all static functions.
+       * call.c (build_over_call): Do not use inline_conversion.
+       * cp-tree.h (possibly_inlined_p): Declare.
+       (inline_conversion): Remove.
+       * pt.c (instantiate_decl): Use possibly_inlined_p predicate.
+       * decl2.c (cp_write_global_declarations): Likewise.
+       (mark_used): Likewise.
+       (possibly_inlined_p): New functions.

as very likely candidate introducing the regression (the optimize dependency).
Comment 3 Richard Biener 2009-02-19 13:50:45 UTC
The error is that RepPtrStore<YStatement,YCode>::assign is instantiated at all
in face of extern template class RepPtrStore<YStatement,YCode>;
Comment 4 Richard Biener 2009-02-19 14:19:08 UTC
This fixes it for me:

Index: decl2.c
===================================================================
--- decl2.c     (revision 144292)
+++ decl2.c     (working copy)
@@ -3755,7 +3755,8 @@ bool
 possibly_inlined_p (tree decl)
 {
   gcc_assert (TREE_CODE (decl) == FUNCTION_DECL);
-  if (DECL_UNINLINABLE (decl))
+  if (DECL_UNINLINABLE (decl)
+      || DECL_REALLY_EXTERN (decl))
     return false;
   if (!optimize)
     return DECL_DECLARED_INLINE_P (decl);
Comment 5 Richard Biener 2009-02-19 14:30:47 UTC
Or rather we don't want template instantiation (and errors from it) to differ
from optimized to non-optimized build.  So,

Index: cp/pt.c
===================================================================
--- cp/pt.c     (revision 144292)
+++ cp/pt.c     (working copy)
@@ -15285,7 +15285,7 @@ instantiate_decl (tree d, int defer_ok,
       /* ... but we instantiate inline functions so that we can inline
         them and ... */
       && ! (TREE_CODE (d) == FUNCTION_DECL
-           && possibly_inlined_p (d))
+           && DECL_DECLARED_INLINE_P (d))
       /* ... we instantiate static data members whose values are
         needed in integral constant expressions.  */
       && ! (TREE_CODE (d) == VAR_DECL
@@ -15363,7 +15363,7 @@ instantiate_decl (tree d, int defer_ok,
       /* Instantiate inline functions so that the inliner can do its
         job, even though we'll not be emitting a copy of this
         function.  */
-      if (!(TREE_CODE (d) == FUNCTION_DECL && possibly_inlined_p (d)))
+      if (!(TREE_CODE (d) == FUNCTION_DECL && !DECL_UNINLINABLE (d)))
        goto out;
     }
 
?
Comment 6 Richard Biener 2009-02-19 14:51:34 UTC
Note that in the original testcase YStatement is incomplete, but properly
derives from Rep in the TU with the explicit instantiation of
RepPtrStore<YStatement,YCode>.

Instead of not instantiating so much we could also try to suppress errors
(like SFINAE) here.

Of course reading the C++0x standard about explicit instantiation declarations
isn't very conclusive (either there's no explanation or everything covering
explicit instantiations also covers their declarations).
Comment 7 Jan Hubicka 2009-02-19 14:59:19 UTC
Subject: Re:  [4.4 Regression] Inconsistent reject / accept of code

> Index: cp/pt.c
> ===================================================================
> --- cp/pt.c     (revision 144292)
> +++ cp/pt.c     (working copy)
> @@ -15285,7 +15285,7 @@ instantiate_decl (tree d, int defer_ok,
>        /* ... but we instantiate inline functions so that we can inline
>          them and ... */
>        && ! (TREE_CODE (d) == FUNCTION_DECL
> -           && possibly_inlined_p (d))
> +           && DECL_DECLARED_INLINE_P (d))

this way we will lose inlining opurtunities at -O3, sine we will never
instantiate methods not declared inline.

Is standard really making difference in between inline and !inline here?
Or we are just supposed to suppress the errors and instantiate only
stuff that is complette or are we supposed to give error at -O0 too or
is it up to our choice?

Honza
Comment 8 Richard Biener 2009-02-19 15:20:36 UTC
Note that the patch only affects extern explicit template instantiations.

  /* Check to see whether we know that this template will be
     instantiated in some other file, as with "extern template"
     extension.  */
  external_p = (DECL_INTERFACE_KNOWN (d) && DECL_REALLY_EXTERN (d));
  /* In general, we do not instantiate such templates...  */
  if (external_p
      /* ... but we instantiate inline functions so that we can inline
         them and ... */
      && ! (TREE_CODE (d) == FUNCTION_DECL
            && DECL_DECLARED_INLINE_P (d))

For them we would no longer inline (or rather instantiate) not inline declared
members.  This may affect parts of libstdc++, but I don't know.

In YCP the extern trick seems to be used to break a dependency cycle, so the
situation is somewhat special.

The ultimate question is of course if the standard allows (or even requires)
an error here.
Comment 9 Jan Hubicka 2009-02-19 15:40:16 UTC
Subject: Re:  [4.4 Regression] Inconsistent reject / accept of code

>   /* Check to see whether we know that this template will be
>      instantiated in some other file, as with "extern template"
>      extension.  */
>   external_p = (DECL_INTERFACE_KNOWN (d) && DECL_REALLY_EXTERN (d));
>   /* In general, we do not instantiate such templates...  */
>   if (external_p
>       /* ... but we instantiate inline functions so that we can inline
>          them and ... */
>       && ! (TREE_CODE (d) == FUNCTION_DECL
>             && DECL_DECLARED_INLINE_P (d))

Hmm, in general it is benefical to instantiate stuff for sake of IPA
optimizers.  It would be interesting to know how this affects code
quality...

Honza
Comment 10 Mark Mitchell 2009-02-19 16:41:22 UTC
Subject: Re:  [4.4 Regression] Inconsistent reject / accept
 of code

rguenth at gcc dot gnu dot org wrote:

> The ultimate question is of course if the standard allows (or even requires)
> an error here.

The (someone old) C++ WP I have is pretty clear:

"An explicit instantiation declaration that names a class template
specialization has no effect on the class template specialization
itself (except for perhaps resulting in its implicit instantiation).
Except for inline functions, other explicit
instantiation declarations have the effect of suppressing the implicit
instantiation of the entity to which they refer. [ Note:
The intent is that an inline function that is the subject of an explicit
instantiation declaration will still be implicitly instantiated
when used so that the body can be considered for inlining, but that no
out-of-line copy of the inline function
would be generated in the translation unit. —end note ]"

Here, "inline function" is of course the C++ definition thereof, i.e.,
functions declared "inline" or defined in the body of a class
definition, rather than outside the class.

What that means is that we *must not* implicitly instantiate things
declared "extern template" unless they are DECL_DECLARED_INLINE_P.  As a
consequence, at -O3, we cannot implicitly instantiate non-inline "extern
template" functions.

So, I think first hunk in the patch is correct.  It needs a comment,
though, right above DECL_DECLARED_INLINE to point out that this is a
restriction coming from the standard:

/* An explicit instantiation declaration prohibits implicit
instantiation of non-inline functions.  With high levels of
optimization, we would normally inline non-inline functions -- but we're
not allowed to do that for "extern template" functions.  Therefore, we
check DECL_DECLARED_INLINE_P, rather than possibly_inlined_p.  */

OK with that change.

I don't yet understand why the second hunk is required.

Comment 11 rguenther@suse.de 2009-02-19 16:54:54 UTC
Subject: Re:  [4.4 Regression] Inconsistent reject / accept
 of code

On Thu, 19 Feb 2009, mark at codesourcery dot com wrote:

> ------- Comment #10 from mark at codesourcery dot com  2009-02-19 16:41 -------
> Subject: Re:  [4.4 Regression] Inconsistent reject / accept
>  of code
> 
> rguenth at gcc dot gnu dot org wrote:
> 
> > The ultimate question is of course if the standard allows (or even requires)
> > an error here.
> 
> The (someone old) C++ WP I have is pretty clear:
> 
> "An explicit instantiation declaration that names a class template
> specialization has no effect on the class template specialization
> itself (except for perhaps resulting in its implicit instantiation).
> Except for inline functions, other explicit
> instantiation declarations have the effect of suppressing the implicit
> instantiation of the entity to which they refer. [ Note:
> The intent is that an inline function that is the subject of an explicit
> instantiation declaration will still be implicitly instantiated
> when used so that the body can be considered for inlining, but that no
> out-of-line copy of the inline function
> would be generated in the translation unit. ???end note ]"
> 
> Here, "inline function" is of course the C++ definition thereof, i.e.,
> functions declared "inline" or defined in the body of a class
> definition, rather than outside the class.
> 
> What that means is that we *must not* implicitly instantiate things
> declared "extern template" unless they are DECL_DECLARED_INLINE_P.  As a
> consequence, at -O3, we cannot implicitly instantiate non-inline "extern
> template" functions.
> 
> So, I think first hunk in the patch is correct.  It needs a comment,
> though, right above DECL_DECLARED_INLINE to point out that this is a
> restriction coming from the standard:
> 
> /* An explicit instantiation declaration prohibits implicit
> instantiation of non-inline functions.  With high levels of
> optimization, we would normally inline non-inline functions -- but we're
> not allowed to do that for "extern template" functions.  Therefore, we
> check DECL_DECLARED_INLINE_P, rather than possibly_inlined_p.  */
> 
> OK with that change.
> 
> I don't yet understand why the second hunk is required.

It is probably not, I just looked for all possibly_inlined_p
occurrences in instantiate_decl and tried to make sure behavior
does not depend on optimization level.

I'll split out the first part and test/submit it.

Richard.
Comment 12 Jason Merrill 2009-02-19 17:39:04 UTC
(In reply to comment #10)
> What that means is that we *must not* implicitly instantiate things
> declared "extern template" unless they are DECL_DECLARED_INLINE_P.  As a
> consequence, at -O3, we cannot implicitly instantiate non-inline "extern
> template" functions.

I'm not entirely sure that's what we want it to say, but it does seem like a reasonable expectation for users to have.

Beyond this issue, what is the compile speed impact of the earlier change to use possibly_inlined_p?  It seems like it might be making us speculatively instantiate a lot more functions for potential inlining even at -O1, which I would expect to cause memory bloat and slower compilation.
Comment 13 Jan Hubicka 2009-02-20 14:39:54 UTC
Subject: Re:  [4.4 Regression] Inconsistent reject / accept of code

> > What that means is that we *must not* implicitly instantiate things
> > declared "extern template" unless they are DECL_DECLARED_INLINE_P.  As a
> > consequence, at -O3, we cannot implicitly instantiate non-inline "extern
> > template" functions.
> 
> I'm not entirely sure that's what we want it to say, but it does seem like a
> reasonable expectation for users to have.
> 
> Beyond this issue, what is the compile speed impact of the earlier change to
> use possibly_inlined_p?  It seems like it might be making us speculatively
> instantiate a lot more functions for potential inlining even at -O1, which I
> would expect to cause memory bloat and slower compilation.

There was no slowdowns visible on our C++ testers, so I hope it is not
too bad.  We now get rid of unreachable functions quite early in queue
so they are not consuming too much of compilation time.

Honza
Comment 14 Richard Biener 2009-02-21 12:54:52 UTC
Patch posted.
Comment 15 Richard Biener 2009-02-24 14:50:47 UTC
Subject: Bug 39242

Author: rguenth
Date: Tue Feb 24 14:50:30 2009
New Revision: 144408

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=144408
Log:
2009-02-24  Richard Guenther  <rguenther@suse.de>

	PR c++/39242
	* pt.c (instantiate_decl): Do not instantiate extern, non-inline
	declared functions.

	* g++.dg/template/instantiate10.C: New testcase.

Added:
    trunk/gcc/testsuite/g++.dg/template/instantiate10.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/pt.c
    trunk/gcc/testsuite/ChangeLog

Comment 16 Richard Biener 2009-02-24 14:52:27 UTC
Fixed.