Re: C++ PATCH for c++/70449 (ICE when printing a filename of unknown location)

[Marek Polacek <polacek at redhat dot com>]

On Wed, Mar 30, 2016 at 11:51:26PM +0100, Manuel LÃpez-IbÃÃez wrote:
> On 30 March 2016 at 23:42, Manuel LÃpez-IbÃÃez <lopezibanez@gmail.com> wrote:
> > On 30/03/16 17:14, Marek Polacek wrote:
> >>
> >> This test ICEs since the addition of the assert in pp_string which ensures
> >> that
> >> we aren't trying to print an empty string.  But that's what happens here,
> >> the
> >> location is actually UNKNOWN_LOCATION, so LOCATION_FILE on that yields
> >> null.
> >> Fixed byt not trying to print the filename of UNKNOWN_LOCATION.
> 
> > Even if we accept the broken location for now (adding some FIXME to the code
> > would help the next person to realise this is not normal), if
> > LOCATION_FILE() is NULL, we should print "progname" like
> > diagnostic_build_prefix() does. Moreover, the filename string should be
> > built with file_name_as_prefix() to get correct coloring.
> 
> Even better: Use "f ? f : progname" in file_name_as_prefix() and
> simplify the code to:

It seems wrong to me to just add that.  I wonder if the function shouldn't
use diagnostic_get_location_text instead which also handles "<built-in>" etc.
In any case I'd rather not touch the diagnostic stuff in the scope of this
patch (which wouldn't probably be suitable for stage4 anyway).
 
>       /* FIXME: Somehow we may get UNKNOWN_LOCATION here: See
> g++.dg/cpp0x/constexpr-70449.C */
>       const char * prefix = file_name_as_prefix (context,
> LOCATION_FILE (location));
>         pp_verbatim (context->printer,
>                      TREE_CODE (p->decl) == TREE_LIST
>                      ? _("%s: In substitution of %qS:\n")
>                      : _("%s: In instantiation of %q#D:\n"),
>                      prefix, p->decl);
>       free (prefix);
> 
> Fixes the ICE, adds colors, mentions the broken location and does not
> add extra strings.

It fixes the ICE, but I don't see any more colors than with my patch.
Moreover your suggestion would print
cc1plus: : In instantiation of âconstexpr int f() [with int I = 0]â:
, not sure how's that better than simply saying
In instantiation of âconstexpr int f() [with int I = 0]â:
I.e. adding "cc1plus" to the output doesn't seem like an improvement.

Anyway, here's a patch which uses file_name_as_prefix.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-03-31  Marek Polacek  <polacek@redhat.com>

	PR c++/70449
	* error.c (print_instantiation_full_context): Build prefix by using
	file_name_as_prefix.

	* g++.dg/cpp0x/constexpr-70449.C: New test.

diff --git gcc/cp/error.c gcc/cp/error.c
index aa5fd41..14b0205 100644
--- gcc/cp/error.c
+++ gcc/cp/error.c
@@ -3312,12 +3312,15 @@ print_instantiation_full_context (diagnostic_context *context)
 
   if (p)
     {
+      char *prefix = file_name_as_prefix (context, LOCATION_FILE (location)
+						   ? LOCATION_FILE (location)
+						   : progname);
       pp_verbatim (context->printer,
 		   TREE_CODE (p->decl) == TREE_LIST
-		   ? _("%s: In substitution of %qS:\n")
-		   : _("%s: In instantiation of %q#D:\n"),
-		   LOCATION_FILE (location),
-		   p->decl);
+		   ? _("%sIn substitution of %qS:\n")
+		   : _("%sIn instantiation of %q#D:\n"),
+		   prefix, p->decl);
+      free (prefix);
 
       location = p->locus;
       p = p->next;
diff --git gcc/testsuite/g++.dg/cpp0x/constexpr-70449.C gcc/testsuite/g++.dg/cpp0x/constexpr-70449.C
index e69de29..bc5dd71 100644
--- gcc/testsuite/g++.dg/cpp0x/constexpr-70449.C
+++ gcc/testsuite/g++.dg/cpp0x/constexpr-70449.C
@@ -0,0 +1,12 @@
+// PR c++/70449
+// { dg-do compile { target c++11 } }
+
+template <int>
+constexpr
+int f (void)
+{
+  enum E { a = f<0> () };
+  return 0;
+}
+
+// { dg-error "body of constexpr function" "" { target { ! c++14 } } 0 }

	Marek