| Message ID | CAO2gOZVy0TO4n4BZ6aCiOuO1ADkhMCac0j2GOqQz4GH6vSX51A@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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);
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.
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
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);
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.