| Message ID | 0624ce85-2ee7-38eb-d115-509d600d3f20@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix column information for omp_clauses in Fortran code | expand |
LGTM. Thanks for the patch! Tobias On 12/9/19 4:58 PM, Harwath, Frederik wrote: > Hi, > Tobias has recently fixed a problem with the column information in gfortran locations > ("PR 92793 - fix column used for error diagnostic"). Diagnostic messages for OpenMP/OpenACC > clauses do not contain the right column information yet. The reason is that the location > information of the first clause is used for all clauses on a line and hence the columns > are wrong for all but the first clause. The attached patch fixes this problem. > > I have tested the patch manually by adapting the validity check for nested OpenACC reductions (see omp-low.c) > to include the location of clauses in warnings instead of the location of the loop to which the clause belongs. > I can add a regression test based on this later on after adapting the code in omp-low.c. > > Is it ok to include the patch in trunk? > > Best regards, > Frederik > > > On 04.12.19 14:37, Tobias Burnus wrote: >> As reported internally by Frederik, gfortran currently passes LOCATION_COLUMN == 0 to the middle end. The reason for that is how parsing works – gfortran reads the input line by line. >> >> For internal error diagnostic (fortran/error.c), the column location was corrected – but not for locations passed to the middle end. Hence, the diagnostic there wasn't optimal. >> >> Fixed by introducing a new function; now one only needs to make sure that no new code will re-introduce "lb->location" :-) >> >> Build and regtested on x86-64-gnu-linux. >> OK for the trunk? >> >> Tobias
Hi! On 2019-12-09T16:58:44+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: > Tobias has recently fixed a problem with the column information in gfortran locations > ("PR 92793 - fix column used for error diagnostic"). In context of: > [use] the location of clauses in warnings instead of the location of the loop to which the clause belongs. ..., Frederik then did: > Subject: [PATCH] Fix column information for omp_clauses in Fortran code > > The location of all OpenMP/OpenACC clauses on any given line in Fortran code > always points to the first clause on that line. Hence, the column information > is wrong for all clauses but the first one. > > Use the correct location for each clause instead. Actually, that was specific for 'reduction' clauses: > --- a/gcc/fortran/trans-openmp.c > +++ b/gcc/fortran/trans-openmp.c > @@ -1982,7 +1982,7 @@ gfc_trans_omp_reduction_list (gfc_omp_namelist *namelist, tree list, > tree t = gfc_trans_omp_variable (namelist->sym, false); > if (t != error_mark_node) > { > - tree node = build_omp_clause (gfc_get_location (&where), > + tree node = build_omp_clause (gfc_get_location (&namelist->where), > OMP_CLAUSE_REDUCTION); Similar changes are desirable for other directives/clauses, too. I've just pushed "[Fortran] More precise location information for OpenACC 'gang', 'worker', 'vector' clauses with argument [PR92793]" to master branch in commit 41f7f6178e2d35288273656dc55dae8fcf3edeb5, and backported to releases/gcc-10 in commit 5ceaf8a54abb3f9bd3c268fe420999a7962b2a15, see attached. Grüße Thomas ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
From af3a63b64f38d522b0091a123a919d1f20f5a8b1 Mon Sep 17 00:00:00 2001 From: Frederik Harwath <frederik@codesourcery.com> Date: Mon, 9 Dec 2019 15:07:53 +0100 Subject: [PATCH] Fix column information for omp_clauses in Fortran code The location of all OpenMP/OpenACC clauses on any given line in Fortran code always points to the first clause on that line. Hence, the column information is wrong for all clauses but the first one. Use the correct location for each clause instead. 2019-12-09 Frederik Harwath <frederik@codesourcery.com> /gcc/fortran/ * trans-openmp (gfc_trans_omp_reduction_list): Pass correct location for each clause to build_omp_clause. --- gcc/fortran/trans-openmp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c index d07ff86fc0b..356fd04e6c3 100644 --- a/gcc/fortran/trans-openmp.c +++ b/gcc/fortran/trans-openmp.c @@ -1982,7 +1982,7 @@ gfc_trans_omp_reduction_list (gfc_omp_namelist *namelist, tree list, tree t = gfc_trans_omp_variable (namelist->sym, false); if (t != error_mark_node) { - tree node = build_omp_clause (gfc_get_location (&where), + tree node = build_omp_clause (gfc_get_location (&namelist->where), OMP_CLAUSE_REDUCTION); OMP_CLAUSE_DECL (node) = t; if (mark_addressable) -- 2.17.1
Hi, Tobias has recently fixed a problem with the column information in gfortran locations ("PR 92793 - fix column used for error diagnostic"). Diagnostic messages for OpenMP/OpenACC clauses do not contain the right column information yet. The reason is that the location information of the first clause is used for all clauses on a line and hence the columns are wrong for all but the first clause. The attached patch fixes this problem. I have tested the patch manually by adapting the validity check for nested OpenACC reductions (see omp-low.c) to include the location of clauses in warnings instead of the location of the loop to which the clause belongs. I can add a regression test based on this later on after adapting the code in omp-low.c. Is it ok to include the patch in trunk? Best regards, Frederik On 04.12.19 14:37, Tobias Burnus wrote: > As reported internally by Frederik, gfortran currently passes LOCATION_COLUMN == 0 to the middle end. The reason for that is how parsing works – gfortran reads the input line by line. > > For internal error diagnostic (fortran/error.c), the column location was corrected – but not for locations passed to the middle end. Hence, the diagnostic there wasn't optimal. > > Fixed by introducing a new function; now one only needs to make sure that no new code will re-introduce "lb->location" :-) > > Build and regtested on x86-64-gnu-linux. > OK for the trunk? > > Tobias