Fix 69650, bogus line numbers from libcpp

Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

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

This is a case where bogus #line directives can confuse libcpp into 
producing nonsensical line numbers, even leading to a crash later on in LTO.

The following patch moves the test earlier to a point where we can more 
easily recover from the error condition. I should note that I changed 
the raw fprintf (stderr) to a cpp_error call, which is a slight change 
in behaviour (we don't even get to LTO anymore due to erroring out earlier).

Bootstrapped and tested on x86_64-linux (as always including Ada, which 
failed with an earlier version of the patch). Ok?


Bernd

[-- Attachment #2: 69650.diff --]
[-- Type: text/x-patch, Size: 3528 bytes --]

	PR lto/69650
	* directives.c (do_linemarker): Test for file left but not entered
	here.
	* line-map.c (linemap_add): Not here.

	PR lto/69650
	* gcc.dg/pr69650.c: New test.

Index: libcpp/directives.c
===================================================================
--- libcpp/directives.c	(revision 234025)
+++ libcpp/directives.c	(working copy)
@@ -1046,6 +1046,19 @@ do_linemarker (cpp_reader *pfile)
 
   skip_rest_of_line (pfile);
 
+  if (reason == LC_LEAVE)
+    {
+      const line_map_ordinary *from;      
+      if (MAIN_FILE_P (map)
+	  || (new_file
+	      && (from = INCLUDED_FROM (pfile->line_table, map)) != NULL
+	      && filename_cmp (ORDINARY_MAP_FILE_NAME (from), new_file) != 0))
+	{
+	  cpp_error (pfile, CPP_DL_ERROR,
+		     "file \"%s\" left but not entered", new_file);
+	  return;
+	}
+    }
   /* Compensate for the increment in linemap_add that occurs in
      _cpp_do_file_change.  We're currently at the start of the line
      *following* the #line directive.  A separate source_location for this
Index: libcpp/line-map.c
===================================================================
--- libcpp/line-map.c	(revision 234025)
+++ libcpp/line-map.c	(working copy)
@@ -514,43 +514,23 @@ linemap_add (struct line_maps *set, enum
 	 "included", this variable points the map in use right before the
 	 #include "included", inside the same "includer" file.  */
       line_map_ordinary *from;
-      bool error;
-
-      if (MAIN_FILE_P (map - 1))
-	{
-	  /* So this _should_ mean we are leaving the main file --
-	     effectively ending the compilation unit. But to_file not
-	     being NULL means the caller thinks we are leaving to
-	     another file. This is an erroneous behaviour but we'll
-	     try to recover from it. Let's pretend we are not leaving
-	     the main file.  */
-	  error = true;
-          reason = LC_RENAME;
-          from = map - 1;
-	}
-      else
-	{
-	  /* (MAP - 1) points to the map we are leaving. The
-	     map from which (MAP - 1) got included should be the map
-	     that comes right before MAP in the same file.  */
-	  from = INCLUDED_FROM (set, map - 1);
-	  error = to_file && filename_cmp (ORDINARY_MAP_FILE_NAME (from),
-					   to_file);
-	}
 
-      /* Depending upon whether we are handling preprocessed input or
-	 not, this can be a user error or an ICE.  */
-      if (error)
-	fprintf (stderr, "line-map.c: file \"%s\" left but not entered\n",
-		 to_file);
+      linemap_assert (!MAIN_FILE_P (map - 1));
+      /* (MAP - 1) points to the map we are leaving. The
+	 map from which (MAP - 1) got included should be the map
+	 that comes right before MAP in the same file.  */
+      from = INCLUDED_FROM (set, map - 1);
 
       /* A TO_FILE of NULL is special - we use the natural values.  */
-      if (error || to_file == NULL)
+      if (to_file == NULL)
 	{
 	  to_file = ORDINARY_MAP_FILE_NAME (from);
 	  to_line = SOURCE_LINE (from, from[1].start_location);
 	  sysp = ORDINARY_MAP_IN_SYSTEM_HEADER_P (from);
 	}
+      else
+	linemap_assert (filename_cmp (ORDINARY_MAP_FILE_NAME (from),
+				      to_file) == 0);
     }
 
   map->sysp = sysp;
Index: gcc/testsuite/gcc.dg/pr69650.c
===================================================================
--- gcc/testsuite/gcc.dg/pr69650.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr69650.c	(working copy)
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+/* { dg-options "-std=gnu99" } */
+
+# 9 "" 2 /* { dg-error "left but not entered" } */
+not_a_type a; /* { dg-error "unknown type" } */

Re: Fix 69650, bogus line numbers from libcpp

[David Malcolm]

On Thu, 2016-03-10 at 09:40 +0100, Bernd Schmidt wrote:
> This is a case where bogus #line directives can confuse libcpp into 
> producing nonsensical line numbers, even leading to a crash later on
> in LTO.
> 
> The following patch moves the test earlier to a point where we can
> more 
> easily recover from the error condition. I should note that I changed
> the raw fprintf (stderr) to a cpp_error call, which is a slight
> change 
> in behaviour (we don't even get to LTO anymore due to erroring out
> earlier).
> 
> Bootstrapped and tested on x86_64-linux (as always including Ada,
> which 
> failed with an earlier version of the patch). Ok?

Thanks for looking at this.

> --- libcpp/directives.c	(revision 234025)
> +++ libcpp/directives.c	(working copy)
> @@ -1046,6 +1046,19 @@ do_linemarker (cpp_reader *pfile)
>  
>    skip_rest_of_line (pfile);
>  
> +  if (reason == LC_LEAVE)
> +    {
> +      const line_map_ordinary *from;      
> +      if (MAIN_FILE_P (map)
> +	  || (new_file
> +	      && (from = INCLUDED_FROM (pfile->line_table, map)) !=
NULL
> +	      && filename_cmp (ORDINARY_MAP_FILE_NAME (from),
new_file) != 0))
> +	{
> +	  cpp_error (pfile, CPP_DL_ERROR,
> +		     "file \"%s\" left but not entered", new_file);
                                                         ^^^^^^^^
Although it looks like you're preserving the existing behavior from
when this was in linemap_add, shouldn't this be
  ORDINARY_MAP_FILE_NAME (from)
rather than new_file?  (i.e. shouldn't it report the name of the file
being *left*, rather than the one being entered?)

[...]

> Index: gcc/testsuite/gcc.dg/pr69650.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/pr69650.c	(revision 0)
> +++ gcc/testsuite/gcc.dg/pr69650.c	(working copy)
> @@ -0,0 +1,5 @@
> +/* { dg-do compile } */
> +/* { dg-options "-std=gnu99" } */
> +
> +# 9 "" 2 /* { dg-error "left but not entered" } */
> +not_a_type a; /* { dg-error "unknown type" } */

Can we also have a testcase with a non-empty filename?  I'm interested
in seeing what the exact error messages looks like.

Also, is it possible to construct a testcase that triggers
the logic in the non-MAIN_FILE_P clause? (perhaps with some
# directives for a variety of "files")?


Hope this is constructive
Dave

Re: Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

On 03/11/2016 11:09 PM, David Malcolm wrote:
>> +	  cpp_error (pfile, CPP_DL_ERROR,
>> +		     "file \"%s\" left but not entered", new_file);
>                                                           ^^^^^^^^
> Although it looks like you're preserving the existing behavior from
> when this was in linemap_add, shouldn't this be
>    ORDINARY_MAP_FILE_NAME (from)
> rather than new_file?  (i.e. shouldn't it report the name of the file
> being *left*, rather than the one being entered?)

Hmm, almost but not quite. We don't necessarily know the name of the 
file that's being left, if there's just a single #line directive as in 
the testcase. I don't think we can reliably get a meaningful filename 
other than the in the line directive. So maybe the error message needs 
to be changed to something like "file %s unexpectedly reentered"?

> Can we also have a testcase with a non-empty filename?  I'm interested
> in seeing what the exact error messages looks like.

   # 1 "v.c"
   # 1 "t.h" 1
   int t;
   # 2 "v.c" 2

   int b;

t.h:2:12: error: file "b.c" left but not entered

So this shows the line number for the file we think we are in, which is 
t.h. Would you accept this with the wording changed as suggested above?


Bernd

Re: Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

Ping.


Bernd

On 03/14/2016 02:20 PM, Bernd Schmidt wrote:
> On 03/11/2016 11:09 PM, David Malcolm wrote:
>>> +      cpp_error (pfile, CPP_DL_ERROR,
>>> +             "file \"%s\" left but not entered", new_file);
>>                                                           ^^^^^^^^
>> Although it looks like you're preserving the existing behavior from
>> when this was in linemap_add, shouldn't this be
>>    ORDINARY_MAP_FILE_NAME (from)
>> rather than new_file?  (i.e. shouldn't it report the name of the file
>> being *left*, rather than the one being entered?)
>
> Hmm, almost but not quite. We don't necessarily know the name of the
> file that's being left, if there's just a single #line directive as in
> the testcase. I don't think we can reliably get a meaningful filename
> other than the in the line directive. So maybe the error message needs
> to be changed to something like "file %s unexpectedly reentered"?
>
>> Can we also have a testcase with a non-empty filename?  I'm interested
>> in seeing what the exact error messages looks like.
>
>    # 1 "v.c"
>    # 1 "t.h" 1
>    int t;
>    # 2 "v.c" 2
>
>    int b;
>
> t.h:2:12: error: file "b.c" left but not entered
>
> So this shows the line number for the file we think we are in, which is
> t.h. Would you accept this with the wording changed as suggested above?
>
>
> Bernd
>

Re: Fix 69650, bogus line numbers from libcpp

[David Malcolm]

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

On Mon, 2016-03-14 at 14:20 +0100, Bernd Schmidt wrote:
> On 03/11/2016 11:09 PM, David Malcolm wrote:
> > > +	  cpp_error (pfile, CPP_DL_ERROR,
> > > +		     "file \"%s\" left but not entered",
> > > new_file);
> >                                                           ^^^^^^^^
> > Although it looks like you're preserving the existing behavior from
> > when this was in linemap_add, shouldn't this be
> >    ORDINARY_MAP_FILE_NAME (from)
> > rather than new_file?  (i.e. shouldn't it report the name of the
> > file
> > being *left*, rather than the one being entered?)

I realize now that I was wrong here: "new_file" refers to the filename
given in the linemarker directive, which, depending on the (optional)
flag could be a directive to enter or leave a linemap.

Sorry about that; you may want to disregard my earlier worries.


> Hmm, almost but not quite. We don't necessarily know the name of the
> file that's being left, if there's just a single #line directive as
> in 
> the testcase. I don't think we can reliably get a meaningful filename
> other than the in the line directive. So maybe the error message
> needs 
> to be changed to something like "file %s unexpectedly reentered"?

> > Can we also have a testcase with a non-empty filename?  I'm
> > interested
> > in seeing what the exact error messages looks like.
> 
>    # 1 "v.c"
>    # 1 "t.h" 1
>    int t;
>    # 2 "v.c" 2
> 
>    int b;
> 
> t.h:2:12: error: file "b.c" left but not entered

I was thinking of something like the attached, which makes things more
explicit; I felt the first dg-error in your patch was a little too
concise.

> So this shows the line number for the file we think we are in, which
> is 
> t.h. Would you accept this with the wording changed as suggested
> above?

I'm very familiar with the code for tracking ranges and issuing
diagnostics, but less familiar with other parts of libcpp, so I'm not
sure I'm fully qualified to approve the patch.  That said, the patch
looks sane, mostly...  The remaining thing I have a concern about
relates to the other question I had, which I don't think you addressed:
is it possible to construct a testcase that triggers the logic in the
non-MAIN_FILE_P clause? (perhaps with some # directives for a variety
of "files")?  In other words, please can we have branch coverage for
the branches in this code that the patch adds to do_linemarker:

      if (MAIN_FILE_P (map)
	  || (new_file
	      && (from = INCLUDED_FROM (pfile->line_table, map)) != NULL
	      && filename_cmp (ORDINARY_MAP_FILE_NAME (from), new_file) != 0))

Alternatively, if you're rushed, I can try to write these cases myself.

(How much existing test coverage do we have for line-markers?  I found
about 15 existing testcases that use them in a crude search with grep,
but these are all apparently only incidentally as part of testing other
functionality; is it worth me adding some more general test coverage
for this?)

Hope this is constructive
Dave

[-- Attachment #2: 0001-Add-name-to-test-case-for-PR-69650.patch --]
[-- Type: text/x-patch, Size: 752 bytes --]

From 68d9d811ce4c088e2b62caae5b9a95c9494ff939 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Mon, 21 Mar 2016 15:50:06 -0400
Subject: [PATCH] Add name to test case for PR 69650

---
 gcc/testsuite/gcc.dg/pr69650.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/testsuite/gcc.dg/pr69650.c b/gcc/testsuite/gcc.dg/pr69650.c
index e0abbd8..e77ed06 100644
--- a/gcc/testsuite/gcc.dg/pr69650.c
+++ b/gcc/testsuite/gcc.dg/pr69650.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
 /* { dg-options "-std=gnu99" } */
 
-# 9 "" 2 /* { dg-error "left but not entered" } */
+# 9 "unbalanced.c" 2 /* { dg-error "file .unbalanced.c. left but not entered" } */
 not_a_type a; /* { dg-error "unknown type" } */
-- 
1.8.5.3

Re: Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

On 03/21/2016 09:15 PM, David Malcolm wrote:
> On Mon, 2016-03-14 at 14:20 +0100, Bernd Schmidt wrote:
>> On 03/11/2016 11:09 PM, David Malcolm wrote:
>>>> +	  cpp_error (pfile, CPP_DL_ERROR,
>>>> +		     "file \"%s\" left but not entered",
>>>> new_file);
>>>                                                            ^^^^^^^^
>>> Although it looks like you're preserving the existing behavior from
>>> when this was in linemap_add, shouldn't this be
>>>     ORDINARY_MAP_FILE_NAME (from)
>>> rather than new_file?  (i.e. shouldn't it report the name of the
>>> file
>>> being *left*, rather than the one being entered?)
>
> I realize now that I was wrong here: "new_file" refers to the filename
> given in the linemarker directive, which, depending on the (optional)
> flag could be a directive to enter or leave a linemap.
>
> Sorry about that; you may want to disregard my earlier worries.

No, I think you were mostly on point: new_file is the one in the #line 
directive, which AFAICT is the file being reentered. so the wording is 
in fact misleading. Including a file called "t.h" from "v.c" produces 
this pattern:

# 1 "t.h" 1
int t;
# 2 "v.c" 2

> I was thinking of something like the attached, which makes things more
> explicit; I felt the first dg-error in your patch was a little too
> concise.

No problem, but I do think we want to change the wording of the message 
to something like "file %s unexpectedly reentered".

> I'm very familiar with the code for tracking ranges and issuing
> diagnostics, but less familiar with other parts of libcpp, so I'm not
> sure I'm fully qualified to approve the patch.  That said, the patch
> looks sane, mostly...  The remaining thing I have a concern about
> relates to the other question I had, which I don't think you addressed:
> is it possible to construct a testcase that triggers the logic in the
> non-MAIN_FILE_P clause?

Are you thinking of anything more complex than the following?

# 1 "v.c"
# 1 "t.h" 1
# 1 "t2.h" 1
int t;
# 2 "t.h" 2
# 2 "v.c" 2

Change any of the filenames of the "^#.*2$" lines and you'll see error 
messages. For example, changing "t.h" to "x.h" in the second to last line:

In file included from t.h:1:0,
                  from v.c:1:
t2.h:2:13: error: file "x.h" left but not entered
t2.h:3:12: error: file "v.c" left but not entered

Of course any such error is likely to have a large number of follow-on 
errors. Not sure how to avoid this given that the input file most likely 
has a completely messed up structure.

> (How much existing test coverage do we have for line-markers?  I found
> about 15 existing testcases that use them in a crude search with grep,
> but these are all apparently only incidentally as part of testing other
> functionality; is it worth me adding some more general test coverage
> for this?)

It might be worthwhile but I'm not planning to for this issue.


Bernd

Re: Fix 69650, bogus line numbers from libcpp

[Richard Biener]

On Mon, Mar 21, 2016 at 11:58 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 03/21/2016 09:15 PM, David Malcolm wrote:
>>
>> On Mon, 2016-03-14 at 14:20 +0100, Bernd Schmidt wrote:
>>>
>>> On 03/11/2016 11:09 PM, David Malcolm wrote:
>>>>>
>>>>> +         cpp_error (pfile, CPP_DL_ERROR,
>>>>> +                    "file \"%s\" left but not entered",
>>>>> new_file);
>>>>
>>>>                                                            ^^^^^^^^
>>>> Although it looks like you're preserving the existing behavior from
>>>> when this was in linemap_add, shouldn't this be
>>>>     ORDINARY_MAP_FILE_NAME (from)
>>>> rather than new_file?  (i.e. shouldn't it report the name of the
>>>> file
>>>> being *left*, rather than the one being entered?)
>>
>>
>> I realize now that I was wrong here: "new_file" refers to the filename
>> given in the linemarker directive, which, depending on the (optional)
>> flag could be a directive to enter or leave a linemap.
>>
>> Sorry about that; you may want to disregard my earlier worries.
>
>
> No, I think you were mostly on point: new_file is the one in the #line
> directive, which AFAICT is the file being reentered. so the wording is in
> fact misleading. Including a file called "t.h" from "v.c" produces this
> pattern:
>
> # 1 "t.h" 1
> int t;
> # 2 "v.c" 2
>
>> I was thinking of something like the attached, which makes things more
>> explicit; I felt the first dg-error in your patch was a little too
>> concise.
>
>
> No problem, but I do think we want to change the wording of the message to
> something like "file %s unexpectedly reentered".
>
>> I'm very familiar with the code for tracking ranges and issuing
>> diagnostics, but less familiar with other parts of libcpp, so I'm not
>> sure I'm fully qualified to approve the patch.  That said, the patch
>> looks sane, mostly...  The remaining thing I have a concern about
>> relates to the other question I had, which I don't think you addressed:
>> is it possible to construct a testcase that triggers the logic in the
>> non-MAIN_FILE_P clause?
>
>
> Are you thinking of anything more complex than the following?
>
> # 1 "v.c"
> # 1 "t.h" 1
> # 1 "t2.h" 1
> int t;
> # 2 "t.h" 2
> # 2 "v.c" 2
>
> Change any of the filenames of the "^#.*2$" lines and you'll see error
> messages. For example, changing "t.h" to "x.h" in the second to last line:
>
> In file included from t.h:1:0,
>                  from v.c:1:
> t2.h:2:13: error: file "x.h" left but not entered
> t2.h:3:12: error: file "v.c" left but not entered
>
> Of course any such error is likely to have a large number of follow-on
> errors. Not sure how to avoid this given that the input file most likely has
> a completely messed up structure.
>
>> (How much existing test coverage do we have for line-markers?  I found
>> about 15 existing testcases that use them in a crude search with grep,
>> but these are all apparently only incidentally as part of testing other
>> functionality; is it worth me adding some more general test coverage
>> for this?)
>
>
> It might be worthwhile but I'm not planning to for this issue.

Btw, the issue in the PR is also fixed with a simple

Index: libcpp/line-map.c
===================================================================
--- libcpp/line-map.c   (revision 234415)
+++ libcpp/line-map.c   (working copy)
@@ -543,7 +543,7 @@ linemap_add (struct line_maps *set, enum
                 to_file);

       /* A TO_FILE of NULL is special - we use the natural values.  */
-      if (error || to_file == NULL)
+      if (to_file == NULL)
        {
          to_file = ORDINARY_MAP_FILE_NAME (from);
          to_line = SOURCE_LINE (from, from[1].start_location);

I did some archeology and the revision that introduced the error || is
44789 which documented
that part as

        (add_line_map): Return pointer to const.  When passed NULL to_file
        with LC_LEAVE, use the obvious values for the return point so the
        caller doesn't have to figure them out.

where obviously the values used in the error case are not obvious.
Simply keeping the
info parsed from the line directive makes us happy here.

Bootstrap/test still running, I also have a LTO testcase for the crash.

Note this doesn't make the testcase error as I think your patch does
(which I think is
an improvement in itself but possibly would require some -fpermissive
handling as I
expect some old code generators to generate invalid line directives).

Richard.

>
> Bernd
>

Re: Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

On 03/23/2016 01:41 PM, Richard Biener wrote:
> Btw, the issue in the PR is also fixed with a simple
>
> Index: libcpp/line-map.c
> ===================================================================
> --- libcpp/line-map.c   (revision 234415)
> +++ libcpp/line-map.c   (working copy)
> @@ -543,7 +543,7 @@ linemap_add (struct line_maps *set, enum
>                   to_file);
>
>         /* A TO_FILE of NULL is special - we use the natural values.  */
> -      if (error || to_file == NULL)
> +      if (to_file == NULL)
>          {
>            to_file = ORDINARY_MAP_FILE_NAME (from);
>            to_line = SOURCE_LINE (from, from[1].start_location);

I looked at that, but that made it hard to add the testcase as the line 
numbers no longer match the dg-error directives. By moving this code we 
can ignore the erroneous #line directive, and for this one testcase at 
least, that makes the line numbers (and caret diagnostics etc.) come out 
right.


Bernd

Re: Fix 69650, bogus line numbers from libcpp

[Richard Biener]

On Wed, Mar 23, 2016 at 2:15 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 03/23/2016 01:41 PM, Richard Biener wrote:
>>
>> Btw, the issue in the PR is also fixed with a simple
>>
>> Index: libcpp/line-map.c
>> ===================================================================
>> --- libcpp/line-map.c   (revision 234415)
>> +++ libcpp/line-map.c   (working copy)
>> @@ -543,7 +543,7 @@ linemap_add (struct line_maps *set, enum
>>                   to_file);
>>
>>         /* A TO_FILE of NULL is special - we use the natural values.  */
>> -      if (error || to_file == NULL)
>> +      if (to_file == NULL)
>>          {
>>            to_file = ORDINARY_MAP_FILE_NAME (from);
>>            to_line = SOURCE_LINE (from, from[1].start_location);
>
>
> I looked at that, but that made it hard to add the testcase as the line
> numbers no longer match the dg-error directives. By moving this code we can
> ignore the erroneous #line directive, and for this one testcase at least,
> that makes the line numbers (and caret diagnostics etc.) come out right.

After some more digging and looking at your patch I'd approve that if it would
emit a warning rather than an error - so can you please adjust it?

Thanks,
Richard.

>
> Bernd

Re: Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

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



On 03/23/2016 03:21 PM, Richard Biener wrote:
> On Wed, Mar 23, 2016 at 2:15 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>> On 03/23/2016 01:41 PM, Richard Biener wrote:
>>>
>>> Btw, the issue in the PR is also fixed with a simple
>>>
>>> Index: libcpp/line-map.c
>>> ===================================================================
>>> --- libcpp/line-map.c   (revision 234415)
>>> +++ libcpp/line-map.c   (working copy)
>>> @@ -543,7 +543,7 @@ linemap_add (struct line_maps *set, enum
>>>                    to_file);
>>>
>>>          /* A TO_FILE of NULL is special - we use the natural values.  */
>>> -      if (error || to_file == NULL)
>>> +      if (to_file == NULL)
>>>           {
>>>             to_file = ORDINARY_MAP_FILE_NAME (from);
>>>             to_line = SOURCE_LINE (from, from[1].start_location);
>>
>>
>> I looked at that, but that made it hard to add the testcase as the line
>> numbers no longer match the dg-error directives. By moving this code we can
>> ignore the erroneous #line directive, and for this one testcase at least,
>> that makes the line numbers (and caret diagnostics etc.) come out right.
>
> After some more digging and looking at your patch I'd approve that if it would
> emit a warning rather than an error - so can you please adjust it?

Like this? No one has yet approved any better wording for the message, 
so given that you said "it's not a regression" I've left it, but I would 
now prefer "linemarker ignored due to incorrect nesting".


Bernd

[-- Attachment #2: cpp-leave.diff --]
[-- Type: text/x-patch, Size: 3463 bytes --]


	PR lto/69650
	* directives.c (do_linemarker): Test for file left but not entered
	here.
	* line-map.c (linemap_add): Not here.

	PR lto/69650
	* gcc.dg/pr69650.c: New test.

Index: gcc/testsuite/gcc.dg/pr69650.c
===================================================================
--- gcc/testsuite/gcc.dg/pr69650.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr69650.c	(working copy)
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+/* { dg-options "-std=gnu99" } */
+
+# 9 "somefile" 2 /* { dg-warning "left but not entered" } */
+not_a_type a; /* { dg-error "unknown type" } */
Index: libcpp/directives.c
===================================================================
--- libcpp/directives.c	(revision 234341)
+++ libcpp/directives.c	(working copy)
@@ -1046,6 +1046,19 @@
 
   skip_rest_of_line (pfile);
 
+  if (reason == LC_LEAVE)
+    {
+      const line_map_ordinary *from;      
+      if (MAIN_FILE_P (map)
+	  || (new_file
+	      && (from = INCLUDED_FROM (pfile->line_table, map)) != NULL
+	      && filename_cmp (ORDINARY_MAP_FILE_NAME (from), new_file) != 0))
+	{
+	  cpp_warning (pfile, CPP_W_NONE,
+		     "file \"%s\" left but not entered", new_file);
+	  return;
+	}
+    }
   /* Compensate for the increment in linemap_add that occurs in
      _cpp_do_file_change.  We're currently at the start of the line
      *following* the #line directive.  A separate source_location for this
Index: libcpp/line-map.c
===================================================================
--- libcpp/line-map.c	(revision 234341)
+++ libcpp/line-map.c	(working copy)
@@ -512,43 +512,23 @@
 	 "included", this variable points the map in use right before the
 	 #include "included", inside the same "includer" file.  */
       line_map_ordinary *from;
-      bool error;
 
-      if (MAIN_FILE_P (map - 1))
-	{
-	  /* So this _should_ mean we are leaving the main file --
-	     effectively ending the compilation unit. But to_file not
-	     being NULL means the caller thinks we are leaving to
-	     another file. This is an erroneous behaviour but we'll
-	     try to recover from it. Let's pretend we are not leaving
-	     the main file.  */
-	  error = true;
-          reason = LC_RENAME;
-          from = map - 1;
-	}
-      else
-	{
-	  /* (MAP - 1) points to the map we are leaving. The
-	     map from which (MAP - 1) got included should be the map
-	     that comes right before MAP in the same file.  */
-	  from = INCLUDED_FROM (set, map - 1);
-	  error = to_file && filename_cmp (ORDINARY_MAP_FILE_NAME (from),
-					   to_file);
-	}
+      linemap_assert (!MAIN_FILE_P (map - 1));
+      /* (MAP - 1) points to the map we are leaving. The
+	 map from which (MAP - 1) got included should be the map
+	 that comes right before MAP in the same file.  */
+      from = INCLUDED_FROM (set, map - 1);
 
-      /* Depending upon whether we are handling preprocessed input or
-	 not, this can be a user error or an ICE.  */
-      if (error)
-	fprintf (stderr, "line-map.c: file \"%s\" left but not entered\n",
-		 to_file);
-
       /* A TO_FILE of NULL is special - we use the natural values.  */
-      if (error || to_file == NULL)
+      if (to_file == NULL)
 	{
 	  to_file = ORDINARY_MAP_FILE_NAME (from);
 	  to_line = SOURCE_LINE (from, from[1].start_location);
 	  sysp = ORDINARY_MAP_IN_SYSTEM_HEADER_P (from);
 	}
+      else
+	linemap_assert (filename_cmp (ORDINARY_MAP_FILE_NAME (from),
+				      to_file) == 0);
     }
 
   map->sysp = sysp;

Re: Fix 69650, bogus line numbers from libcpp

[Jeff Law]

On 03/24/2016 09:20 AM, Bernd Schmidt wrote:
>
>
> On 03/23/2016 03:21 PM, Richard Biener wrote:
>> On Wed, Mar 23, 2016 at 2:15 PM, Bernd Schmidt <bschmidt@redhat.com>
>> wrote:
>>> On 03/23/2016 01:41 PM, Richard Biener wrote:
>>>>
>>>> Btw, the issue in the PR is also fixed with a simple
>>>>
>>>> Index: libcpp/line-map.c
>>>> ===================================================================
>>>> --- libcpp/line-map.c   (revision 234415)
>>>> +++ libcpp/line-map.c   (working copy)
>>>> @@ -543,7 +543,7 @@ linemap_add (struct line_maps *set, enum
>>>>                    to_file);
>>>>
>>>>          /* A TO_FILE of NULL is special - we use the natural
>>>> values.  */
>>>> -      if (error || to_file == NULL)
>>>> +      if (to_file == NULL)
>>>>           {
>>>>             to_file = ORDINARY_MAP_FILE_NAME (from);
>>>>             to_line = SOURCE_LINE (from, from[1].start_location);
>>>
>>>
>>> I looked at that, but that made it hard to add the testcase as the line
>>> numbers no longer match the dg-error directives. By moving this code
>>> we can
>>> ignore the erroneous #line directive, and for this one testcase at
>>> least,
>>> that makes the line numbers (and caret diagnostics etc.) come out right.
>>
>> After some more digging and looking at your patch I'd approve that if
>> it would
>> emit a warning rather than an error - so can you please adjust it?
>
> Like this? No one has yet approved any better wording for the message,
> so given that you said "it's not a regression" I've left it, but I would
> now prefer "linemarker ignored due to incorrect nesting".
>
>
> Bernd
>
> cpp-leave.diff
>
>
> 	PR lto/69650
> 	* directives.c (do_linemarker): Test for file left but not entered
> 	here.
> 	* line-map.c (linemap_add): Not here.
>
> 	PR lto/69650
> 	* gcc.dg/pr69650.c: New test.
OK.

Also OK if you want to fixup the message.

jeff

Re: Fix 69650, bogus line numbers from libcpp

[Jeff Law]

On 03/24/2016 03:25 PM, Jeff Law wrote:
> On 03/24/2016 09:20 AM, Bernd Schmidt wrote:
>>
>> Like this? No one has yet approved any better wording for the message,
>> so given that you said "it's not a regression" I've left it, but I would
>> now prefer "linemarker ignored due to incorrect nesting".
>>
>>
>> Bernd
>>
>> cpp-leave.diff
>>
>>
>>     PR lto/69650
>>     * directives.c (do_linemarker): Test for file left but not entered
>>     here.
>>     * line-map.c (linemap_add): Not here.
>>
>>     PR lto/69650
>>     * gcc.dg/pr69650.c: New test.
> OK.
>
> Also OK if you want to fixup the message.
And I just realized Bernd is out due to the holidays, so I went ahead 
and committed this for him.

jeff