Re: RFA: PATCH to tree-inline.c:remap_decls for c++/70353 (ICE with __func__ and constexpr)

[Richard Biener]

On Tue, Mar 29, 2016 at 8:42 PM, Jason Merrill  wrote:
> On 03/29/2016 06:37 AM, Jan Hubicka wrote:
>>>
>>> On Mon, Mar 28, 2016 at 11:26 PM, Jason Merrill  wrote:

 The constexpr evaluation code uses the inlining code to remap the
 constexpr
 function body for evaluation so that recursion works properly.  In this
 testcase __func__ is declared as a static local variable, so rather than
 remap it, remap_decls tries to add it to the local_decls list for the
 function we're inlining into.  But there is no such function in this
 case,
 so we crash.

 Avoid the add_local_decl call when cfun is null avoids the ICE (thanks
 Jakub), but results in an undefined symbol.  Calling
 varpool_node::finalize_decl instead allows cgraph to handle the
 reference
 from 'c' properly.

 OK if testing passes?
>>>
>>>
>>> So ce will never be instantiated?
>
>
> Right, because no calls to it survive constexpr evaluation.  And the front
> end avoids finalizing it in make_rtl_for_nonlocal_decl...which is another
> place I could fix this.  Thus.
>
> Tested x86_64-pc-linux-gnu, applying to trunk.

Much better!

Thanks,
Richard.

> Jason
>

Re: RFA: PATCH to tree-inline.c:remap_decls for c++/70353 (ICE with __func__ and constexpr)

[Jason Merrill]

On 03/29/2016 06:37 AM, Jan Hubicka wrote:

On Mon, Mar 28, 2016 at 11:26 PM, Jason Merrill  wrote:

The constexpr evaluation code uses the inlining code to remap the constexpr
function body for evaluation so that recursion works properly.  In this
testcase __func__ is declared as a static local variable, so rather than
remap it, remap_decls tries to add it to the local_decls list for the
function we're inlining into.  But there is no such function in this case,
so we crash.

Avoid the add_local_decl call when cfun is null avoids the ICE (thanks
Jakub), but results in an undefined symbol.  Calling
varpool_node::finalize_decl instead allows cgraph to handle the reference
from 'c' properly.

OK if testing passes?


So ce will never be instantiated?


Right, because no calls to it survive constexpr evaluation.  And the 
front end avoids finalizing it in make_rtl_for_nonlocal_decl...which is 
another place I could fix this.  Thus.


Tested x86_64-pc-linux-gnu, applying to trunk.

Jason

commit 28b2d2bfe2c55aa41e1d540a30595357f000279c
Author: Jason Merrill 
Date:   Mon Mar 28 17:14:43 2016 -0400

	PR c++/70353

gcc/
	* tree-inline.c (remap_decls): Don't add_local_decl if
	cfun is null.
gcc/cp/
	* decl.c (make_rtl_for_nonlocal_decl): Don't defer local statics
	in constexpr functions.

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index cd5db3f..cfae210 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -6251,8 +6251,11 @@ make_rtl_for_nonlocal_decl (tree decl, tree init, const char* asmspec)
 return;
 
   /* We defer emission of local statics until the corresponding
- DECL_EXPR is expanded.  */
-  defer_p = DECL_FUNCTION_SCOPE_P (decl) || DECL_VIRTUAL_P (decl);
+ DECL_EXPR is expanded.  But with constexpr its function might never
+ be expanded, so go ahead and tell cgraph about the variable now.  */
+  defer_p = ((DECL_FUNCTION_SCOPE_P (decl)
+	  && !DECL_DECLARED_CONSTEXPR_P (DECL_CONTEXT (decl)))
+	 || DECL_VIRTUAL_P (decl));
 
   /* Defer template instantiations.  */
   if (DECL_LANG_SPECIFIC (decl)
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-__func__2.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-__func__2.C
new file mode 100644
index 000..e678290
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-__func__2.C
@@ -0,0 +1,13 @@
+// PR c++/70353
+// { dg-do link { target c++11 } }
+
+constexpr const char* ce ()
+{
+  return __func__;
+}
+
+const char *c = ce();
+
+int main()
+{
+}
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 9d4f8f7..5206d20 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -616,7 +616,8 @@ remap_decls (tree decls, vec **nonlocalized_list,
 	  /* We need to add this variable to the local decls as otherwise
 	 nothing else will do so.  */
 	  if (TREE_CODE (old_var) == VAR_DECL
-	  && ! DECL_EXTERNAL (old_var))
+	  && ! DECL_EXTERNAL (old_var)
+	  && cfun)
 	add_local_decl (cfun, old_var);
 	  if ((!optimize || debug_info_level > DINFO_LEVEL_TERSE)
 	  && !DECL_IGNORED_P (old_var)

Re: RFA: PATCH to tree-inline.c:remap_decls for c++/70353 (ICE with __func__ and constexpr)

[Jan Hubicka]

> On Mon, Mar 28, 2016 at 11:26 PM, Jason Merrill  wrote:
> > The constexpr evaluation code uses the inlining code to remap the constexpr
> > function body for evaluation so that recursion works properly.  In this
> > testcase __func__ is declared as a static local variable, so rather than
> > remap it, remap_decls tries to add it to the local_decls list for the
> > function we're inlining into.  But there is no such function in this case,
> > so we crash.
> >
> > Avoid the add_local_decl call when cfun is null avoids the ICE (thanks
> > Jakub), but results in an undefined symbol.  Calling
> > varpool_node::finalize_decl instead allows cgraph to handle the reference
> > from 'c' properly.
> >
> > OK if testing passes?
> 
> So ce will never be instantiated?
> 
> And 'c' will have a DECL_INITIAL of __func__ so I wonder why the cgraph
> code when finalizing 'c' does not end up seeing __func__ and finalizing it?
> Ah, it only creates a varpool-node it seems but never finalizes it itself.
> Honza?

While we walk DECL_INITIAL to populate symbol table, we want explicit
cgraph_finalize/varpool_finalize on every symbol that needs to be output.
Otherwise they count just as external references.

Honza
> 
> Richard.

Re: RFA: PATCH to tree-inline.c:remap_decls for c++/70353 (ICE with __func__ and constexpr)

[Richard Biener]

On Mon, Mar 28, 2016 at 11:26 PM, Jason Merrill  wrote:
> The constexpr evaluation code uses the inlining code to remap the constexpr
> function body for evaluation so that recursion works properly.  In this
> testcase __func__ is declared as a static local variable, so rather than
> remap it, remap_decls tries to add it to the local_decls list for the
> function we're inlining into.  But there is no such function in this case,
> so we crash.
>
> Avoid the add_local_decl call when cfun is null avoids the ICE (thanks
> Jakub), but results in an undefined symbol.  Calling
> varpool_node::finalize_decl instead allows cgraph to handle the reference
> from 'c' properly.
>
> OK if testing passes?

So ce will never be instantiated?

And 'c' will have a DECL_INITIAL of __func__ so I wonder why the cgraph
code when finalizing 'c' does not end up seeing __func__ and finalizing it?
Ah, it only creates a varpool-node it seems but never finalizes it itself.
Honza?

Richard.

RFA: PATCH to tree-inline.c:remap_decls for c++/70353 (ICE with __func__ and constexpr)

[Jason Merrill]

The constexpr evaluation code uses the inlining code to remap the 
constexpr function body for evaluation so that recursion works properly. 
 In this testcase __func__ is declared as a static local variable, so 
rather than remap it, remap_decls tries to add it to the local_decls 
list for the function we're inlining into.  But there is no such 
function in this case, so we crash.


Avoid the add_local_decl call when cfun is null avoids the ICE (thanks 
Jakub), but results in an undefined symbol.  Calling 
varpool_node::finalize_decl instead allows cgraph to handle the 
reference from 'c' properly.


OK if testing passes?
commit d875b432434be92ab345c3851fa3ba4bef738399
Author: Jason Merrill 
Date:   Mon Mar 28 17:14:43 2016 -0400

	PR c++/70353
	* tree-inline.c (remap_decls): Use varpool_node::finalize_decl if
	cfun is null.

diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-__func__2.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-__func__2.C
new file mode 100644
index 000..e678290
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-__func__2.C
@@ -0,0 +1,13 @@
+// PR c++/70353
+// { dg-do link { target c++11 } }
+
+constexpr const char* ce ()
+{
+  return __func__;
+}
+
+const char *c = ce();
+
+int main()
+{
+}
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 9d4f8f7..74b0ca9 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -614,10 +614,16 @@ remap_decls (tree decls, vec **nonlocalized_list,
   if (can_be_nonlocal (old_var, id))
 	{
 	  /* We need to add this variable to the local decls as otherwise
-	 nothing else will do so.  */
+	 nothing else will do so.  If we're not in a function,
+	 tell cgraph about it.  */
 	  if (TREE_CODE (old_var) == VAR_DECL
 	  && ! DECL_EXTERNAL (old_var))
-	add_local_decl (cfun, old_var);
+	{
+	  if (cfun)
+		add_local_decl (cfun, old_var);
+	  else
+		varpool_node::finalize_decl (old_var);
+	}
 	  if ((!optimize || debug_info_level > DINFO_LEVEL_TERSE)
 	  && !DECL_IGNORED_P (old_var)
 	  && nonlocalized_list)