On 12 June 2014 01:23, Peter Bergner <[email protected]> wrote:
> On Wed, 2014-06-11 at 23:07 +0000, Joseph S. Myers wrote:
>> On Wed, 11 Jun 2014, Peter Bergner wrote:
>>
>> > I'd like to ping the following patch that fixes PR57653. This did
>> > bootstrap and regtest with no regressions on powerpc64-linux.
>> >
>> > https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01571.html
>> >
>> > Is this ok for trunk, 4.9 and 4.8?
>>
>> I think the code change is correct, but the comment added needs expanding
>> to explain better what's going on (i.e. the circumstances in which the
>> condition include_cursor > deferred_count may hold, and why, in those
>> circumstances, returning early is the correct thing to do).
>
> Manuel, can you offer an updated comment? Being just the patch
> tester and not knowing this code at all, I'm not going to be of
> much use at expanding the Manuel's original comment.
It has been a long time, and it seems that even at the time I proposed
the patch, I had no idea why it worked:
"For some reason unknown to me, push_commandline_include should not be
called while processing -imacros. -imacros tries to achieve this by
playing tricks with include_cursor, but that doesn't stop the
pre-included files. Calling cpp_push_include (or
cpp_push_default_include) seems to mess up everything (again, no idea
why!)."
The long explanation is -imacro triggers:
/* Handle -imacros after -D and -U. */
for (i = 0; i < deferred_count; i++)
{
struct deferred_opt *opt = &deferred_opts[i];
if (opt->code == OPT_imacros
&& cpp_push_include (parse_in, opt->arg))
{
/* Disable push_command_line_include callback for now. */
include_cursor = deferred_count + 1;
cpp_scan_nooutput (parse_in);
}
}
Then push_command_line_include is roughly as follows:
/* Give CPP the next file given by -include, if any. */
static void
push_command_line_include (void)
{
if (!done_preinclude)
{
done_preinclude = true;
if (flag_hosted && std_inc && !cpp_opts->preprocessed)
{
const char *preinc = targetcm.c_preinclude ();
if (preinc && cpp_push_default_include (parse_in, preinc))
return;
}
}
pch_cpp_save_state ();
while (include_cursor < deferred_count)
{
[...]
}
if (include_cursor == deferred_count)
{
[...]
}
}
that is, when -imacros is given, push_command_line_include still calls
the cpp_push_default_include, which messes up everything. Why or how,
I have no idea. Someone else will need to investigate more.
I don't think the patch is the best possible approach. I think it
would be better to push the default includes as soon as it is
reasonably possible (before/after processing -imacros?), instead of
relying on push_command_line_include. Also, if
push_command_line_include needs to be disabled for -imacros, it would
be clearer to have a file-local boolean for this purpose.
The way push_command_line_include is called is a mystery to me:
if (new_map == 0 || (new_map->reason == LC_LEAVE && MAIN_FILE_P (new_map)))
{
pch_cpp_save_state ();
push_command_line_include ();
}
Why is it called when leaving the main file at cb_file_change?
Also, c_finish_options has at the end:
include_cursor = 0;
push_command_line_include ();
but shouldn't this happen instead when (or before) the main file is entered?
Cheers,
Manuel.
