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" } */ + { + } +}