Dehao Chen <de...@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.
--
Dodji
