Patchwork [libcpp] Do not decrease highest_location if the included file has be included twice.

login
register
mail settings
Submitter Dehao Chen
Date June 1, 2013, 2:55 p.m.
Message ID <CAO2gOZVy0TO4n4BZ6aCiOuO1ADkhMCac0j2GOqQz4GH6vSX51A@mail.gmail.com>
Download mbox | patch
Permalink /patch/248081/
State New
Headers show

Comments

Dehao Chen - June 1, 2013, 2:55 p.m.
This patch fixes the bug that when include a header file, if the
header file is already included (with #define _HEADER_H_), libcpp
should not decrease its highest_location, otherwise it'll cause
incorrect source location when source location numbers are large
enough to omit columns.

Passed regression test.

OK for trunk?

Thanks,
Dehao

libcpp/ChangeLog:

2013-05-31  Dehao Chen  <dehao@google.com>

        * files.c (_cpp_stack_include): Fix the highest_location when header
        file is guarded by #ifndef and is included twice.
Dodji Seketeli - June 3, 2013, 2:27 p.m.
Hello Dehao,

Dehao Chen <dehao@google.com> a écrit:

> This patch fixes the bug that when include a header file, if the
> header file is already included (with #define _HEADER_H_), libcpp
> should not decrease its highest_location, otherwise it'll cause
> incorrect source location when source location numbers are large
> enough to omit columns.

Just for my education, could you please explain why decreasing
pfile->line_table->highest_location here would incur incorrect source
location *when* location numbers are so large that we emit column
numbers?  As the patch lacks a test case, I cannot e.g, run this in a
debugger to understand precisely what you mean.  Thus it'd be helpful
for me to understand what code spots are involved exactly under the
conditions you are seeing.

I am CC-ing Tom for this as well.

Thanks.

[...]

> 2013-05-31  Dehao Chen  <dehao@google.com>
>
>         * files.c (_cpp_stack_include): Fix the highest_location when header
>         file is guarded by #ifndef and is included twice.
>
>
> Index: libcpp/files.c
> ===================================================================
> --- libcpp/files.c (revision 199416)
> +++ libcpp/files.c (working copy)
> @@ -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))
>      pfile->line_table->highest_location--;
>
>    return _cpp_stack_file (pfile, file, type == IT_IMPORT);
Dodji Seketeli - June 3, 2013, 6:46 p.m.
Dehao Chen <dehao@google.com> a écrit:

> Thanks for looking into this.

You are welcome.  Libcpp is fun.  Is it not?  :-)

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

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

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.
Dehao Chen - June 3, 2013, 11:24 p.m.
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

Patch

Index: libcpp/files.c
===================================================================
--- libcpp/files.c (revision 199416)
+++ libcpp/files.c (working copy)
@@ -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))
     pfile->line_table->highest_location--;

   return _cpp_stack_file (pfile, file, type == IT_IMPORT);