| Message ID | 9471094d-05b6-0196-02c5-f7868f139332@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | [c++,openmp] Improve diagnostics for unmappable types | expand |
On 6/28/19 6:46 AM, Andrew Stubbs wrote: > This patch improves the diagnostics given for unmappable C++ types in > OpenMP programs. > > Here is the output *without* the patch, for the new testcase: > > ---- > unmappable-1.C: In function 'int main()': > unmappable-1.C:16:24: error: 'v' does not have a mappable type in 'map' > clause > 16 | #pragma omp target map(v) > ---- > > This is correct, but not very helpful for anything but the most trivial > C++ types. Anything involving inheritance, templates, typedefs, etc. > could be extremely difficult to track down. > > With the patch applied we now get this (I removed the "dg-message" > comments for clarity): > > ---- > unmappable-1.C: In function 'int main()': > unmappable-1.C:16:24: error: 'v' does not have a mappable type in 'map' > clause > 16 | #pragma omp target map(v) > | ^ > cc1plus: note: incomplete types are not mappable > unmappable-1.C:4:7: note: types with virtual members are not mappable > 4 | class C > | ^ > unmappable-1.C:7:14: note: static fields are not mappable > 7 | static int static_member; > | ^~~~~~~~~~~~~ > ---- > > The compiler now reports all the problematic fields in the whole type, > recursively. If anybody knows how to report the location of incomplete > array declarations then that would be nice to add. > > OK to commit? > + inform ((decl ? DECL_SOURCE_LOCATION (decl) : UNKNOWN_LOCATION), > + "incomplete types are not mappable"); It's better to use input_location as a fallback; essentially no diagnostics should use UNKNOWN_LOCATION. And let's print the type with %qT. > + if (notes) > + result = false; > + else > + return false; Returning early when !notes doesn't seem worth the extra lines of code. Jason
On 28/06/2019 17:21, Jason Merrill wrote: >> + inform ((decl ? DECL_SOURCE_LOCATION (decl) : UNKNOWN_LOCATION), >> + "incomplete types are not mappable"); > > It's better to use input_location as a fallback; essentially no > diagnostics should use UNKNOWN_LOCATION. And let's print the type with > %qT. > >> + if (notes) >> + result = false; >> + else >> + return false; > > Returning early when !notes doesn't seem worth the extra lines of code. How is this version? Andrew
On 7/1/19 7:16 AM, Andrew Stubbs wrote: > On 28/06/2019 17:21, Jason Merrill wrote: >>> + inform ((decl ? DECL_SOURCE_LOCATION (decl) : UNKNOWN_LOCATION), >>> + "incomplete types are not mappable"); >> >> It's better to use input_location as a fallback; essentially no >> diagnostics should use UNKNOWN_LOCATION. And let's print the type >> with %qT. >> >>> + if (notes) >>> + result = false; >>> + else >>> + return false; >> >> Returning early when !notes doesn't seem worth the extra lines of code. > > How is this version? OK, thanks. Jason
On 03/07/2019 18:58, Jason Merrill wrote:
> OK, thanks.
Committed.
Thanks for the reviews.
Andrew
This patch has now been backported to openacc-gcc-9-branch (git). Andre On 01/07/2019 12:16, Andrew Stubbs wrote: > Improve OpenMP map diagnostics. > > 2019-07-01 Andrew Stubbs<ams@codesourcery.com> > > gcc/cp/ > * cp-tree.h (cp_omp_emit_unmappable_type_notes): New prototype. > * decl.c (cp_finish_decl): Call cp_omp_emit_unmappable_type_notes. > * decl2.c (cp_omp_mappable_type): Move contents to ... > (cp_omp_mappable_type_1): ... here and add note output. > (cp_omp_emit_unmappable_type_notes): New function. > * semantics.c (finish_omp_clauses): Call > cp_omp_emit_unmappable_type_notes in four places. > > gcc/testsuite/ > * g++.dg/gomp/unmappable-1.C: New file. > > ======================================= > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index bf47f67721e..a7b2151e6dd 100644 > > --- a/gcc/cp/cp-tree.h > > +++ b/gcc/cp/cp-tree.h > > @@ -6533,6 +6533,7 @@ > > extern int parm_index (tree); > > extern tree vtv_start_verification_constructor_init_function (void); > extern tree vtv_finish_verification_constructor_init_function (tree); > extern bool cp_omp_mappable_type (tree); > +extern bool cp_omp_emit_unmappable_type_notes (tree); > extern void cp_check_const_attributes (tree); > /* in error.c */ > > ======================================= > > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c > index 5d49535b0d9..74343bc1ec4 100644 > > --- a/gcc/cp/decl.c > > +++ b/gcc/cp/decl.c > > @@ -7433,8 +7433,11 @@ > > cp_finish_decl (tree decl, tree init, bool init_const_expr_p, > > DECL_ATTRIBUTES (decl)); > complete_type (TREE_TYPE (decl)); > if (!cp_omp_mappable_type (TREE_TYPE (decl))) > - error ("%q+D in declare target directive does not have mappable type", > - decl); > + { > + error ("%q+D in declare target directive does not have mappable" > + " type", decl); > + cp_omp_emit_unmappable_type_notes (TREE_TYPE (decl)); > + } > else if (!lookup_attribute ("omp declare target", > DECL_ATTRIBUTES (decl)) > && !lookup_attribute ("omp declare target link", > > ======================================= > > diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c > index 206f04c6320..b415716c7dd 100644 > > --- a/gcc/cp/decl2.c > > +++ b/gcc/cp/decl2.c > > @@ -1406,32 +1406,68 @@ > > cp_check_const_attributes (tree attributes) > > } > } > -/* Return true if TYPE is an OpenMP mappable type. */ > -bool > -cp_omp_mappable_type (tree type) > +/* Return true if TYPE is an OpenMP mappable type. > + If NOTES is non-zero, emit a note message for each problem. */ > +static bool > +cp_omp_mappable_type_1 (tree type, bool notes) > { > + bool result = true; > + > /* Mappable type has to be complete. */ > if (type == error_mark_node || !COMPLETE_TYPE_P (type)) > - return false; > + { > + if (notes) > + { > + tree decl = TYPE_MAIN_DECL (type); > + inform ((decl ? DECL_SOURCE_LOCATION (decl) : input_location), > + "incomplete type %qT is not mappable", type); > + } > + result = false; > + } > /* Arrays have mappable type if the elements have mappable type. */ > while (TREE_CODE (type) == ARRAY_TYPE) > type = TREE_TYPE (type); > /* A mappable type cannot contain virtual members. */ > if (CLASS_TYPE_P (type) && CLASSTYPE_VTABLES (type)) > - return false; > + { > + if (notes) > + inform (DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (type)), > + "type %qT with virtual members is not mappable", type); > + result = false; > + } > /* All data members must be non-static. */ > if (CLASS_TYPE_P (type)) > { > tree field; > for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field)) > if (VAR_P (field)) > - return false; > + { > + if (notes) > + inform (DECL_SOURCE_LOCATION (field), > + "static field %qD is not mappable", field); > + result = false; > + } > /* All fields must have mappable types. */ > else if (TREE_CODE (field) == FIELD_DECL > - && !cp_omp_mappable_type (TREE_TYPE (field))) > - return false; > + && !cp_omp_mappable_type_1 (TREE_TYPE (field), notes)) > + result = false; > } > - return true; > + return result; > +} > + > +/* Return true if TYPE is an OpenMP mappable type. */ > +bool > +cp_omp_mappable_type (tree type) > +{ > + return cp_omp_mappable_type_1 (type, false); > +} > + > +/* Return true if TYPE is an OpenMP mappable type. > + Emit an error messages if not. */ > +bool > +cp_omp_emit_unmappable_type_notes (tree type) > +{ > + return cp_omp_mappable_type_1 (type, true); > } > /* Return the last pushed declaration for the symbol DECL or NULL > > ======================================= > > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c > index 92c48753d42..8f019580d0f 100644 > > --- a/gcc/cp/semantics.c > > +++ b/gcc/cp/semantics.c > > @@ -7090,6 +7090,7 @@ > > finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > > "array section does not have mappable type " > "in %qs clause", > omp_clause_code_name[OMP_CLAUSE_CODE (c)]); > + cp_omp_emit_unmappable_type_notes (TREE_TYPE (t)); > remove = true; > } > while (TREE_CODE (t) == ARRAY_REF) > > @@ -7158,6 +7159,7 @@ > > finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > > error_at (OMP_CLAUSE_LOCATION (c), > "%qE does not have a mappable type in %qs clause", > t, omp_clause_code_name[OMP_CLAUSE_CODE (c)]); > + cp_omp_emit_unmappable_type_notes (TREE_TYPE (t)); > remove = true; > } > while (TREE_CODE (t) == COMPONENT_REF) > > @@ -7236,6 +7238,7 @@ > > finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > > error_at (OMP_CLAUSE_LOCATION (c), > "%qD does not have a mappable type in %qs clause", t, > omp_clause_code_name[OMP_CLAUSE_CODE (c)]); > + cp_omp_emit_unmappable_type_notes (TREE_TYPE (t)); > remove = true; > } > else if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP > > @@ -7384,6 +7387,7 @@ > > finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > > error_at (OMP_CLAUSE_LOCATION (c), > "%qD does not have a mappable type in %qs clause", t, > omp_clause_code_name[OMP_CLAUSE_CODE (c)]); > + cp_omp_emit_unmappable_type_notes (TREE_TYPE (t)); > remove = true; > } > if (remove) > > ======================================= > > diff --git a/gcc/testsuite/g++.dg/gomp/unmappable-1.C b/gcc/testsuite/g++.dg/gomp/unmappable-1.C > new file mode 100644 > index 00000000000..d00ccb5ad79 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/gomp/unmappable-1.C > > @@ -0,0 +1,20 @@ > > +/* { dg-do compile } */ > +/* { dg-options "-fopenmp" } */ > + > +class C /* { dg-message "type .C. with virtual members is not mappable" > } */ > +{ > +public: > + static int static_member; /* { dg-message "static field > .C::static_member. is not mappable" } */ > + virtual void f() {} > +}; > + > +extern C v[]; > + > +int > +main () > +{ > +#pragma omp target map(v) /* { dg-error ".v. does not have a mappable > type in .map. clause" } */ > + /* { dg-message "incomplete type .C \\\[\\\]. is not mappable" "" { > target *-*-* } .-1 } */ > + { > + } > +}
On Thu, Jul 04, 2019 at 12:44:32PM +0100, Andrew Stubbs wrote: > On 03/07/2019 18:58, Jason Merrill wrote: > > OK, thanks. > > Committed. This broke following testcase. error_mark_node type isn't really incomplete, it is errorneous, doesn't have TYPE_MAIN_DECL and we should have diagnosed it earlier, so it makes no sense to emit extra explanation messages. Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, committed to trunk. 2019-07-08 Jakub Jelinek <jakub@redhat.com> PR c++/91110 * decl2.c (cp_omp_mappable_type_1): Don't emit any note for error_mark_node type. * g++.dg/gomp/pr91110.C: New test. --- gcc/cp/decl2.c.jj 2019-07-04 23:39:02.579106113 +0200 +++ gcc/cp/decl2.c 2019-07-08 13:22:52.552898230 +0200 @@ -1416,7 +1416,7 @@ cp_omp_mappable_type_1 (tree type, bool /* Mappable type has to be complete. */ if (type == error_mark_node || !COMPLETE_TYPE_P (type)) { - if (notes) + if (notes && type != error_mark_node) { tree decl = TYPE_MAIN_DECL (type); inform ((decl ? DECL_SOURCE_LOCATION (decl) : input_location), --- gcc/testsuite/g++.dg/gomp/pr91110.C.jj 2019-07-08 13:29:43.803163534 +0200 +++ gcc/testsuite/g++.dg/gomp/pr91110.C 2019-07-08 13:29:17.550593456 +0200 @@ -0,0 +1,11 @@ +// PR c++/91110 +// { dg-do compile } + +void +foo () +{ + X b[2]; // { dg-error "'X' was not declared in this scope" } + b[0] = 1; // { dg-error "'b' was not declared in this scope" } + #pragma omp target map(to: b) // { dg-error "'b' does not have a mappable type in 'map' clause" } + ; +} Jakub
On 08/07/2019 23:10, Jakub Jelinek wrote: > This broke following testcase. > error_mark_node type isn't really incomplete, it is errorneous, doesn't have > TYPE_MAIN_DECL and we should have diagnosed it earlier, so it makes no sense > to emit extra explanation messages. Apologies. Did I miss something in the regression tests? _Atomic-5 seems fine? Andrew
I've backported Jakub's patch to openacc-gcc-9-branch. Andrew On 08/07/2019 23:10, Jakub Jelinek wrote: > On Thu, Jul 04, 2019 at 12:44:32PM +0100, Andrew Stubbs wrote: >> On 03/07/2019 18:58, Jason Merrill wrote: >>> OK, thanks. >> >> Committed. > > This broke following testcase. > error_mark_node type isn't really incomplete, it is errorneous, doesn't have > TYPE_MAIN_DECL and we should have diagnosed it earlier, so it makes no sense > to emit extra explanation messages. > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, > committed to trunk. > > 2019-07-08 Jakub Jelinek <jakub@redhat.com> > > PR c++/91110 > * decl2.c (cp_omp_mappable_type_1): Don't emit any note for > error_mark_node type. > > * g++.dg/gomp/pr91110.C: New test. > > --- gcc/cp/decl2.c.jj 2019-07-04 23:39:02.579106113 +0200 > +++ gcc/cp/decl2.c 2019-07-08 13:22:52.552898230 +0200 > @@ -1416,7 +1416,7 @@ cp_omp_mappable_type_1 (tree type, bool > /* Mappable type has to be complete. */ > if (type == error_mark_node || !COMPLETE_TYPE_P (type)) > { > - if (notes) > + if (notes && type != error_mark_node) > { > tree decl = TYPE_MAIN_DECL (type); > inform ((decl ? DECL_SOURCE_LOCATION (decl) : input_location), > --- gcc/testsuite/g++.dg/gomp/pr91110.C.jj 2019-07-08 13:29:43.803163534 +0200 > +++ gcc/testsuite/g++.dg/gomp/pr91110.C 2019-07-08 13:29:17.550593456 +0200 > @@ -0,0 +1,11 @@ > +// PR c++/91110 > +// { dg-do compile } > + > +void > +foo () > +{ > + X b[2]; // { dg-error "'X' was not declared in this scope" } > + b[0] = 1; // { dg-error "'b' was not declared in this scope" } > + #pragma omp target map(to: b) // { dg-error "'b' does not have a mappable type in 'map' clause" } > + ; > +} > > > Jakub >
Improve OpenMP map diagnostics. 2019-06-27 Andrew Stubbs <ams@codesourcery.com> gcc/cp/ * cp-tree.h (cp_omp_emit_unmappable_type_notes): New prototype. * decl.c (cp_finish_decl): Call cp_omp_emit_unmappable_type_notes. * decl2.c (cp_omp_mappable_type): Move contents to ... (cp_omp_mappable_type_1): ... here and add note output. (cp_omp_emit_unmappable_type_notes): New function. * semantics.c (finish_omp_clauses): Call cp_omp_emit_unmappable_type_notes in four places. gcc/testsuite/ * g++.dg/gomp/unmappable-1.C: New file. diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index bf47f67721e..a7b2151e6dd 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -6533,6 +6533,7 @@ extern int parm_index (tree); extern tree vtv_start_verification_constructor_init_function (void); extern tree vtv_finish_verification_constructor_init_function (tree); extern bool cp_omp_mappable_type (tree); +extern bool cp_omp_emit_unmappable_type_notes (tree); extern void cp_check_const_attributes (tree); /* in error.c */ diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c index 5d49535b0d9..74343bc1ec4 100644 --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -7433,8 +7433,11 @@ cp_finish_decl (tree decl, tree init, bool init_const_expr_p, DECL_ATTRIBUTES (decl)); complete_type (TREE_TYPE (decl)); if (!cp_omp_mappable_type (TREE_TYPE (decl))) - error ("%q+D in declare target directive does not have mappable type", - decl); + { + error ("%q+D in declare target directive does not have mappable" + " type", decl); + cp_omp_emit_unmappable_type_notes (TREE_TYPE (decl)); + } else if (!lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl)) && !lookup_attribute ("omp declare target link", diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c index 206f04c6320..17deeda75f8 100644 --- a/gcc/cp/decl2.c +++ b/gcc/cp/decl2.c @@ -1406,32 +1406,83 @@ cp_check_const_attributes (tree attributes) } } -/* Return true if TYPE is an OpenMP mappable type. */ -bool -cp_omp_mappable_type (tree type) +/* Return true if TYPE is an OpenMP mappable type. + If NOTES is non-zero, emit a note message for each problem. */ +static bool +cp_omp_mappable_type_1 (tree type, bool notes) { + bool result = true; + /* Mappable type has to be complete. */ if (type == error_mark_node || !COMPLETE_TYPE_P (type)) - return false; + { + if (notes) + { + tree decl = TYPE_MAIN_DECL (type); + inform ((decl ? DECL_SOURCE_LOCATION (decl) : UNKNOWN_LOCATION), + "incomplete types are not mappable"); + result = false; + } + else + return false; + } /* Arrays have mappable type if the elements have mappable type. */ while (TREE_CODE (type) == ARRAY_TYPE) type = TREE_TYPE (type); /* A mappable type cannot contain virtual members. */ if (CLASS_TYPE_P (type) && CLASSTYPE_VTABLES (type)) - return false; + { + if (notes) + { + inform (DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (type)), + "types with virtual members are not mappable"); + result = false; + } + else + return false; + } /* All data members must be non-static. */ if (CLASS_TYPE_P (type)) { tree field; for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field)) if (VAR_P (field)) - return false; + { + if (notes) + { + inform (DECL_SOURCE_LOCATION (field), + "static fields are not mappable"); + result = false; + } + else + return false; + } /* All fields must have mappable types. */ else if (TREE_CODE (field) == FIELD_DECL - && !cp_omp_mappable_type (TREE_TYPE (field))) - return false; + && !cp_omp_mappable_type_1 (TREE_TYPE (field), notes)) + { + if (notes) + result = false; + else + return false; + } } - return true; + return result; +} + +/* Return true if TYPE is an OpenMP mappable type. */ +bool +cp_omp_mappable_type (tree type) +{ + return cp_omp_mappable_type_1 (type, false); +} + +/* Return true if TYPE is an OpenMP mappable type. + Emit an error messages if not. */ +bool +cp_omp_emit_unmappable_type_notes (tree type) +{ + return cp_omp_mappable_type_1 (type, true); } /* Return the last pushed declaration for the symbol DECL or NULL diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c index 92c48753d42..8f019580d0f 100644 --- a/gcc/cp/semantics.c +++ b/gcc/cp/semantics.c @@ -7090,6 +7090,7 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) "array section does not have mappable type " "in %qs clause", omp_clause_code_name[OMP_CLAUSE_CODE (c)]); + cp_omp_emit_unmappable_type_notes (TREE_TYPE (t)); remove = true; } while (TREE_CODE (t) == ARRAY_REF) @@ -7158,6 +7159,7 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) error_at (OMP_CLAUSE_LOCATION (c), "%qE does not have a mappable type in %qs clause", t, omp_clause_code_name[OMP_CLAUSE_CODE (c)]); + cp_omp_emit_unmappable_type_notes (TREE_TYPE (t)); remove = true; } while (TREE_CODE (t) == COMPONENT_REF) @@ -7236,6 +7238,7 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) error_at (OMP_CLAUSE_LOCATION (c), "%qD does not have a mappable type in %qs clause", t, omp_clause_code_name[OMP_CLAUSE_CODE (c)]); + cp_omp_emit_unmappable_type_notes (TREE_TYPE (t)); remove = true; } else if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP @@ -7384,6 +7387,7 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) error_at (OMP_CLAUSE_LOCATION (c), "%qD does not have a mappable type in %qs clause", t, omp_clause_code_name[OMP_CLAUSE_CODE (c)]); + cp_omp_emit_unmappable_type_notes (TREE_TYPE (t)); remove = true; } if (remove) diff --git a/gcc/testsuite/g++.dg/gomp/unmappable-1.C b/gcc/testsuite/g++.dg/gomp/unmappable-1.C new file mode 100644 index 00000000000..29739240620 --- /dev/null +++ b/gcc/testsuite/g++.dg/gomp/unmappable-1.C @@ -0,0 +1,19 @@ +/* { dg-do compile } */ +/* { dg-options "-fopenmp" } */ + +class C /* { dg-message "types with virtual members are not mappable" } */ +{ +public: + static int static_member; /* { dg-message "static fields are not mappable" } */ + virtual void f() {} +}; + +extern C v[]; /* { dg-message "note: incomplete types are not mappable" "" { target *-*-* } 0 } */ + +int +main () +{ +#pragma omp target map(v) /* { dg-error ".v. does not have a mappable type in .map. clause" } */ + { + } +}
This patch improves the diagnostics given for unmappable C++ types in OpenMP programs. Here is the output *without* the patch, for the new testcase: ---- unmappable-1.C: In function 'int main()': unmappable-1.C:16:24: error: 'v' does not have a mappable type in 'map' clause 16 | #pragma omp target map(v) ---- This is correct, but not very helpful for anything but the most trivial C++ types. Anything involving inheritance, templates, typedefs, etc. could be extremely difficult to track down. With the patch applied we now get this (I removed the "dg-message" comments for clarity): ---- unmappable-1.C: In function 'int main()': unmappable-1.C:16:24: error: 'v' does not have a mappable type in 'map' clause 16 | #pragma omp target map(v) | ^ cc1plus: note: incomplete types are not mappable unmappable-1.C:4:7: note: types with virtual members are not mappable 4 | class C | ^ unmappable-1.C:7:14: note: static fields are not mappable 7 | static int static_member; | ^~~~~~~~~~~~~ ---- The compiler now reports all the problematic fields in the whole type, recursively. If anybody knows how to report the location of incomplete array declarations then that would be nice to add. OK to commit? Andrew