Re: [PATCH, libcpp] Do not decrease highest_location if the included file has be included twice.

[Dehao Chen <dehao at google dot com>]

On Mon, Jun 3, 2013 at 11:46 AM, Dodji Seketeli <dodji@seketeli.org> wrote:
> Dehao Chen <dehao@google.com> a écrit:
>
>> Thanks for looking into this.
>
> You are welcome.  Libcpp is fun.  Is it not?  :-)

It truly is ;-)

>
>> The reason a test is missing is that it would need a super large
>> source file to reproduce the problem.
>
> I see.  It's kind of a pity that we cannot have tests for interesting
> cases like this, though.  I am wondering if with some #line tricks (like
> setting #line <a super large number> we couldn't simulate that.  I
> haven't tried myself though.

The problem is it's not about the line number, but number of lines. We
need to have no less than 10M lines in one module in order for this
problem to show up. (and the parsing itself takes several minutes on a
powerful machine)

>
>> However, if you apply the attached patch, you can reproduce the
>> problem with attached testcase:
>>
>> g++ a.cpp -g -S -c -o a.s
>>
>> in a.s, the linenos are off-by-one.
>
> Thanks.
>
>> The root cause is that highest_location-- should not happen when the
>> header file is not gonna be read. In should_stack file, there is
>> following check:
>>
>>   /* Skip if the file had a header guard and the macro is defined.
>>      PCH relies on this appearing before the PCH handler below.  */
>>   if (file->cmacro && file->cmacro->type == NT_MACRO)
>>     return false;
>>
>> Thus we should add it back to _cpp_stack_include too.
>
> Yeah, I figured that out.  What I didn't get is how the column number
> disabling thing was interacting with this ....
>
>> The problem was hidden when column number is used because
>> highest_location is updated in linemap_position_for_column. However,
>> when linemap are too large, it disables columns and do not update the
>> highest_location.
>
> Gotcha.  Thank you for the insight.
>
> So, I'd say that in this hunk of your patch:
>
>> @@ -1002,7 +1002,8 @@ _cpp_stack_include (cpp_reader *pfile, const char
>>       linemap_add is not called) or we were included from the
>>       command-line.  */
>>    if (file->pchname == NULL && file->err_no == 0
>> -      && type != IT_CMDLINE && type != IT_DEFAULT)
>> +      && type != IT_CMDLINE && type != IT_DEFAULT
>> +      && !(file->cmacro && file->cmacro->type == NT_MACRO))
>
> Maybe you should test:
>
>     && should_stack_file (pfile, file, type == IT_IMPORT)
>
> rather than testing the last conjunction you added?  This is because
> there are many conditions that could make the header to not be loaded,
> besides the one you are testing.  Would this work in your environment?

I tried to apply this change, but it failed several PCH related
regression tests. I still haven't look into why because those tests
are hard to reproduce...

Dehao

>
> If that works, maybe add a comment to :
>
> /* Compensate for the increment in linemap_add that occurs in
>      _cpp_stack_file. ... */
>
> Saying that the compensation should happen when _cpp_stack_file really
> stacks the file, that is, when should_stack_file returns true; this does
> not seem obvious.  At least not to me.  :-)
>
> Cheers.
>
> --
>                 Dodji