Re: Fix 69650, bogus line numbers from libcpp

[David Malcolm <dmalcolm@redhat.com>]

[-- 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