Message ID | CAESRpQD7gLG9x3BQ7gysKNkjwcATEuNYhZExiHkt8yyXOtu+BA@mail.gmail.com |
---|---|
State | New |
Headers | show |
Manuel López-Ibáñez wrote: > This patch adds handling of Fortran %C using the common diagnostics > machinery. This is achieved by dynamically generating a location given > a location and an offset. This only works for non-macro line-maps (for > now), but this is OK since Fortran does not have virtual locations > (and I'm afraid it won't have them in the foreseeable future). > > Dodji, are the linemap_asserts() appropriate? I tried to follow your > previous comments whenever possible. > Boot®tested on x86_64-linux-gnu. > OK? From my side, the patch is OK. Thanks again for your diagnostic work (for this patch set and in general)! Tobias > libcpp/ChangeLog: > > 2014-10-16 Manuel López-Ibáñez <manu@gcc.gnu.org> > > PR fortran/44054 > * include/line-map.h (linemap_position_for_loc_and_offset): > Declare. > * line-map.c (linemap_position_for_loc_and_offset): New. > > > gcc/fortran/ChangeLog: > > 2014-10-16 Manuel López-Ibáñez <manu@gcc.gnu.org> > > PR fortran/44054 > * gfortran.h (warn_use_without_only): Remove. > (gfc_diagnostics_finish): Declare. > * error.c: Include tree-diagnostics.h > (gfc_format_decoder): New. > (gfc_diagnostics_init): Use gfc_format_decoder. Set default caret > char. > (gfc_diagnostics_finish): Restore tree diagnostics defaults, but > keep gfc_diagnostics_starter and finalizer. Restore default caret. > * options.c: Remove all uses of warn_use_without_only. > * lang.opt (Wuse-without-only): Add Var. > * module.c (gfc_use_module): Use gfc_warning_now_2. > * f95-lang.c (gfc_be_parse_file): Call gfc_diagnostics_finish. > > gcc/testsuite/ChangeLog: > > 2014-10-16 Manuel López-Ibáñez <manu@gcc.gnu.org> > > PR fortran/44054 > * lib/gfortran-dg.exp: Update regexp to match locus and message > without caret. > * gfortran.dg/use_without_only_1.f90: Add column numbers.
Hello Manuel, Manuel López-Ibáñez <lopezibanez@gmail.com> writes: > Dodji, are the linemap_asserts() appropriate? Yes they are. I have some additional comments though. > libcpp/ChangeLog: > > 2014-10-16 Manuel López-Ibáñez <manu@gcc.gnu.org> > > PR fortran/44054 > * include/line-map.h (linemap_position_for_loc_and_offset): > Declare. > * line-map.c (linemap_position_for_loc_and_offset): New. [...] > --- libcpp/include/line-map.h (revision 216257) > +++ libcpp/include/line-map.h (working copy) > @@ -601,10 +601,17 @@ linemap_position_for_column (struct line > column. */ > source_location > linemap_position_for_line_and_column (const struct line_map *, > linenum_type, unsigned int); > > +/* Encode and return a source_location starting from location LOC > + and shifting it by OFFSET columns. */ > +source_location > +linemap_position_for_loc_and_offset (struct line_maps *set, > + source_location loc, > + unsigned int offset); > + OK. [...] > --- libcpp/line-map.c (revision 216257) > +++ libcpp/line-map.c (working copy) [...] > +/* Encode and return a source_location starting from location LOC > + and shifting it by OFFSET columns. */ > + The comment is OK. I would just add that this function currently only works with non-virtual locations. > +source_location > +linemap_position_for_loc_and_offset (struct line_maps *set, > + source_location loc, > + unsigned int offset) > +{ > + const struct line_map * map = NULL; > + > + /* This function does not support virtual locations yet. */ > + linemap_assert (!linemap_location_from_macro_expansion_p (set, loc)); > + > + if (offset == 0) > + return loc; Here, I'd replace the above condition and return status statement with: if (offset == 0 /* Adding an offset to a reserved location (like UNKNOWN_LOCATION for the C/C++ FEs) does not really make sense. So let's live the location intact in that case. */ || loc < RESERVED_LOCATION) return loc; > + > + /* First, we find the real location and shift it. */ > + loc = linemap_resolve_location (set, loc, LRK_SPELLING_LOCATION, &map); > + linemap_assert (MAP_START_LOCATION (map) < loc + offset); OK. First I'd add a comment above the assert that says: /* The new location (loc + offset) should be higher than the first location encoded by MAP. */ and I'd add another assert: /* If MAP is not the last line map of its set, then the new location (loc + offset) should be less than the first location encoded by the next line map of the set. */ if (map < LINEMAPS_LAST_ORDINARY_MAP(set)) linemap_assert(MAP_START_LOCATION(&map[1]) < loc + offset); > + > + offset += SOURCE_COLUMN (map, loc); > + linemap_assert (offset < (1u << map->d.ordinary.column_bits)); > + > + source_location r = > + linemap_position_for_line_and_column (map, > + SOURCE_LINE (map, loc), > + offset); > + linemap_assert (map == linemap_lookup (set, r)); > + return r; > +} > + OK. So the line map part of the patch is OK from me if it passes bootstrap with the added asserts. Thank you for looking into this. Cheers.
Sorry, I forgot to make it clear that I have no power to approve the line-map changes. I just gave my casual point view; so please take it for what it is worth. I am CC-ing Tom and Jason who can approve this. Cheers. Dodji Seketeli <dodji@redhat.com> writes: > Hello Manuel, > > Manuel López-Ibáñez <lopezibanez@gmail.com> writes: > > >> Dodji, are the linemap_asserts() appropriate? > > Yes they are. I have some additional comments though. > >> libcpp/ChangeLog: >> >> 2014-10-16 Manuel López-Ibáñez <manu@gcc.gnu.org> >> >> PR fortran/44054 >> * include/line-map.h (linemap_position_for_loc_and_offset): >> Declare. >> * line-map.c (linemap_position_for_loc_and_offset): New. > [...] > >> --- libcpp/include/line-map.h (revision 216257) >> +++ libcpp/include/line-map.h (working copy) >> @@ -601,10 +601,17 @@ linemap_position_for_column (struct line >> column. */ >> source_location >> linemap_position_for_line_and_column (const struct line_map *, >> linenum_type, unsigned int); >> >> +/* Encode and return a source_location starting from location LOC >> + and shifting it by OFFSET columns. */ >> +source_location >> +linemap_position_for_loc_and_offset (struct line_maps *set, >> + source_location loc, >> + unsigned int offset); >> + > > OK. > > [...] > >> --- libcpp/line-map.c (revision 216257) >> +++ libcpp/line-map.c (working copy) > > [...] > >> +/* Encode and return a source_location starting from location LOC >> + and shifting it by OFFSET columns. */ >> + > > The comment is OK. I would just add that this function currently only > works with non-virtual locations. > >> +source_location >> +linemap_position_for_loc_and_offset (struct line_maps *set, >> + source_location loc, >> + unsigned int offset) >> +{ >> + const struct line_map * map = NULL; >> + >> + /* This function does not support virtual locations yet. */ >> + linemap_assert (!linemap_location_from_macro_expansion_p (set, loc)); >> + >> + if (offset == 0) >> + return loc; > > Here, I'd replace the above condition and return status statement with: > > if (offset == 0 > /* Adding an offset to a reserved location (like > UNKNOWN_LOCATION for the C/C++ FEs) does not really make > sense. So let's live the location intact in that case. */ > || loc < RESERVED_LOCATION) > return loc; > >> + >> + /* First, we find the real location and shift it. */ >> + loc = linemap_resolve_location (set, loc, LRK_SPELLING_LOCATION, &map); >> + linemap_assert (MAP_START_LOCATION (map) < loc + offset); > > OK. > > First I'd add a comment above the assert that says: > > /* The new location (loc + offset) should be higher than the first > location encoded by MAP. */ > > and I'd add another assert: > > /* If MAP is not the last line map of its set, then the new location > (loc + offset) should be less than the first location encoded by > the next line map of the set. */ > if (map < LINEMAPS_LAST_ORDINARY_MAP(set)) > linemap_assert(MAP_START_LOCATION(&map[1]) < loc + offset); > >> + >> + offset += SOURCE_COLUMN (map, loc); >> + linemap_assert (offset < (1u << map->d.ordinary.column_bits)); >> + >> + source_location r = >> + linemap_position_for_line_and_column (map, >> + SOURCE_LINE (map, loc), >> + offset); >> + linemap_assert (map == linemap_lookup (set, r)); >> + return r; >> +} >> + > > OK. > > So the line map part of the patch is OK from me if it passes bootstrap > with the added asserts. > > Thank you for looking into this. > > Cheers.
Hi Manuel, Dodji Seketeli wrote: > Sorry, I forgot to make it clear that I have no power to approve the > line-map changes. I just gave my casual point view; so please take it > for what it is worth. > > I am CC-ing Tom and Jason who can approve this. And I have a minor nit, which I forgot in my original review - even though I had spotted it before: In gfc_format_decoder, you are mixing space and tab indention. Tobias
On 23 October 2014 21:10, Dodji Seketeli <dodji@redhat.com> wrote: > Sorry, I forgot to make it clear that I have no power to approve the > line-map changes. I just gave my casual point view; so please take it > for what it is worth. > > I am CC-ing Tom and Jason who can approve this. I don't see the CCs in the copy I received. Please, Tom and Jason, could you nominate Dodji as line-map.[ch] and input.[ch] maintainer? He is surely the person who knows more about it right now. Or simply state that line-map.[ch] and input.[ch] fall within the diagnostic machinery. Cheers, Manuel.
Hi Dodji, On 23 October 2014 at 13:11, Dodji Seketeli <dodji@redhat.com> wrote: > /* If MAP is not the last line map of its set, then the new location > (loc + offset) should be less than the first location encoded by > the next line map of the set. */ which is currently implemented as: if (map != LINEMAPS_LAST_ORDINARY_MAP (set)) linemap_assert (loc + offset < MAP_START_LOCATION (&map[1])); However, this can easily trigger in Fortran, for example, when adding one line adds a new map, the location of the new map is 1 + set->highest_location. If the highest_location corresponds to the start of the previous line (because Fortran does not actually track columns yet with line-maps) then any offset will trigger this. One solution could be to make the new map's start location = set->highest_location + set->max_column_hint, but I'm not sure if there is a more conservative approach. I'm going to propose a patch that changes the asserts to assert only if checking otherwise return loc unchanged. That seems safer than giving a wildly inaccurate location. Cheers, Manuel.
diff -u gcc/fortran/gfortran.h gcc/fortran/gfortran.h --- gcc/fortran/gfortran.h (working copy) +++ gcc/fortran/gfortran.h (working copy) @@ -2455,7 +2455,6 @@ int warn_tabs; int warn_underflow; int warn_intrinsic_shadow; - int warn_use_without_only; int warn_intrinsics_std; int warn_character_truncation; int warn_array_temp; @@ -2691,7 +2690,8 @@ } gfc_error_buf; void gfc_error_init_1 (void); -void gfc_diagnostics_init(void); +void gfc_diagnostics_init (void); +void gfc_diagnostics_finish (void); void gfc_buffer_error (int); const char *gfc_print_wide_char (gfc_char_t); diff -u gcc/fortran/error.c gcc/fortran/error.c --- gcc/fortran/error.c (working copy) +++ gcc/fortran/error.c (working copy) @@ -40,6 +40,7 @@ #include "diagnostic.h" #include "diagnostic-color.h" +#include "tree-diagnostic.h" /* tree_diagnostics_defaults */ static int suppress_errors = 0; @@ -958,6 +959,38 @@ buffer_flag = i; } +/* Called from output_format -- during diagnostic message processing -- + to handle Fortran specific format specifiers with the following meanings: + + %C Current locus (no argument) +*/ +static bool +gfc_format_decoder (pretty_printer *pp, + text_info *text, const char *spec, + int precision ATTRIBUTE_UNUSED, bool wide ATTRIBUTE_UNUSED, + bool plus ATTRIBUTE_UNUSED, bool hash ATTRIBUTE_UNUSED) +{ + switch (*spec) + { + case 'C': + { + static const char *result = "(1)"; + gcc_assert (gfc_current_locus.nextc - gfc_current_locus.lb->line >= 0); + unsigned int c1 = gfc_current_locus.nextc - gfc_current_locus.lb->line; + gcc_assert (text->locus); + *text->locus + = linemap_position_for_loc_and_offset (line_table, + gfc_current_locus.lb->location, + c1); + global_dc->caret_char = '1'; + pp_string (pp, result); + return true; + } + default: + return false; + } +} + /* Return a malloc'd string describing a location. The caller is responsible for freeing the memory. */ static char * @@ -1357,4 +1390,16 @@ diagnostic_starter (global_dc) = gfc_diagnostic_starter; diagnostic_finalizer (global_dc) = gfc_diagnostic_finalizer; + diagnostic_format_decoder (global_dc) = gfc_format_decoder; + global_dc->caret_char = '^'; +} + +void +gfc_diagnostics_finish (void) +{ + tree_diagnostics_defaults (global_dc); + /* We still want to use the gfc starter and finalizer, not the tree + defaults. */ + diagnostic_starter (global_dc) = gfc_diagnostic_starter; + diagnostic_finalizer (global_dc) = gfc_diagnostic_finalizer; global_dc->caret_char = '^'; } diff -u gcc/fortran/options.c gcc/fortran/options.c --- gcc/fortran/options.c (working copy) +++ gcc/fortran/options.c (working copy) @@ -107,7 +107,6 @@ gfc_option.warn_tabs = 1; gfc_option.warn_underflow = 1; gfc_option.warn_intrinsic_shadow = 0; - gfc_option.warn_use_without_only = 0; gfc_option.warn_intrinsics_std = 0; gfc_option.warn_align_commons = 1; gfc_option.warn_real_q_constant = 0; @@ -737,10 +736,6 @@ gfc_option.warn_intrinsic_shadow = value; break; - case OPT_Wuse_without_only: - gfc_option.warn_use_without_only = value; - break; - case OPT_Walign_commons: gfc_option.warn_align_commons = value; break; only in patch2: unchanged: --- gcc/testsuite/lib/gfortran-dg.exp (revision 216257) +++ gcc/testsuite/lib/gfortran-dg.exp (working copy) @@ -47,38 +47,49 @@ proc gfortran-dg-test { prog do_what ext # # some code and some more code # 1 2 # Error: Some error at (1) and (2) # - # Where [locus] is either [line] or [line].[columns] . + # or + # [name]:[locus]: Error: Some error + # + # Where [locus] is either [line] or [line].[column] or + # [line].[column]-[column] . # # We collapse these to look like: # [name]:[line]:[column]: Error: Some error at (1) and (2) # or # [name]:[line]:[column]: Error: Some error at (1) and (2) # [name]:[line2]:[column]: Error: Some error at (1) and (2) - # We proceed in two steps: first we deal with the form with two - # different locus lines, then with the form with only one locus line. # # Note that these regexps only make sense in the combinations used below. # Note also that is imperative that we first deal with the form with # two loci. - set locus_regexp "(\[^\n\]*):(\[0-9\]+)\[\.:\](\[0-9\]*)(-\[0-9\]*)?:\n\n\[^\n\]*\n\[^\n\]*\n" - set diag_regexp "(\[^\n\]*)\n" + set locus_regexp "(\[^\n\]+:\[0-9\]+)\[\.:\](\[0-9\]+)(-\[0-9\]+)?:\n\n\[^\n\]+\n\[^\n\]+\n" + set diag_regexp "(\[^\n\]+)\n" - # Add column number if none exists - set colnum_regexp "(Warning: |Error: )?(\[^\n\]*):(\[0-9\]+):(\[ \n\])" - regsub -all $colnum_regexp $comp_output "\\2:\\3:0:\\4\\1" comp_output - - set two_loci "$locus_regexp$locus_regexp$diag_regexp" - set single_locus "$locus_regexp$diag_regexp" - regsub -all $two_loci $comp_output "\\1:\\2:\\3: \\9\n\\5:\\6:\\7: \\9\n" comp_output - regsub -all $single_locus $comp_output "\\1:\\2:\\3: \\5\n" comp_output + # We proceed in steps: - # Add a line number if none exists - regsub -all "(^|\n)(Warning: |Error: )" $comp_output "\\1:0:0: \\2" comp_output + # 1. We add first a column number if none exists. + # (Some Fortran diagnostics have the locus after Warning|Error) + set colnum_regexp "(^|\n)(Warning: |Error: )?(\[^:\n\]+:\[0-9\]+):(\[ \n\])" + regsub -all $colnum_regexp $comp_output "\\1\\3:0:\\4\\2" comp_output + verbose "comput_output0:\n$comp_output" + # 2. We deal with the form with two different locus lines, + set two_loci "(^|\n)$locus_regexp$locus_regexp$diag_regexp" + regsub -all $two_loci $comp_output "\\1\\2:\\3: \\8\n\\5\:\\6: \\8\n" comp_output + verbose "comput_output1:\n$comp_output" + + # 3. then with the form with only one locus line. + set single_locus "(^|\n)$locus_regexp$diag_regexp" + regsub -all $single_locus $comp_output "\\1\\2:\\3: \\5\n" comp_output + verbose "comput_output2:\n$comp_output" + + # 4. Add a line number if none exists + regsub -all "(^|\n)(Warning: |Error: )" $comp_output "\\1:0:0: \\2" comp_output + verbose "comput_output3:\n$comp_output" return [list $comp_output $output_file] } proc gfortran-dg-prune { system text } { return [gcc-dg-prune $system $text] only in patch2: unchanged: --- gcc/testsuite/gfortran.dg/use_without_only_1.f90 (revision 216257) +++ gcc/testsuite/gfortran.dg/use_without_only_1.f90 (working copy) @@ -4,19 +4,19 @@ MODULE foo INTEGER :: bar END MODULE MODULE testmod - USE foo ! { dg-warning "has no ONLY qualifier" } + USE foo ! { dg-warning "6:has no ONLY qualifier" } IMPLICIT NONE CONTAINS SUBROUTINE S1 - USE foo ! { dg-warning "has no ONLY qualifier" } + USE foo ! { dg-warning "9:has no ONLY qualifier" } END SUBROUTINE S1 SUBROUTINE S2 USE foo, ONLY: bar END SUBROUTINE SUBROUTINE S3 - USE ISO_C_BINDING ! { dg-warning "has no ONLY qualifier" } + USE ISO_C_BINDING ! { dg-warning "9:has no ONLY qualifier" } END SUBROUTINE S3 END MODULE ! { dg-final { cleanup-modules "foo testmod" } } only in patch2: unchanged: --- gcc/fortran/lang.opt (revision 216257) +++ gcc/fortran/lang.opt (working copy) @@ -260,11 +260,11 @@ Warn on intrinsics not part of the selec Wmissing-include-dirs Fortran ; Documented in C/C++ Wuse-without-only -Fortran Warning +Fortran Var(warn_use_without_only) Warning Warn about USE statements that have no ONLY qualifier Wopenmp-simd Fortran ; Documented in C only in patch2: unchanged: --- gcc/fortran/module.c (revision 216257) +++ gcc/fortran/module.c (working copy) @@ -6742,12 +6742,13 @@ gfc_use_module (gfc_use_list *module) module_name = module->module_name; gfc_rename_list = module->rename; only_flag = module->only_flag; current_intmod = INTMOD_NONE; - if (!only_flag && gfc_option.warn_use_without_only) - gfc_warning_now ("USE statement at %C has no ONLY qualifier"); + if (!only_flag) + gfc_warning_now_2 (OPT_Wuse_without_only, + "USE statement at %C has no ONLY qualifier"); filename = XALLOCAVEC (char, strlen (module_name) + strlen (MODULE_EXTENSION) + 1); strcpy (filename, module_name); strcat (filename, MODULE_EXTENSION); only in patch2: unchanged: --- gcc/fortran/f95-lang.c (revision 216257) +++ gcc/fortran/f95-lang.c (working copy) @@ -218,10 +218,14 @@ gfc_be_parse_file (void) warningcount += warnings; /* Clear the binding level stack. */ while (!global_bindings_p ()) poplevel (0, 0); + + /* Switch to the default tree diagnostics here, because there may be + diagnostics before gfc_finish(). */ + gfc_diagnostics_finish (); } /* Initialize everything. */ only in patch2: unchanged: --- libcpp/include/line-map.h (revision 216257) +++ libcpp/include/line-map.h (working copy) @@ -601,10 +601,17 @@ linemap_position_for_column (struct line column. */ source_location linemap_position_for_line_and_column (const struct line_map *, linenum_type, unsigned int); +/* Encode and return a source_location starting from location LOC + and shifting it by OFFSET columns. */ +source_location +linemap_position_for_loc_and_offset (struct line_maps *set, + source_location loc, + unsigned int offset); + /* Return the file this map is for. */ #define LINEMAP_FILE(MAP) \ (linemap_check_ordinary (MAP)->d.ordinary.to_file) /* Return the line number this map started encoding location from. */ only in patch2: unchanged: --- libcpp/line-map.c (revision 216257) +++ libcpp/line-map.c (working copy) @@ -631,10 +631,41 @@ linemap_position_for_line_and_column (co + ((line - ORDINARY_MAP_STARTING_LINE_NUMBER (map)) << ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map)) + (column & ((1 << ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map)) - 1))); } +/* Encode and return a source_location starting from location LOC + and shifting it by OFFSET columns. */ + +source_location +linemap_position_for_loc_and_offset (struct line_maps *set, + source_location loc, + unsigned int offset) +{ + const struct line_map * map = NULL; + + /* This function does not support virtual locations yet. */ + linemap_assert (!linemap_location_from_macro_expansion_p (set, loc)); + + if (offset == 0) + return loc; + + /* First, we find the real location and shift it. */ + loc = linemap_resolve_location (set, loc, LRK_SPELLING_LOCATION, &map); + linemap_assert (MAP_START_LOCATION (map) < loc + offset); + + offset += SOURCE_COLUMN (map, loc); + linemap_assert (offset < (1u << map->d.ordinary.column_bits)); + + source_location r = + linemap_position_for_line_and_column (map, + SOURCE_LINE (map, loc), + offset); + linemap_assert (map == linemap_lookup (set, r)); + return r; +} + /* Given a virtual source location yielded by a map (either an ordinary or a macro map), returns that map. */ const struct line_map* linemap_lookup (struct line_maps *set, source_location line)