[PATCH 2/2] Better system header location detection for built-in macro tokens

[PATCH 2/2] Better system header location detection for built-in macro tokens

[Dodji Seketeli]

Hello,

The location for a built-in macro token is BUILTIN_LOCATION.  When we
see that location value, we cannot know if that token was used in a
system header or not.  And that can trigger some unwanted warnings on
e.g, the use of __LONG_LONG_MAX__ built-in macro in system headers
when we compile with -pedantic, like in the test case accompanying
this patch.

In that case, I think we ought to look at the location for the
expansion point of the built-in macro instead.

Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk.

libcpp/

	* line-map.c (linemap_location_in_system_header_p): Check
	expansion point location for built-in macro tokens.

gcc/testsuite/

	* g++.dg/cpp/limits.C: New test.
---
 gcc/testsuite/g++.dg/cpp/limits.C |   21 +++++++++++++++++++++
 libcpp/line-map.c                 |   18 ++++++++++++++----
 2 files changed, 35 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp/limits.C

diff --git a/gcc/testsuite/g++.dg/cpp/limits.C b/gcc/testsuite/g++.dg/cpp/limits.C
new file mode 100644
index 0000000..b64e1e2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp/limits.C
@@ -0,0 +1,21 @@
+// { dg-options "-pedantic" }
+// { dg-do compile }
+
+#include <limits>
+
+// Compiling this with -pedantic was wrongly triggering this error:
+// libstdc++-v3/include/limits:1269:45: warning : use of C++0x long long integer constant [-Wlong-long]
+//       min() _GLIBCXX_USE_NOEXCEPT { return -__LONG_LONG_MAX__ - 1; }
+//                                             ^
+// libstdc++-v3/include/limits:1272:44: warning : use of C++0x long long integer constant [-Wlong-long]
+//       max() _GLIBCXX_USE_NOEXCEPT { return __LONG_LONG_MAX__; }
+//                                            ^
+// libstdc++-v3/include/limits:1342:44: warning : use of C++0x long long integer constant [-Wlong-long]
+//       max() _GLIBCXX_USE_NOEXCEPT { return __LONG_LONG_MAX__ * 2ULL + 1
+//                                            ^
+
+int
+main ()
+{
+    return 0;
+}
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 6e514e5..5a2ca0f 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -754,12 +754,22 @@ linemap_location_in_system_header_p (struct line_maps *set,
 				     source_location location)
 {
   const struct line_map *map = NULL;
-
-  location =
+  source_location loc =
     linemap_resolve_location (set, location, LRK_SPELLING_LOCATION, &map);
 
-  if (location < RESERVED_LOCATION_COUNT)
-    return false;
+  if (loc < RESERVED_LOCATION_COUNT)
+    {
+      /* Maybe LOCATION is for a token that comes from the expansion
+	 of a built-in macro.  In that case, we cannot know from its
+	 spelling location (which is thus a reserved location) where
+	 exactly the token is located.  So let's see where that
+	 built-in macro has been expanded, instead.  */
+      loc = linemap_resolve_location (set, location,
+				      LRK_MACRO_EXPANSION_POINT,
+				      &map);
+      if (loc < RESERVED_LOCATION_COUNT)
+	return false;
+    }
 
   return LINEMAP_SYSP (map);
 }
-- 
		Dodji

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Jason Merrill]

What if the built-in macro appears in a macro defined in a system header 
but used in user code?  This would resolve the location all the way to 
the user code, and warn.  I think we only want to step out until we 
reach a non-built-in macro, not all the way to the outermost expansion 
point.

Jason

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Dodji Seketeli]

Jason Merrill <jason@redhat.com> writes:

> What if the built-in macro appears in a macro defined in a system
> header but used in user code?  This would resolve the location all the
> way to the user code, and warn.  I think we only want to step out
> until we reach a non-built-in macro, not all the way to the outermost
> expansion point.

I think it's not that simple.  If we simply do what you propose, then we
regress on a test like the below, distilled from
testsuite/c-c++-common/dfp/convert-int-saturate.c:

        volatile unsigned int usi;
        int
        main ()
        {
          usi = DEC32_MAX;  /* { dg-warning "overflow in implicit constant conversion" } */
         ...
        }

We fail to warn here because DEC32_MAX is defined in the system header
float.h to a built-in macro:

    #define DEC32_MAX	__DEC32_MAX__

I tried to do what the C FE seems to do, which is to consider that the
default location (the global input_location variable) is on the LHS of
the assignment (on the usi variable), rather than on the token that
comes from DEC32_MAX.  But then it regresses notably on tests like
testsuite/g++.dg/warn/pr35635.C:

    void func3()
    {
      unsigned char uchar_x;

      ...

      uchar_x = bar != 0 
        ? (unsigned char) 1024 
        : -1; /* { dg-warning "negative integer implicitly converted to unsigned type" } */
    }

It seems to me that ultimately, things like conversion routines that
emit diagnostics should know about the expression that represents the
context in which they are emitting the said diagnostic.  In this
particular case, warnings_for_convert_and_check should know about the
assignment expression "usi = DEC32_MAX", so that it can determine that
the whole expression is spelled in user code, and thus, the diagnostic
should be emitted.  IOW, just knowing about the single token on which
the error occurs is not enough to decide.

But handling that seems like a huge task.  So maybe we could, for now,
going for the solution that makes us regress the less, while improving
things nonetheless?

-- 
		Dodji

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Jason Merrill]

On 05/23/2012 04:52 AM, Dodji Seketeli wrote:
> I tried to do what the C FE seems to do, which is to consider that the
> default location (the global input_location variable) is on the LHS of
> the assignment (on the usi variable), rather than on the token that
> comes from DEC32_MAX.

That makes sense.  Though really the relevant location would be the '='.

> But then it regresses notably on tests like
> testsuite/g++.dg/warn/pr35635.C:
>
>      void func3()
>      {
>        unsigned char uchar_x;
>
>        ...
>
>        uchar_x = bar != 0
>          ? (unsigned char) 1024
>          : -1; /* { dg-warning "negative integer implicitly converted to unsigned type" } */
>      }

Regresses how?  In this case the locus of the conversion is again the 
'='.  So the line number of the warning might change, but that doesn't 
seem like a significant regression.

> It seems to me that ultimately, things like conversion routines that
> emit diagnostics should know about the expression that represents the
> context in which they are emitting the said diagnostic.  In this
> particular case, warnings_for_convert_and_check should know about the
> assignment expression "usi = DEC32_MAX", so that it can determine that
> the whole expression is spelled in user code, and thus, the diagnostic
> should be emitted.  IOW, just knowing about the single token on which
> the error occurs is not enough to decide.

I'm not sure I agree.  We don't want to warn about a conversion that's 
all within a macro from a system header, even if it is expanded within 
an expression in user code.  We just need to get the location right for 
our diagnostics, which is an ongoing process, but we've been making a 
lot of progress just recently.

Jason

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Hans-Peter Nilsson]

> From: Dodji Seketeli <dodji@redhat.com>
> Date: Mon, 21 May 2012 15:55:19 +0200

> The location for a built-in macro token is BUILTIN_LOCATION.  When we
> see that location value, we cannot know if that token was used in a
> system header or not.  And that can trigger some unwanted warnings on
> e.g, the use of __LONG_LONG_MAX__ built-in macro in system headers
> when we compile with -pedantic, like in the test case accompanying
> this patch.
> 
> In that case, I think we ought to look at the location for the
> expansion point of the built-in macro instead.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk.
> 

The ChangeLog entry is missing PR decoration (to make the
check-in automatically show up in the bug-report).

> libcpp/
> 
	PR preprocessor/53463
> 	* line-map.c (linemap_location_in_system_header_p): Check
> 	expansion point location for built-in macro tokens.

FWIW, this patch fixes the regressions noted in that PR.  Thanks.

I don't have anything else to add to the discussion besides
uninformed bits like "why warn on language extensions for
built-in macros" or "why can't we (any longer?) see that the
location of use is in a system header when the error message
points at it".

brgds, H-P

RE: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Greta Yorsh]

This patch fixes the regression reported in PR53463 for arm-none-eabi as
well.

Thanks,
Greta


> -----Original Message-----
> From: Dodji Seketeli [mailto:dodji@redhat.com]
> Sent: 21 May 2012 14:55
> To: GCC Patches
> Cc: Jason Merrill; Tom Tromey; Paolo Carlini
> Subject: [PATCH 2/2] Better system header location detection for built-
> in macro tokens
> 
> Hello,
> 
> The location for a built-in macro token is BUILTIN_LOCATION.  When we
> see that location value, we cannot know if that token was used in a
> system header or not.  And that can trigger some unwanted warnings on
> e.g, the use of __LONG_LONG_MAX__ built-in macro in system headers
> when we compile with -pedantic, like in the test case accompanying
> this patch.
> 
> In that case, I think we ought to look at the location for the
> expansion point of the built-in macro instead.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk.
> 
> libcpp/
> 
> 	* line-map.c (linemap_location_in_system_header_p): Check
> 	expansion point location for built-in macro tokens.
> 
> gcc/testsuite/
> 
> 	* g++.dg/cpp/limits.C: New test.
> ---
>  gcc/testsuite/g++.dg/cpp/limits.C |   21 +++++++++++++++++++++
>  libcpp/line-map.c                 |   18 ++++++++++++++----
>  2 files changed, 35 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp/limits.C
> 
> diff --git a/gcc/testsuite/g++.dg/cpp/limits.C
> b/gcc/testsuite/g++.dg/cpp/limits.C
> new file mode 100644
> index 0000000..b64e1e2
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp/limits.C
> @@ -0,0 +1,21 @@
> +// { dg-options "-pedantic" }
> +// { dg-do compile }
> +
> +#include <limits>
> +
> +// Compiling this with -pedantic was wrongly triggering this error:
> +// libstdc++-v3/include/limits:1269:45: warning : use of C++0x long
> long integer constant [-Wlong-long]
> +//       min() _GLIBCXX_USE_NOEXCEPT { return -__LONG_LONG_MAX__ - 1;
> }
> +//                                             ^
> +// libstdc++-v3/include/limits:1272:44: warning : use of C++0x long
> long integer constant [-Wlong-long]
> +//       max() _GLIBCXX_USE_NOEXCEPT { return __LONG_LONG_MAX__; }
> +//                                            ^
> +// libstdc++-v3/include/limits:1342:44: warning : use of C++0x long
> long integer constant [-Wlong-long]
> +//       max() _GLIBCXX_USE_NOEXCEPT { return __LONG_LONG_MAX__ * 2ULL
> + 1
> +//                                            ^
> +
> +int
> +main ()
> +{
> +    return 0;
> +}
> diff --git a/libcpp/line-map.c b/libcpp/line-map.c
> index 6e514e5..5a2ca0f 100644
> --- a/libcpp/line-map.c
> +++ b/libcpp/line-map.c
> @@ -754,12 +754,22 @@ linemap_location_in_system_header_p (struct
> line_maps *set,
>  				     source_location location)
>  {
>    const struct line_map *map = NULL;
> -
> -  location =
> +  source_location loc =
>      linemap_resolve_location (set, location, LRK_SPELLING_LOCATION,
> &map);
> 
> -  if (location < RESERVED_LOCATION_COUNT)
> -    return false;
> +  if (loc < RESERVED_LOCATION_COUNT)
> +    {
> +      /* Maybe LOCATION is for a token that comes from the expansion
> +	 of a built-in macro.  In that case, we cannot know from its
> +	 spelling location (which is thus a reserved location) where
> +	 exactly the token is located.  So let's see where that
> +	 built-in macro has been expanded, instead.  */
> +      loc = linemap_resolve_location (set, location,
> +				      LRK_MACRO_EXPANSION_POINT,
> +				      &map);
> +      if (loc < RESERVED_LOCATION_COUNT)
> +	return false;
> +    }
> 
>    return LINEMAP_SYSP (map);
>  }
> --
> 		Dodji

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Dodji Seketeli]

For built-in macros, the patch is now stepping up until it sees a
location that is not for a built-in macro, and at point, checks if we
are in a system header.

It also uses the location of the '=' operator as the location for
assignment expressions.  I had to adjust a couple of tests due to that change.

Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk.

OK for trunk?

libcpp/

	* line-map.c (linemap_location_in_system_header_p): For built-in
	macro tokens, check the first expansion point location for that is
	not for a token coming from a built-in macro.

gcc/cp/

	* parser.c (cp_parser_assignment_expression): Use the location
	for the LHS as the default location for the expression.

gcc/testsuite/

	* g++.dg/cpp/limits.C: New test.
	* g++.dg/parse/error19.C: Adjust.
	* g++.dg/warn/Wconversion-real-integer2.C: Likewise.
	* g++.dg/warn/pr35635.C: Likewise.
	* g++.old-deja/g++.pt/assign1.C: Likewise.
---
 gcc/cp/parser.c                                    |    4 ++-
 gcc/testsuite/g++.dg/cpp/limits.C                  |   21 ++++++++++++++
 gcc/testsuite/g++.dg/parse/error19.C               |    2 +-
 .../g++.dg/warn/Wconversion-real-integer2.C        |    4 +-
 gcc/testsuite/g++.dg/warn/pr35635.C                |    4 +-
 gcc/testsuite/g++.old-deja/g++.pt/assign1.C        |    5 ++-
 libcpp/line-map.c                                  |   30 +++++++++++++++++--
 7 files changed, 58 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp/limits.C

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 9fd8c84..e393f48 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -7491,7 +7491,9 @@ cp_parser_assignment_expression (cp_parser* parser, bool cast_p,
 	      if (cp_parser_non_integral_constant_expression (parser,
 							      NIC_ASSIGNMENT))
 		return error_mark_node;
-	      /* Build the assignment expression.  */
+	      /* Build the assignment expression.  Its default
+		 location is the location of the '=' token.  */
+	      input_location = loc;
 	      expr = build_x_modify_expr (loc, expr,
 					  assignment_operator,
 					  rhs,
diff --git a/gcc/testsuite/g++.dg/cpp/limits.C b/gcc/testsuite/g++.dg/cpp/limits.C
new file mode 100644
index 0000000..b64e1e2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp/limits.C
@@ -0,0 +1,21 @@
+// { dg-options "-pedantic" }
+// { dg-do compile }
+
+#include <limits>
+
+// Compiling this with -pedantic was wrongly triggering this error:
+// libstdc++-v3/include/limits:1269:45: warning : use of C++0x long long integer constant [-Wlong-long]
+//       min() _GLIBCXX_USE_NOEXCEPT { return -__LONG_LONG_MAX__ - 1; }
+//                                             ^
+// libstdc++-v3/include/limits:1272:44: warning : use of C++0x long long integer constant [-Wlong-long]
+//       max() _GLIBCXX_USE_NOEXCEPT { return __LONG_LONG_MAX__; }
+//                                            ^
+// libstdc++-v3/include/limits:1342:44: warning : use of C++0x long long integer constant [-Wlong-long]
+//       max() _GLIBCXX_USE_NOEXCEPT { return __LONG_LONG_MAX__ * 2ULL + 1
+//                                            ^
+
+int
+main ()
+{
+    return 0;
+}
diff --git a/gcc/testsuite/g++.dg/parse/error19.C b/gcc/testsuite/g++.dg/parse/error19.C
index 010a403..6d84f71 100644
--- a/gcc/testsuite/g++.dg/parse/error19.C
+++ b/gcc/testsuite/g++.dg/parse/error19.C
@@ -10,6 +10,6 @@ const A& foo();
 
 void bar()
 {
-  foo()=A(0); // { dg-error "12:no match for 'operator='" }
+  foo()=A(0); // { dg-error "8:no match for 'operator='" }
   // { dg-message "candidate" "candidate note" { target *-*-* } 13 }
 }
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
index 6a95b0e..0494588 100644
--- a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
@@ -23,11 +23,11 @@
 //
 // That is more useful.
 
-#define INT_MAX __INT_MAX__ // { dg-warning "conversion to .float. alters .int. constant value" }
+#define INT_MAX __INT_MAX__ 
 
 float  vfloat;
 
 void h (void)
 {
-    vfloat = INT_MAX; // { dg-message "in expansion of macro 'INT_MAX'" }
+    vfloat = INT_MAX; // { dg-warning "conversion to .float. alters .int. constant value" }
 }
diff --git a/gcc/testsuite/g++.dg/warn/pr35635.C b/gcc/testsuite/g++.dg/warn/pr35635.C
index 66ade8b..de68ceb 100644
--- a/gcc/testsuite/g++.dg/warn/pr35635.C
+++ b/gcc/testsuite/g++.dg/warn/pr35635.C
@@ -62,9 +62,9 @@ void func3()
   /* At least one branch of ? does not fit in the destination, thus
      warn.  */
   uchar_x = bar != 0 ? 2.1 : 10; /* { dg-warning "conversion" } */
-  uchar_x = bar != 0 
+  uchar_x = bar != 0  /* { dg-warning "negative integer implicitly converted to unsigned type" } */
     ? (unsigned char) 1024 
-    : -1; /* { dg-warning "negative integer implicitly converted to unsigned type" } */
+    : -1; 
 }
 
 void func4()
diff --git a/gcc/testsuite/g++.old-deja/g++.pt/assign1.C b/gcc/testsuite/g++.old-deja/g++.pt/assign1.C
index 854d8ee..951d882 100644
--- a/gcc/testsuite/g++.old-deja/g++.pt/assign1.C
+++ b/gcc/testsuite/g++.old-deja/g++.pt/assign1.C
@@ -2,7 +2,8 @@
 // Origin: Mark Mitchell <mark@codesourcery.com>
 
 template <class T>
-struct S {			// { dg-error "const|operator=" }
+struct S {  // { dg-message "operator=\[\n\r\]*is implicitly deleted" { target c++11 } 5 }
+            // { dg-error "const|operator=" "" { target *-*-* } 5 }
   S();
   T t;
 };
@@ -10,5 +11,5 @@ struct S {			// { dg-error "const|operator=" }
 void f()
 {
   S<const int> s;
-  s = s; // { dg-message "synthesized|deleted" }
+  s = s;
 }
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 8a368ee..e6a344f 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -755,13 +755,35 @@ linemap_location_in_system_header_p (struct line_maps *set,
 {
   const struct line_map *map = NULL;
 
-  location =
-    linemap_resolve_location (set, location, LRK_SPELLING_LOCATION, &map);
-
   if (location < RESERVED_LOCATION_COUNT)
     return false;
 
-  return LINEMAP_SYSP (map);
+  /* Let's look at where the token for LOCATION comes from.  */
+  while (true)
+    {
+      map = linemap_lookup (set, location);
+      if (map != NULL)
+	{
+	  if (!linemap_macro_expansion_map_p (map))
+	    /* It's a normal token.  */
+	    return LINEMAP_SYSP (map);
+	  else
+	    {
+	      /* It's a token resulting from a macro expansion.  */
+	      source_location loc =
+		linemap_macro_map_loc_unwind_toward_spelling (map, location);
+	      if (loc < RESERVED_LOCATION_COUNT)
+		/* This token might come from a built-in macro.  Let's
+		   look at where that macro got expanded.  */
+		location = linemap_macro_map_loc_to_exp_point (map, location);
+	      else
+		location = loc;
+	    }
+	}
+      else
+	break;
+    }
+  return false;
 }
 
 /* Return TRUE if LOCATION is a source code location of a token coming
-- 
		Dodji

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Paolo Carlini]

Hi,

> -          /* Build the assignment expression.  */
> +          /* Build the assignment expression.  Its default
> +         location is the location of the '=' token.  */
> +          input_location = loc;
>          expr = build_x_modify_expr (loc, expr,
>                      assignment_operator,
>                      rhs,

Now I have one more reason to be interested in this issue ;)

Background: as you may have noticed, I'm working on replacing the various build_min* functions used by the various build_x_* functions with _loc variants. One of the problems I'm facing with replacing completely one of them has to do exactly with assignment expressions and the location of the error we get for a library testcase not having the location of the left hand side, as it does now, after the patch. Thus, in short, your change here may well help me as-is ;) 

That said, the tricks we are playing with the global input_location vs the loc we are passing around still confuse me quite a lot. Actually any *assignment* to input_location makes me a bit more nervous than I was already ;) Do you have any idea whether just passing down to build_x_modify_expr a different value for loc instead of assigning to input_location would also work for you? Maybe together with more throughly forwarding the loc from build_x_modify_expr itself to the build_min* functions (ie the project I mentioned above)??

In any case in a day or two I'll let you know how whether your patch as-is works well for the specific issue I'm facing and anyway I will send over the actual work in progress patch which I can't submit because of the regression it would cause.

Thanks,
Paolo

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Jason Merrill]

On 06/02/2012 12:40 PM, Paolo Carlini wrote:
> That said, the tricks we are playing with the global input_location vs the loc we are passing around still confuse me quite a lot. Actually any *assignment* to input_location makes me a bit more nervous than I was already ;) Do you have any idea whether just passing down to build_x_modify_expr a different value for loc instead of assigning to input_location would also work for you? Maybe together with more throughly forwarding the loc from build_x_modify_expr itself to the build_min* functions (ie the project I mentioned above)??

We already pass to build_x_modify_expr the location that he is assigning 
to input_location.  I would guess that the issue in this case is with 
the "in_system_header" macro, which uses input_location.

I think the input_location hack here is OK until we improve our use of 
explicit locations to make it unnecessary.

> -  s = s; // { dg-message "synthesized|deleted" }
> +  s = s;

Why do we lose this error?

Jason

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 1474 bytes --]

On 06/03/2012 05:27 AM, Jason Merrill wrote:
> On 06/02/2012 12:40 PM, Paolo Carlini wrote:
>> That said, the tricks we are playing with the global input_location 
>> vs the loc we are passing around still confuse me quite a lot. 
>> Actually any *assignment* to input_location makes me a bit more 
>> nervous than I was already ;) Do you have any idea whether just 
>> passing down to build_x_modify_expr a different value for loc instead 
>> of assigning to input_location would also work for you? Maybe 
>> together with more throughly forwarding the loc from 
>> build_x_modify_expr itself to the build_min* functions (ie the 
>> project I mentioned above)??
>
> We already pass to build_x_modify_expr the location that he is 
> assigning to input_location.  I would guess that the issue in this 
> case is with the "in_system_header" macro, which uses input_location.
>
> I think the input_location hack here is OK until we improve our use of 
> explicit locations to make it unnecessary.
Good.

In any case, as far as I can see, the assignment Dodji is adding just 
before calling build_x_modify_expr doesn't change anything for "my" 
issue, which actually has to do with build_x_binary_op: if I apply to 
below, thus passing an actual loc to build_min_non_dep_loc, there is a 
diagnostic regression for the locations of 
libstdc++-v3/testsuite/20_util/bind_ref_neg.cc. If you see something 
obviously wrong somewhere, just let me know...

Paolo.

///////////////////

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 4986 bytes --]

Index: typeck.c
===================================================================
--- typeck.c	(revision 188155)
+++ typeck.c	(working copy)
@@ -2696,9 +2696,8 @@ finish_class_member_access_expr (tree object, tree
 	    BASELINK_QUALIFIED_P (member) = 1;
 	  orig_name = member;
 	}
-      return build_min_non_dep (COMPONENT_REF, expr,
-				orig_object, orig_name,
-				NULL_TREE);
+      return build_min_non_dep_loc (UNKNOWN_LOCATION, COMPONENT_REF, expr,
+				    orig_object, orig_name, NULL_TREE);
     }
 
   return expr;
@@ -2763,7 +2762,7 @@ build_x_indirect_ref (location_t loc, tree expr, r
     rval = cp_build_indirect_ref (expr, errorstring, complain);
 
   if (processing_template_decl && rval != error_mark_node)
-    return build_min_non_dep (INDIRECT_REF, rval, orig_expr);
+    return build_min_non_dep_loc (loc, INDIRECT_REF, rval, orig_expr);
   else
     return rval;
 }
@@ -3631,7 +3630,7 @@ build_x_binary_op (location_t loc, enum tree_code
     warn_about_parentheses (code, arg1_code, orig_arg1, arg2_code, orig_arg2);
 
   if (processing_template_decl && expr != error_mark_node)
-    return build_min_non_dep (code, expr, orig_arg1, orig_arg2);
+    return build_min_non_dep_loc (loc, code, expr, orig_arg1, orig_arg2);
 
   return expr;
 }
@@ -3660,8 +3659,8 @@ build_x_array_ref (location_t loc, tree arg1, tree
 		       NULL_TREE, /*overload=*/NULL, complain);
 
   if (processing_template_decl && expr != error_mark_node)
-    return build_min_non_dep (ARRAY_REF, expr, orig_arg1, orig_arg2,
-			      NULL_TREE, NULL_TREE);
+    return build_min_non_dep_loc (loc, ARRAY_REF, expr, orig_arg1, orig_arg2,
+				  NULL_TREE, NULL_TREE);
   return expr;
 }
 
@@ -4764,7 +4763,7 @@ build_x_unary_op (location_t loc, enum tree_code c
     }
 
   if (processing_template_decl && exp != error_mark_node)
-    exp = build_min_non_dep (code, exp, orig_expr,
+    exp = build_min_non_dep_loc (loc, code, exp, orig_expr,
 			     /*For {PRE,POST}{INC,DEC}REMENT_EXPR*/NULL_TREE);
   if (TREE_CODE (exp) == ADDR_EXPR)
     PTRMEM_OK_P (exp) = ptrmem;
@@ -5624,8 +5623,8 @@ build_x_conditional_expr (location_t loc, tree ife
   expr = build_conditional_expr (ifexp, op1, op2, complain);
   if (processing_template_decl && expr != error_mark_node)
     {
-      tree min = build_min_non_dep (COND_EXPR, expr,
-				    orig_ifexp, orig_op1, orig_op2);
+      tree min = build_min_non_dep_loc (loc, COND_EXPR, expr,
+					orig_ifexp, orig_op1, orig_op2);
       /* In C++11, remember that the result is an lvalue or xvalue.
          In C++98, lvalue_kind can just assume lvalue in a template.  */
       if (cxx_dialect >= cxx0x
@@ -5742,7 +5741,8 @@ build_x_compound_expr (location_t loc, tree op1, t
     result = cp_build_compound_expr (op1, op2, complain);
 
   if (processing_template_decl && result != error_mark_node)
-    return build_min_non_dep (COMPOUND_EXPR, result, orig_op1, orig_op2);
+    return build_min_non_dep_loc (loc, COMPOUND_EXPR, result,
+				  orig_op1, orig_op2);
 
   return result;
 }
Index: tree.c
===================================================================
--- tree.c	(revision 188155)
+++ tree.c	(working copy)
@@ -2086,7 +2086,7 @@ build_min (enum tree_code code, tree tt, ...)
    built.  */
 
 tree
-build_min_non_dep (enum tree_code code, tree non_dep, ...)
+build_min_non_dep_loc (location_t loc, enum tree_code code, tree non_dep, ...)
 {
   tree t;
   int length;
@@ -2101,6 +2101,7 @@ tree
     non_dep = TREE_OPERAND (non_dep, 0);
 
   t = make_node (code);
+  SET_EXPR_LOCATION (t, loc);
   length = TREE_CODE_LENGTH (code);
   TREE_TYPE (t) = TREE_TYPE (non_dep);
   TREE_SIDE_EFFECTS (t) = TREE_SIDE_EFFECTS (non_dep);
Index: decl2.c
===================================================================
--- decl2.c	(revision 188155)
+++ decl2.c	(working copy)
@@ -407,8 +407,8 @@ grok_array_decl (location_t loc, tree array_expr,
       expr = build_array_ref (input_location, array_expr, index_exp);
     }
   if (processing_template_decl && expr != error_mark_node)
-    return build_min_non_dep (ARRAY_REF, expr, orig_array_expr, orig_index_exp,
-			      NULL_TREE, NULL_TREE);
+    return build_min_non_dep_loc (loc, ARRAY_REF, expr, orig_array_expr,
+				  orig_index_exp, NULL_TREE, NULL_TREE);
   return expr;
 }
 
Index: cp-tree.h
===================================================================
--- cp-tree.h	(revision 188155)
+++ cp-tree.h	(working copy)
@@ -5697,7 +5697,8 @@ extern bool builtin_valid_in_constant_expr_p    (c
 extern tree build_min				(enum tree_code, tree, ...);
 extern tree build_min_nt_loc			(location_t, enum tree_code,
 						 ...);
-extern tree build_min_non_dep			(enum tree_code, tree, ...);
+extern tree build_min_non_dep_loc		(location_t, enum tree_code,
+						 tree, ...);
 extern tree build_min_non_dep_call_vec		(tree, tree, VEC(tree,gc) *);
 extern tree build_cplus_new			(tree, tree, tsubst_flags_t);
 extern tree build_aggr_init_expr		(tree, tree, tsubst_flags_t);

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Jason Merrill]

On 06/03/2012 06:04 PM, Paolo Carlini wrote:
> if I apply to
> below, thus passing an actual loc to build_min_non_dep_loc, there is a
> diagnostic regression for the locations of
> libstdc++-v3/testsuite/20_util/bind_ref_neg.cc. If you see something
> obviously wrong somewhere, just let me know...

Nope, it looks fine to me.  I guess the change is due to having an 
explicit location on a tree that used to have none.

Jason

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Dodji Seketeli]

> > -  s = s; // { dg-message "synthesized|deleted" }
> > +  s = s;
> 
> Why do we lose this error?

Good question.

This is the result of a weird behaviour of DejaGNU, I think.

For the record, here is the whole test:

     1	// { dg-do compile  }
     2	// Origin: Mark Mitchell <mark@codesourcery.com>
     3	
     4	template <class T>
     5	struct S {			// { dg-error "const|operator=" }
     6	  S();
     7	  T t;
     8	};
     9	
    10	void f()
    11	{
    12	  S<const int> s;
    13	  s = s; // { dg-message "synthesized|deleted" }
    14	}

And here is the output of the test, when run in trunk, without my
patch: http://people.redhat.com/dseketel/gcc/master-g++.log

Then, look at the output with the patch:
http://people.redhat.com/dseketel/gcc/PR53463-g++.log.

It says:

    FAIL: g++.old-deja/g++.pt/assign1.C -std=c++98  (test for warnings, line 13)

But in the compiler output, we duly have:

    assign1.C:13:5: note: synthesized method 'S<const int>& S<const int>::operator=(const
    S<const int>&)' first required here 

which seems correct and shouldn't have triggered a FAIL there, IMHO.
Which, after much punching in my TCL darkness, brings me to this
theory:

It seems the { dg-error "const|operator=" } at line 5 is "swallowing"
somehow the compiler output above.  Why exactly? I have no idea.

One thing I noted though, is that there is no error emitted by g++ on
line 5 that has the string "operator=".  There is just one "note" with
that string.  So the "|operator=" seems superfluous there.  Note
however that the note above, on line 13, has the string "operator=".

Incidentally, changing that DejaGNU directive at line 5 with the more
specific:

    // { dg-error "const member\[^\n\r\]*can't use default assignment operator" }

makes the whole shebang work.

And of course, I sent you the wrong patch.  The one I sent you does
pass testing, but it was just an attempt for me to understand the
issue.  The patch below is the one that uses the more specific DejaGNU
directive above.

libcpp/

	PR preprocessor/53463
	* line-map.c (linemap_location_in_system_header_p): For built-in
	macro tokens, check the first expansion point location for that is
	not for a token coming from a built-in macro.

gcc/cp/

	PR preprocessor/53463
	* parser.c (cp_parser_assignment_expression): Use the location
	for the LHS as the default location for the expression.

gcc/testsuite/

	PR preprocessor/53463
	* g++.dg/cpp/limits.C: New test.
	* g++.dg/parse/error19.C: Adjust.
	* g++.dg/warn/Wconversion-real-integer2.C: Likewise.
	* g++.dg/warn/pr35635.C: Likewise.
	* g++.old-deja/g++.pt/assign1.C: Likewise.
---
 gcc/cp/parser.c                                    |    4 ++-
 gcc/testsuite/g++.dg/cpp/limits.C                  |   21 ++++++++++++++
 gcc/testsuite/g++.dg/parse/error19.C               |    2 +-
 .../g++.dg/warn/Wconversion-real-integer2.C        |    4 +-
 gcc/testsuite/g++.dg/warn/pr35635.C                |    4 +-
 gcc/testsuite/g++.old-deja/g++.pt/assign1.C        |    2 +-
 libcpp/line-map.c                                  |   30 +++++++++++++++++--
 7 files changed, 56 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp/limits.C

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 9fd8c84..e393f48 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -7491,7 +7491,9 @@ cp_parser_assignment_expression (cp_parser* parser, bool cast_p,
 	      if (cp_parser_non_integral_constant_expression (parser,
 							      NIC_ASSIGNMENT))
 		return error_mark_node;
-	      /* Build the assignment expression.  */
+	      /* Build the assignment expression.  Its default
+		 location is the location of the '=' token.  */
+	      input_location = loc;
 	      expr = build_x_modify_expr (loc, expr,
 					  assignment_operator,
 					  rhs,
diff --git a/gcc/testsuite/g++.dg/cpp/limits.C b/gcc/testsuite/g++.dg/cpp/limits.C
new file mode 100644
index 0000000..b64e1e2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp/limits.C
@@ -0,0 +1,21 @@
+// { dg-options "-pedantic" }
+// { dg-do compile }
+
+#include <limits>
+
+// Compiling this with -pedantic was wrongly triggering this error:
+// libstdc++-v3/include/limits:1269:45: warning : use of C++0x long long integer constant [-Wlong-long]
+//       min() _GLIBCXX_USE_NOEXCEPT { return -__LONG_LONG_MAX__ - 1; }
+//                                             ^
+// libstdc++-v3/include/limits:1272:44: warning : use of C++0x long long integer constant [-Wlong-long]
+//       max() _GLIBCXX_USE_NOEXCEPT { return __LONG_LONG_MAX__; }
+//                                            ^
+// libstdc++-v3/include/limits:1342:44: warning : use of C++0x long long integer constant [-Wlong-long]
+//       max() _GLIBCXX_USE_NOEXCEPT { return __LONG_LONG_MAX__ * 2ULL + 1
+//                                            ^
+
+int
+main ()
+{
+    return 0;
+}
diff --git a/gcc/testsuite/g++.dg/parse/error19.C b/gcc/testsuite/g++.dg/parse/error19.C
index 010a403..6d84f71 100644
--- a/gcc/testsuite/g++.dg/parse/error19.C
+++ b/gcc/testsuite/g++.dg/parse/error19.C
@@ -10,6 +10,6 @@ const A& foo();
 
 void bar()
 {
-  foo()=A(0); // { dg-error "12:no match for 'operator='" }
+  foo()=A(0); // { dg-error "8:no match for 'operator='" }
   // { dg-message "candidate" "candidate note" { target *-*-* } 13 }
 }
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
index 6a95b0e..0494588 100644
--- a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
@@ -23,11 +23,11 @@
 //
 // That is more useful.
 
-#define INT_MAX __INT_MAX__ // { dg-warning "conversion to .float. alters .int. constant value" }
+#define INT_MAX __INT_MAX__ 
 
 float  vfloat;
 
 void h (void)
 {
-    vfloat = INT_MAX; // { dg-message "in expansion of macro 'INT_MAX'" }
+    vfloat = INT_MAX; // { dg-warning "conversion to .float. alters .int. constant value" }
 }
diff --git a/gcc/testsuite/g++.dg/warn/pr35635.C b/gcc/testsuite/g++.dg/warn/pr35635.C
index 66ade8b..de68ceb 100644
--- a/gcc/testsuite/g++.dg/warn/pr35635.C
+++ b/gcc/testsuite/g++.dg/warn/pr35635.C
@@ -62,9 +62,9 @@ void func3()
   /* At least one branch of ? does not fit in the destination, thus
      warn.  */
   uchar_x = bar != 0 ? 2.1 : 10; /* { dg-warning "conversion" } */
-  uchar_x = bar != 0 
+  uchar_x = bar != 0  /* { dg-warning "negative integer implicitly converted to unsigned type" } */
     ? (unsigned char) 1024 
-    : -1; /* { dg-warning "negative integer implicitly converted to unsigned type" } */
+    : -1; 
 }
 
 void func4()
diff --git a/gcc/testsuite/g++.old-deja/g++.pt/assign1.C b/gcc/testsuite/g++.old-deja/g++.pt/assign1.C
index 854d8ee..95cbee0 100644
--- a/gcc/testsuite/g++.old-deja/g++.pt/assign1.C
+++ b/gcc/testsuite/g++.old-deja/g++.pt/assign1.C
@@ -2,7 +2,7 @@
 // Origin: Mark Mitchell <mark@codesourcery.com>
 
 template <class T>
-struct S {			// { dg-error "const|operator=" }
+struct S {  // { dg-error "const member\[^\n\r\]*can't use default assignment operator" }
   S();
   T t;
 };
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 8a368ee..e6a344f 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -755,13 +755,35 @@ linemap_location_in_system_header_p (struct line_maps *set,
 {
   const struct line_map *map = NULL;
 
-  location =
-    linemap_resolve_location (set, location, LRK_SPELLING_LOCATION, &map);
-
   if (location < RESERVED_LOCATION_COUNT)
     return false;
 
-  return LINEMAP_SYSP (map);
+  /* Let's look at where the token for LOCATION comes from.  */
+  while (true)
+    {
+      map = linemap_lookup (set, location);
+      if (map != NULL)
+	{
+	  if (!linemap_macro_expansion_map_p (map))
+	    /* It's a normal token.  */
+	    return LINEMAP_SYSP (map);
+	  else
+	    {
+	      /* It's a token resulting from a macro expansion.  */
+	      source_location loc =
+		linemap_macro_map_loc_unwind_toward_spelling (map, location);
+	      if (loc < RESERVED_LOCATION_COUNT)
+		/* This token might come from a built-in macro.  Let's
+		   look at where that macro got expanded.  */
+		location = linemap_macro_map_loc_to_exp_point (map, location);
+	      else
+		location = loc;
+	    }
+	}
+      else
+	break;
+    }
+  return false;
 }
 
 /* Return TRUE if LOCATION is a source code location of a token coming
-- 
		Dodji

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Jason Merrill]

On 06/04/2012 08:36 AM, Dodji Seketeli wrote:
> -	      /* Build the assignment expression.  */
> +	      /* Build the assignment expression.  Its default
> +		 location is the location of the '=' token.  */
> +	      input_location = loc;
>   	      expr = build_x_modify_expr (loc, expr,
>   					  assignment_operator,
>   					  rhs,

Hmm, I meant to say this before, but apparently forgot: We should the 
old input_location after build_x_modify_expr returns.

> This is the result of a weird behaviour of DejaGNU, I think.

Weird, I don't see how that could happen.  The full regexp in dg.exp is

(^|\n)(\[^\n\]+$line\[^\n\]*($pattern)\[^\n\]*\n?)+

I don't see how that could swallow a newline and some of another 
diagnostic.  But your workaround is OK.

Jason

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Dodji Seketeli]

Jason Merrill <jason@redhat.com> writes:

> On 06/04/2012 08:36 AM, Dodji Seketeli wrote:
>> -	      /* Build the assignment expression.  */
>> +	      /* Build the assignment expression.  Its default
>> +		 location is the location of the '=' token.  */
>> +	      input_location = loc;
>>   	      expr = build_x_modify_expr (loc, expr,
>>   					  assignment_operator,
>>   					  rhs,
>
> Hmm, I meant to say this before, but apparently forgot: We should the
> old input_location after build_x_modify_expr returns.

Woops, I wonder how I forgot that.

Fixed thus.  Testing is still underway.


libcpp/

	PR preprocessor/53463
	* line-map.c (linemap_location_in_system_header_p): For built-in
	macro tokens, check the first expansion point location for that is
	not for a token coming from a built-in macro.

gcc/cp/

	PR preprocessor/53463
	* parser.c (cp_parser_assignment_expression): Use the location
	for the LHS as the default location for the expression.

gcc/testsuite/

	PR preprocessor/53463
	* g++.dg/cpp/limits.C: New test.
	* g++.dg/parse/error19.C: Adjust.
	* g++.dg/warn/Wconversion-real-integer2.C: Likewise.
	* g++.dg/warn/pr35635.C: Likewise.
	* g++.old-deja/g++.pt/assign1.C: Likewise.
---
 gcc/cp/parser.c                                    |    7 ++++-
 gcc/testsuite/g++.dg/cpp/limits.C                  |   21 ++++++++++++++
 gcc/testsuite/g++.dg/parse/error19.C               |    2 +-
 .../g++.dg/warn/Wconversion-real-integer2.C        |    4 +-
 gcc/testsuite/g++.dg/warn/pr35635.C                |    4 +-
 gcc/testsuite/g++.old-deja/g++.pt/assign1.C        |    2 +-
 libcpp/line-map.c                                  |   30 +++++++++++++++++--
 7 files changed, 59 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp/limits.C

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 9fd8c84..7e71888 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -7479,6 +7479,7 @@ cp_parser_assignment_expression (cp_parser* parser, bool cast_p,
 	  if (assignment_operator != ERROR_MARK)
 	    {
 	      bool non_constant_p;
+	      location_t saved_input_location;
 
 	      /* Parse the right-hand side of the assignment.  */
 	      tree rhs = cp_parser_initializer_clause (parser, &non_constant_p);
@@ -7491,11 +7492,15 @@ cp_parser_assignment_expression (cp_parser* parser, bool cast_p,
 	      if (cp_parser_non_integral_constant_expression (parser,
 							      NIC_ASSIGNMENT))
 		return error_mark_node;
-	      /* Build the assignment expression.  */
+	      /* Build the assignment expression.  Its default
+		 location is the location of the '=' token.  */
+	      saved_input_location = input_location;
+	      input_location = loc;
 	      expr = build_x_modify_expr (loc, expr,
 					  assignment_operator,
 					  rhs,
 					  tf_warning_or_error);
+	      input_location = saved_input_location;
 	    }
 	}
     }
diff --git a/gcc/testsuite/g++.dg/cpp/limits.C b/gcc/testsuite/g++.dg/cpp/limits.C
new file mode 100644
index 0000000..b64e1e2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp/limits.C
@@ -0,0 +1,21 @@
+// { dg-options "-pedantic" }
+// { dg-do compile }
+
+#include <limits>
+
+// Compiling this with -pedantic was wrongly triggering this error:
+// libstdc++-v3/include/limits:1269:45: warning : use of C++0x long long integer constant [-Wlong-long]
+//       min() _GLIBCXX_USE_NOEXCEPT { return -__LONG_LONG_MAX__ - 1; }
+//                                             ^
+// libstdc++-v3/include/limits:1272:44: warning : use of C++0x long long integer constant [-Wlong-long]
+//       max() _GLIBCXX_USE_NOEXCEPT { return __LONG_LONG_MAX__; }
+//                                            ^
+// libstdc++-v3/include/limits:1342:44: warning : use of C++0x long long integer constant [-Wlong-long]
+//       max() _GLIBCXX_USE_NOEXCEPT { return __LONG_LONG_MAX__ * 2ULL + 1
+//                                            ^
+
+int
+main ()
+{
+    return 0;
+}
diff --git a/gcc/testsuite/g++.dg/parse/error19.C b/gcc/testsuite/g++.dg/parse/error19.C
index 010a403..6d84f71 100644
--- a/gcc/testsuite/g++.dg/parse/error19.C
+++ b/gcc/testsuite/g++.dg/parse/error19.C
@@ -10,6 +10,6 @@ const A& foo();
 
 void bar()
 {
-  foo()=A(0); // { dg-error "12:no match for 'operator='" }
+  foo()=A(0); // { dg-error "8:no match for 'operator='" }
   // { dg-message "candidate" "candidate note" { target *-*-* } 13 }
 }
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
index 6a95b0e..0494588 100644
--- a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
@@ -23,11 +23,11 @@
 //
 // That is more useful.
 
-#define INT_MAX __INT_MAX__ // { dg-warning "conversion to .float. alters .int. constant value" }
+#define INT_MAX __INT_MAX__ 
 
 float  vfloat;
 
 void h (void)
 {
-    vfloat = INT_MAX; // { dg-message "in expansion of macro 'INT_MAX'" }
+    vfloat = INT_MAX; // { dg-warning "conversion to .float. alters .int. constant value" }
 }
diff --git a/gcc/testsuite/g++.dg/warn/pr35635.C b/gcc/testsuite/g++.dg/warn/pr35635.C
index 66ade8b..de68ceb 100644
--- a/gcc/testsuite/g++.dg/warn/pr35635.C
+++ b/gcc/testsuite/g++.dg/warn/pr35635.C
@@ -62,9 +62,9 @@ void func3()
   /* At least one branch of ? does not fit in the destination, thus
      warn.  */
   uchar_x = bar != 0 ? 2.1 : 10; /* { dg-warning "conversion" } */
-  uchar_x = bar != 0 
+  uchar_x = bar != 0  /* { dg-warning "negative integer implicitly converted to unsigned type" } */
     ? (unsigned char) 1024 
-    : -1; /* { dg-warning "negative integer implicitly converted to unsigned type" } */
+    : -1; 
 }
 
 void func4()
diff --git a/gcc/testsuite/g++.old-deja/g++.pt/assign1.C b/gcc/testsuite/g++.old-deja/g++.pt/assign1.C
index 854d8ee..95cbee0 100644
--- a/gcc/testsuite/g++.old-deja/g++.pt/assign1.C
+++ b/gcc/testsuite/g++.old-deja/g++.pt/assign1.C
@@ -2,7 +2,7 @@
 // Origin: Mark Mitchell <mark@codesourcery.com>
 
 template <class T>
-struct S {			// { dg-error "const|operator=" }
+struct S {  // { dg-error "const member\[^\n\r\]*can't use default assignment operator" }
   S();
   T t;
 };
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 8a368ee..e6a344f 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -755,13 +755,35 @@ linemap_location_in_system_header_p (struct line_maps *set,
 {
   const struct line_map *map = NULL;
 
-  location =
-    linemap_resolve_location (set, location, LRK_SPELLING_LOCATION, &map);
-
   if (location < RESERVED_LOCATION_COUNT)
     return false;
 
-  return LINEMAP_SYSP (map);
+  /* Let's look at where the token for LOCATION comes from.  */
+  while (true)
+    {
+      map = linemap_lookup (set, location);
+      if (map != NULL)
+	{
+	  if (!linemap_macro_expansion_map_p (map))
+	    /* It's a normal token.  */
+	    return LINEMAP_SYSP (map);
+	  else
+	    {
+	      /* It's a token resulting from a macro expansion.  */
+	      source_location loc =
+		linemap_macro_map_loc_unwind_toward_spelling (map, location);
+	      if (loc < RESERVED_LOCATION_COUNT)
+		/* This token might come from a built-in macro.  Let's
+		   look at where that macro got expanded.  */
+		location = linemap_macro_map_loc_to_exp_point (map, location);
+	      else
+		location = loc;
+	    }
+	}
+      else
+	break;
+    }
+  return false;
 }
 
 /* Return TRUE if LOCATION is a source code location of a token coming
-- 
		Dodji

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Jason Merrill]

OK.

Jason

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Mike Stump]

On Jun 4, 2012, at 5:36 AM, Dodji Seketeli wrote:
>>> -  s = s; // { dg-message "synthesized|deleted" }
>>> +  s = s;
>> 
>> Why do we lose this error?
> 
> Good question.
> 
> This is the result of a weird behaviour of DejaGNU, I think.

Yup, that is truly weird.  The problem is that the | operator is binding funny because there are no () around the substituted variables in the matching expressions in the .exp files, and the messages for a single line get the opportunity to match and eat multiple lines.

So, dejagnu is using:

	{[0-9]+: error:[^\n]*const|operator}

instead of:

	{[0-9]+: error:[^\n]*(const|operator)}

and that makes all the difference.

If you want, you can use just const, like so:

template <class T>
struct S {  // { dg-error "const" }
  S();
  T t;
};

The key is not to use |, or if you want to use |, you can (for now, until the underlying bug is fix) use:

template <class T>
struct S {  // { dg-error "(const|operator)" }
  S();
  T t;
}

to fix the underlying problem, we need something like:

Index: gcc-dg.exp
===================================================================
--- gcc-dg.exp	(revision 188120)
+++ gcc-dg.exp	(working copy)
@@ -744,6 +744,12 @@ if { [info procs saved-dg-error] == [lis
 proc process-message { msgproc msgprefix dgargs } {
     upvar dg-messages dg-messages
 
+    # We must enclose the expression in () to ensure any | in the
+    # pattern doesn't escape from this part of the expression.
+    set darg1 [lindex $dgargs 0]
+    set dgargs [lindex $dgargs 1]
+    set dgargs "$darg1 ($dgargs)"
+
     # Process the dg- directive, including adding the regular expression
     # to the new message entry in dg-messages.
     set msgcnt [llength ${dg-messages}]


which I'll test and see how bad off any other test cases are.  If people on fast machines want to weigh in, that'd be good.

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Jason Merrill]

On 06/04/2012 02:54 PM, Mike Stump wrote:
> Yup, that is truly weird.  The problem is that the | operator is binding funny because there are no () around the substituted variables in the matching expressions in the .exp files, and the messages for a single line get the opportunity to match and eat multiple lines.
>
> So, dejagnu is using:
>
> 	{[0-9]+: error:[^\n]*const|operator}
>
> instead of:
>
> 	{[0-9]+: error:[^\n]*(const|operator)}
>
> and that makes all the difference.

That was my guess, too, but when I look at dg.exp it seems that the 
pattern is properly wrapped in parens.

Jason

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Mike Stump]

On Jun 4, 2012, at 12:30 PM, Jason Merrill wrote:
> On 06/04/2012 02:54 PM, Mike Stump wrote:
>> Yup, that is truly weird.  The problem is that the | operator is binding funny because there are no () around the substituted variables in the matching expressions in the .exp files, and the messages for a single line get the opportunity to match and eat multiple lines.
>> 
>> So, dejagnu is using:
>> 
>> 	{[0-9]+: error:[^\n]*const|operator}
>> 
>> instead of:
>> 
>> 	{[0-9]+: error:[^\n]*(const|operator)}
>> 
>> and that makes all the difference.
> 
> That was my guess, too, but when I look at dg.exp it seems that the pattern is properly wrapped in parens.

That one is, the problem is the work that added error: and warning: processing differentiation puts together a new expression with concatenation and it is that use that _also_ needs protecting.

	error: CONCAT regexp

is wrong, whereas:

	error: CONCAT (regexp)

is correct.  This type of patch:

-       set expmsg "$column: $msgprefix\[^\n\]*$expmsg"
+       set expmsg "$column: $msgprefix\[^\n\]*($expmsg)"

might have been easier to understand, but, that repeats three times and is dependent upon people getting the obscure detail of () correct.  I instead add the () at the top, to ensure it is impossible to ever have this error again, no matter the code rearrangements and to have it appear just once, instead of three times.  Possibly:

-	set expmsg [lindex $newentry 2]
+	set expmsg "([lindex $newentry 2])"

might be better...

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Jason Merrill]

On 06/04/2012 05:27 PM, Mike Stump wrote:
> That one is, the problem is the work that added error: and warning: processing differentiation puts together a new expression with concatenation and it is that use that _also_ needs protecting.

Aha!  Sneaky gcc-dg.exp.

> -	set expmsg [lindex $newentry 2]
> +	set expmsg "([lindex $newentry 2])"

Looks good to me.

Jason

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Mike Stump]

On Jun 4, 2012, at 2:34 PM, Jason Merrill wrote:
> On 06/04/2012 05:27 PM, Mike Stump wrote:
>> That one is, the problem is the work that added error: and warning: processing differentiation puts together a new expression with concatenation and it is that use that _also_ needs protecting.
> 
> Aha!  Sneaky gcc-dg.exp.
> 
>> -	set expmsg [lindex $newentry 2]
>> +	set expmsg "([lindex $newentry 2])"
> 
> Looks good to me.

Curious, after reading the code a little more and a couple testsuite run, it can only be modified later...  so...  I tested:

Index: testsuite/lib/gcc-dg.exp
===================================================================
--- testsuite/lib/gcc-dg.exp	(revision 188120)
+++ testsuite/lib/gcc-dg.exp	(working copy)
@@ -766,14 +766,14 @@ proc process-message { msgproc msgprefix
 	# Remove "COLUMN:" from the original expression and move it
 	# to the proper place in the search expression.
 	regsub "^\[0-9\]+:" $expmsg "" expmsg
-	set expmsg "$column: $msgprefix\[^\n\]*$expmsg"
+	set expmsg "$column: $msgprefix\[^\n\]*($expmsg)"
     } elseif [string match "" [lindex $newentry 0]] {
 	# The specified line number is 0; don't expect a column number.
-	set expmsg "$msgprefix\[^\n\]*$expmsg"
+	set expmsg "$msgprefix\[^\n\]*($expmsg)"
     } else {
 	# There is no column number in the search expression, but we
 	# should expect one in the message itself.
-	set expmsg "\[0-9\]+: $msgprefix\[^\n\]*$expmsg"
+	set expmsg "\[0-9\]+: $msgprefix\[^\n\]*($expmsg)"
     }
 
     set newentry [lreplace $newentry 2 2 $expmsg]

and found:

Tests that now fail, but worked before:

g++.dg/cpp0x/pr31437.C  (test for errors, line 2)
g++.dg/cpp0x/regress/template-function1.C (test for excess errors)
g++.dg/other/warning1.C -std=c++11  (test for warnings, line 10)
g++.dg/other/warning1.C -std=c++11  (test for warnings, line 11)
g++.dg/other/warning1.C -std=c++11 (test for excess errors)
g++.dg/other/warning1.C -std=c++98  (test for warnings, line 10)
g++.dg/other/warning1.C -std=c++98  (test for warnings, line 11)
g++.dg/other/warning1.C -std=c++98 (test for excess errors)
g++.old-deja/g++.brendan/crash16.C -std=gnu++11 (test for excess errors)
g++.old-deja/g++.brendan/crash16.C -std=gnu++98 (test for excess errors)
gcc.dg/format/attr-3.c   (test for excess errors)
gcc.dg/format/attr-3.c   format on enum (test for errors, line 31)
gcc.dg/format/attr-3.c   format on struct (test for errors, line 29)
gcc.dg/format/attr-3.c   format on union (test for errors, line 30)
gcc.dg/format/attr-3.c   format_arg on enum (test for errors, line 35)
gcc.dg/format/attr-3.c   format_arg on struct (test for errors, line 33)
gcc.dg/format/attr-3.c   format_arg on union (test for errors, line 34)
gcc.dg/format/attr-3.c  -DWIDE  (test for excess errors)
gcc.dg/format/attr-3.c  -DWIDE  format on enum (test for errors, line 31)
gcc.dg/format/attr-3.c  -DWIDE  format on struct (test for errors, line 29)
gcc.dg/format/attr-3.c  -DWIDE  format on union (test for errors, line 30)
gcc.dg/format/attr-3.c  -DWIDE  format_arg on enum (test for errors, line 35)
gcc.dg/format/attr-3.c  -DWIDE  format_arg on struct (test for errors, line 33)
gcc.dg/format/attr-3.c  -DWIDE  format_arg on union (test for errors, line 34)
gcc.dg/format/sentinel-1.c   (test for excess errors)
gcc.dg/format/sentinel-1.c  -DWIDE  (test for excess errors)
gcc.dg/noncompile/940227-1.c  -O0  (test for excess errors)
gcc.dg/noncompile/940227-1.c  -O1  (test for excess errors)
gcc.dg/noncompile/940227-1.c  -O2  (test for excess errors)
gcc.dg/noncompile/940227-1.c  -O3 -fomit-frame-pointer  (test for excess errors)
gcc.dg/noncompile/940227-1.c  -O3 -g  (test for excess errors)
gcc.dg/noncompile/940227-1.c  -Os  (test for excess errors)
gcc.dg/noncompile/950825-1.c  -O0  (test for excess errors)
gcc.dg/noncompile/950825-1.c  -O1  (test for excess errors)
gcc.dg/noncompile/950825-1.c  -O2  (test for excess errors)
gcc.dg/noncompile/950825-1.c  -O3 -fomit-frame-pointer  (test for excess errors)
gcc.dg/noncompile/950825-1.c  -O3 -g  (test for excess errors)
gcc.dg/noncompile/950825-1.c  -Os  (test for excess errors)
gcc.dg/noncompile/init-1.c  -O0  (test for excess errors)
gcc.dg/noncompile/init-1.c  -O1  (test for excess errors)
gcc.dg/noncompile/init-1.c  -O2  (test for excess errors)
gcc.dg/noncompile/init-1.c  -O3 -fomit-frame-pointer  (test for excess errors)
gcc.dg/noncompile/init-1.c  -O3 -g  (test for excess errors)
gcc.dg/noncompile/init-1.c  -Os  (test for excess errors)
gfortran.dg/pr20865.f90  -O  (test for excess errors)

I'll have to check all these out before we can fix it, but my guess is all these tests are broken.  I checked the first one, and sure enough it was.  I'll start fixing them all so that we can fix this issue in the testsuite driver.  If anyone wants to help out...

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Mike Stump]

On Jun 4, 2012, at 7:46 PM, Mike Stump wrote:
> g++.dg/other/warning1.C -std=c++11  (test for warnings, line 10)
> g++.dg/other/warning1.C -std=c++11  (test for warnings, line 11)
> g++.dg/other/warning1.C -std=c++11 (test for excess errors)
> g++.dg/other/warning1.C -std=c++98  (test for warnings, line 10)
> g++.dg/other/warning1.C -std=c++98  (test for warnings, line 11)
> g++.dg/other/warning1.C -std=c++98 (test for excess errors)

So, this one is not obvious.  The testcase checks for warning, and the compiler generates an error.  Could a C++ maintainer weigh in on it?

Re: [PATCH 2/2] Better system header location detection for built-in macro tokens

[Jason Merrill]

On 06/05/2012 12:14 AM, Mike Stump wrote:
> On Jun 4, 2012, at 7:46 PM, Mike Stump wrote:
>> g++.dg/other/warning1.C -std=c++11  (test for warnings, line 10)
>> g++.dg/other/warning1.C -std=c++11  (test for warnings, line 11)
>> g++.dg/other/warning1.C -std=c++11 (test for excess errors)
>> g++.dg/other/warning1.C -std=c++98  (test for warnings, line 10)
>> g++.dg/other/warning1.C -std=c++98  (test for warnings, line 11)
>> g++.dg/other/warning1.C -std=c++98 (test for excess errors)
>
> So, this one is not obvious.  The testcase checks for warning, and the compiler generates an error.  Could a C++ maintainer weigh in on it?

The errors are correct; indeed, we have always given an error for this 
testcase with -pedantic-errors.  It does seem like a diagnostic quality 
regression that we no longer complain specifically about the division by 
0.  That regression seems to have happened in 4.3.  Since this PR was 
specifically about the formatting of 1.0f, that should have had its own 
dg-warning line instead of being lumped in with the initialization error.

Jason