On 07/27/2016 09:17 AM, Thomas Schwinge wrote:
Hi!
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)?
commit bac4c04ca1d52c56a3583f5958e116c62b889d5a
Author: Thomas Schwinge <tho...@codesourcery.com>
Date: Wed Jul 27 16:55:56 2016 +0200
Use correct location information for OpenACC shape and simple clauses in
C/C++
gcc/c/
* c-parser.c (c_parser_oacc_shape_clause)
(c_parser_oacc_simple_clause): Add loc formal parameter. Adjust
all users.
gcc/cp/
* parser.c (cp_parser_oacc_shape_clause): Add loc formal
parameter. Adjust all users.
---
gcc/c/c-parser.c | 25 +++++++++++++------------
gcc/cp/parser.c | 12 +++++++-----
2 files changed, 20 insertions(+), 17 deletions(-)
diff --git gcc/c/c-parser.c gcc/c/c-parser.c
index 0031481..82ac855 100644
--- 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.
With that change and some tests (presumably using David recipe) this is
will be fine.
jeff
