Fix 69650, bogus line numbers from libcpp

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


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
>

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

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