On 12 June 2014 01:23, Peter Bergner <berg...@vnet.ibm.com> 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.