Use correct location information for OpenACC shape and simple clauses in C/C++

[Thomas Schwinge]

Hi!

On Thu, 4 Aug 2016 11:03:25 -0600, Jeff Law <law@redhat.com> wrote:
> On 07/27/2016 09:17 AM, Thomas Schwinge wrote:
> > I found that for a lot of OpenACC (and potentially also OpenMP) clauses
> > (in C/C++ front ends; didn't look at Fortran), we use wrong location
> > information.  The problem is that
> > c_parser_oacc_all_clauses/c_parser_omp_all_clauses calls
> > cp_parser_omp_clause_name to determine the pragma_omp_clause c_kind, and
> > that function (as documented) consumes the clause token before returning.
> > So, when we then do "c_parser_peek_token (parser)->location" or similar
> > in some clause parsing function, that will return the location
> > information of the token _after_ the clause token, which -- at least very
> > often -- is not desirable, in particular if that location information is
> > used then in a build_omp_clause call, which should point to the clause
> > token itself, and not whatever follows after that.
> >
> > Probably that all went unnoticed for so long, because the GCC testsuite
> > largely is running with -fno-diagnostics-show-caret, so we don't visually
> > see the wrong location information (and nobody pays attention to the
> > colum information as given, for example, as line 2, column 32 in
> > "[...]/c-c++-common/goacc/routine-2.c:2:32: error: [...]".
> >
> > There seems to be a lot of inconsistency in that in all the clause
> > parsing; here is just a first patch to fix the immediate problem I've
> > been observing.  OK for trunk already, or need to clean this all up in
> > one go?  Do we need this on release branches, as a "quality of
> > implementation" fix (wrong diagnostic locations)?

> > --- gcc/c/c-parser.c
> > +++ gcc/c/c-parser.c
> > @@ -11758,12 +11758,12 @@ c_parser_oacc_shape_clause (c_parser *parser, omp_clause_code kind,
> >     seq */
> >
> >  static tree
> > -c_parser_oacc_simple_clause (c_parser *parser, enum omp_clause_code code,
> > -			     tree list)
> > +c_parser_oacc_simple_clause (c_parser * /* parser */, location_t loc,
> > +			     enum omp_clause_code code, tree list)

> Any reason not to just drop the parser argument entirely?  If we must 
> have it to match an API, but don't need it, then just drop the argument 
> name entirely rather than commenting it out.  This kind of comment, IMHO 
> serves no useful purpose.

OK.  I had only left it in, as all the parser functions passed it in, but
yeah, that's not actually necessary (and can easily be restored should it
ever be needed).

> With that change and some tests (presumably using David recipe) this is 
> will be fine.

With test cases still deferred for proper inspection of all such clauses,
I have now at least committed the agreed-on code changes to fix things
for the cases already identified: committed to trunk in r269102 "[C, C++]
Use correct location information for OpenACC shape and simple clauses",
as attached.


Grüße
 Thomas


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-C-C-Use-correct-location-information-for-OpenACC-sha.patch
Type: text/x-diff
Size: 10469 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20190222/8fe193a9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 658 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20190222/8fe193a9/attachment.sig>