| Message ID | 56E132FC.2030404@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 2016-03-10 at 09:40 +0100, Bernd Schmidt wrote: > This is a case where bogus #line directives can confuse libcpp into > producing nonsensical line numbers, even leading to a crash later on > in LTO. > > The following patch moves the test earlier to a point where we can > more > easily recover from the error condition. I should note that I changed > the raw fprintf (stderr) to a cpp_error call, which is a slight > change > in behaviour (we don't even get to LTO anymore due to erroring out > earlier). > > Bootstrapped and tested on x86_64-linux (as always including Ada, > which > failed with an earlier version of the patch). Ok? Thanks for looking at this. > --- libcpp/directives.c (revision 234025) > +++ libcpp/directives.c (working copy) > @@ -1046,6 +1046,19 @@ do_linemarker (cpp_reader *pfile) > > skip_rest_of_line (pfile); > > + if (reason == LC_LEAVE) > + { > + const line_map_ordinary *from; > + if (MAIN_FILE_P (map) > + || (new_file > + && (from = INCLUDED_FROM (pfile->line_table, map)) != NULL > + && filename_cmp (ORDINARY_MAP_FILE_NAME (from), new_file) != 0)) > + { > + cpp_error (pfile, CPP_DL_ERROR, > + "file \"%s\" left but not entered", new_file); ^^^^^^^^ Although it looks like you're preserving the existing behavior from when this was in linemap_add, shouldn't this be ORDINARY_MAP_FILE_NAME (from) rather than new_file? (i.e. shouldn't it report the name of the file being *left*, rather than the one being entered?) [...] > Index: gcc/testsuite/gcc.dg/pr69650.c > =================================================================== > --- gcc/testsuite/gcc.dg/pr69650.c (revision 0) > +++ gcc/testsuite/gcc.dg/pr69650.c (working copy) > @@ -0,0 +1,5 @@ > +/* { dg-do compile } */ > +/* { dg-options "-std=gnu99" } */ > + > +# 9 "" 2 /* { dg-error "left but not entered" } */ > +not_a_type a; /* { dg-error "unknown type" } */ Can we also have a testcase with a non-empty filename? I'm interested in seeing what the exact error messages looks like. Also, is it possible to construct a testcase that triggers the logic in the non-MAIN_FILE_P clause? (perhaps with some # directives for a variety of "files")? Hope this is constructive Dave
On 03/11/2016 11:09 PM, David Malcolm wrote: >> + cpp_error (pfile, CPP_DL_ERROR, >> + "file \"%s\" left but not entered", new_file); > ^^^^^^^^ > Although it looks like you're preserving the existing behavior from > when this was in linemap_add, shouldn't this be > ORDINARY_MAP_FILE_NAME (from) > rather than new_file? (i.e. shouldn't it report the name of the file > being *left*, rather than the one being entered?) Hmm, almost but not quite. We don't necessarily know the name of the file that's being left, if there's just a single #line directive as in the testcase. I don't think we can reliably get a meaningful filename other than the in the line directive. So maybe the error message needs to be changed to something like "file %s unexpectedly reentered"? > Can we also have a testcase with a non-empty filename? I'm interested > in seeing what the exact error messages looks like. # 1 "v.c" # 1 "t.h" 1 int t; # 2 "v.c" 2 int b; t.h:2:12: error: file "b.c" left but not entered So this shows the line number for the file we think we are in, which is t.h. Would you accept this with the wording changed as suggested above? Bernd
Ping. Bernd On 03/14/2016 02:20 PM, Bernd Schmidt wrote: > On 03/11/2016 11:09 PM, David Malcolm wrote: >>> + cpp_error (pfile, CPP_DL_ERROR, >>> + "file \"%s\" left but not entered", new_file); >> ^^^^^^^^ >> Although it looks like you're preserving the existing behavior from >> when this was in linemap_add, shouldn't this be >> ORDINARY_MAP_FILE_NAME (from) >> rather than new_file? (i.e. shouldn't it report the name of the file >> being *left*, rather than the one being entered?) > > Hmm, almost but not quite. We don't necessarily know the name of the > file that's being left, if there's just a single #line directive as in > the testcase. I don't think we can reliably get a meaningful filename > other than the in the line directive. So maybe the error message needs > to be changed to something like "file %s unexpectedly reentered"? > >> Can we also have a testcase with a non-empty filename? I'm interested >> in seeing what the exact error messages looks like. > > # 1 "v.c" > # 1 "t.h" 1 > int t; > # 2 "v.c" 2 > > int b; > > t.h:2:12: error: file "b.c" left but not entered > > So this shows the line number for the file we think we are in, which is > t.h. Would you accept this with the wording changed as suggested above? > > > Bernd >
PR lto/69650 * directives.c (do_linemarker): Test for file left but not entered here. * line-map.c (linemap_add): Not here. PR lto/69650 * gcc.dg/pr69650.c: New test. Index: libcpp/directives.c =================================================================== --- libcpp/directives.c (revision 234025) +++ libcpp/directives.c (working copy) @@ -1046,6 +1046,19 @@ do_linemarker (cpp_reader *pfile) skip_rest_of_line (pfile); + if (reason == LC_LEAVE) + { + const line_map_ordinary *from; + if (MAIN_FILE_P (map) + || (new_file + && (from = INCLUDED_FROM (pfile->line_table, map)) != NULL + && filename_cmp (ORDINARY_MAP_FILE_NAME (from), new_file) != 0)) + { + cpp_error (pfile, CPP_DL_ERROR, + "file \"%s\" left but not entered", new_file); + return; + } + } /* Compensate for the increment in linemap_add that occurs in _cpp_do_file_change. We're currently at the start of the line *following* the #line directive. A separate source_location for this Index: libcpp/line-map.c =================================================================== --- libcpp/line-map.c (revision 234025) +++ libcpp/line-map.c (working copy) @@ -514,43 +514,23 @@ linemap_add (struct line_maps *set, enum "included", this variable points the map in use right before the #include "included", inside the same "includer" file. */ line_map_ordinary *from; - bool error; - - if (MAIN_FILE_P (map - 1)) - { - /* So this _should_ mean we are leaving the main file -- - effectively ending the compilation unit. But to_file not - being NULL means the caller thinks we are leaving to - another file. This is an erroneous behaviour but we'll - try to recover from it. Let's pretend we are not leaving - the main file. */ - error = true; - reason = LC_RENAME; - from = map - 1; - } - else - { - /* (MAP - 1) points to the map we are leaving. The - map from which (MAP - 1) got included should be the map - that comes right before MAP in the same file. */ - from = INCLUDED_FROM (set, map - 1); - error = to_file && filename_cmp (ORDINARY_MAP_FILE_NAME (from), - to_file); - } - /* Depending upon whether we are handling preprocessed input or - not, this can be a user error or an ICE. */ - if (error) - fprintf (stderr, "line-map.c: file \"%s\" left but not entered\n", - to_file); + linemap_assert (!MAIN_FILE_P (map - 1)); + /* (MAP - 1) points to the map we are leaving. The + map from which (MAP - 1) got included should be the map + that comes right before MAP in the same file. */ + from = INCLUDED_FROM (set, map - 1); /* A TO_FILE of NULL is special - we use the natural values. */ - if (error || to_file == NULL) + if (to_file == NULL) { to_file = ORDINARY_MAP_FILE_NAME (from); to_line = SOURCE_LINE (from, from[1].start_location); sysp = ORDINARY_MAP_IN_SYSTEM_HEADER_P (from); } + else + linemap_assert (filename_cmp (ORDINARY_MAP_FILE_NAME (from), + to_file) == 0); } map->sysp = sysp; Index: gcc/testsuite/gcc.dg/pr69650.c =================================================================== --- gcc/testsuite/gcc.dg/pr69650.c (revision 0) +++ gcc/testsuite/gcc.dg/pr69650.c (working copy) @@ -0,0 +1,5 @@ +/* { dg-do compile } */ +/* { dg-options "-std=gnu99" } */ + +# 9 "" 2 /* { dg-error "left but not entered" } */ +not_a_type a; /* { dg-error "unknown type" } */