| Message ID | 20141015172244.GE10501@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/15/14 11:22, Marek Polacek wrote: > We've got a complaint that the "dereferencing pointer to incomplete > type" error is printed for all occurrences of the incomplete type, > which is too verbose. Also it'd be nicer to print the type as well. > This patch fixes this; if we find an incomplete type, mark it with error > node, then we don't print the error message more than once. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2014-10-15 Marek Polacek <polacek@redhat.com> > > PR c/63543 > * c-typeck.c (build_indirect_ref): Print the "dereferencing..." > error only once for each type. Print also the type. > > * gcc.dg/pr63543.c: New test. OK. Jeff
On Wed, 15 Oct 2014, Marek Polacek wrote: > We've got a complaint that the "dereferencing pointer to incomplete > type" error is printed for all occurrences of the incomplete type, > which is too verbose. Also it'd be nicer to print the type as well. > This patch fixes this; if we find an incomplete type, mark it with error > node, then we don't print the error message more than once. I don't like this approach of modifying the type; type nodes are shared objects and this could affect all sorts of other logic subsequently working with the type. I think there should be some sort of annotation of the type (either in the type itself, or on the side) that *only* means an error has been given for the type being incomplete, rather than inserting error_mark_node into the type.
On 10/15/14 15:46, Joseph S. Myers wrote: > On Wed, 15 Oct 2014, Marek Polacek wrote: > >> We've got a complaint that the "dereferencing pointer to incomplete >> type" error is printed for all occurrences of the incomplete type, >> which is too verbose. Also it'd be nicer to print the type as well. >> This patch fixes this; if we find an incomplete type, mark it with error >> node, then we don't print the error message more than once. > > I don't like this approach of modifying the type; type nodes are shared > objects and this could affect all sorts of other logic subsequently > working with the type. I think there should be some sort of annotation of > the type (either in the type itself, or on the side) that *only* means an > error has been given for the type being incomplete, rather than inserting > error_mark_node into the type. Isn't slamming error_mark_node well established at this point? I fact I recall seeing it documented to be used in this kind of way to prevent future errors. jeff
On Wed, Oct 15, 2014 at 03:48:20PM -0600, Jeff Law wrote: > On 10/15/14 15:46, Joseph S. Myers wrote: > >On Wed, 15 Oct 2014, Marek Polacek wrote: > > > >>We've got a complaint that the "dereferencing pointer to incomplete > >>type" error is printed for all occurrences of the incomplete type, > >>which is too verbose. Also it'd be nicer to print the type as well. > >>This patch fixes this; if we find an incomplete type, mark it with error > >>node, then we don't print the error message more than once. > > > >I don't like this approach of modifying the type; type nodes are shared > >objects and this could affect all sorts of other logic subsequently > >working with the type. I think there should be some sort of annotation of > >the type (either in the type itself, or on the side) that *only* means an > >error has been given for the type being incomplete, rather than inserting > >error_mark_node into the type. > Isn't slamming error_mark_node well established at this point? I fact I > recall seeing it documented to be used in this kind of way to prevent future > errors. I think in this case it is way too risky to put error_mark_node there, it will affect also all the places where the object with that type isn't dereferenced. If there are spare bits on the type, either using one for this error, or to represent some error has been diagnosed for the particular type and hash table lookup could be used to look up which exactly, or just hash table without any bit... Jakub
On Wed, 15 Oct 2014, Jeff Law wrote: > On 10/15/14 15:46, Joseph S. Myers wrote: > > On Wed, 15 Oct 2014, Marek Polacek wrote: > > > > > We've got a complaint that the "dereferencing pointer to incomplete > > > type" error is printed for all occurrences of the incomplete type, > > > which is too verbose. Also it'd be nicer to print the type as well. > > > This patch fixes this; if we find an incomplete type, mark it with error > > > node, then we don't print the error message more than once. > > > > I don't like this approach of modifying the type; type nodes are shared > > objects and this could affect all sorts of other logic subsequently > > working with the type. I think there should be some sort of annotation of > > the type (either in the type itself, or on the side) that *only* means an > > error has been given for the type being incomplete, rather than inserting > > error_mark_node into the type. > Isn't slamming error_mark_node well established at this point? I fact I > recall seeing it documented to be used in this kind of way to prevent future > errors. Returning error_mark_node for the erroneous expression, yes - the pre-existing code already does that in this case. The problem is that the insertion of error_mark_node into the type will lead to other uses of that type (including ones that have already been processed without errors) being affected, and the type itself isn't erroneous. Indeed, the patch would create a "pointer-to-error_mark" type node, which is not something code in GCC would ever normally expect to handle (build_pointer_type_for_mode just returns error_mark_node if passed error_mark_node, so you can't get a POINTER_TYPE whose target is error_mark_node that way).
On Wed, Oct 15, 2014 at 11:52:59PM +0200, Jakub Jelinek wrote: > On Wed, Oct 15, 2014 at 03:48:20PM -0600, Jeff Law wrote: > > Isn't slamming error_mark_node well established at this point? I fact I > > recall seeing it documented to be used in this kind of way to prevent future > > errors. > > I think in this case it is way too risky to put error_mark_node there, > it will affect also all the places where the object with that type isn't > dereferenced. > If there are spare bits on the type, either using one for this error, or > to represent some error has been diagnosed for the particular type and > hash table lookup could be used to look up which exactly, or just hash table > without any bit... I thought sticking error_mark_node into the type would be feasible here, because we do it elsewhere too, and the program is invalid anyway. Well - maybe the new patch is better. Thanks, Marek
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c index 5c0697a..f00069c 100644 --- gcc/c/c-typeck.c +++ gcc/c/c-typeck.c @@ -2378,7 +2378,10 @@ build_indirect_ref (location_t loc, tree ptr, ref_operator errstring) if (!COMPLETE_OR_VOID_TYPE_P (t) && TREE_CODE (t) != ARRAY_TYPE) { - error_at (loc, "dereferencing pointer to incomplete type"); + /* Print the error only once for each type. */ + TREE_TYPE (ptr) = error_mark_node; + error_at (loc, "dereferencing pointer to incomplete type %qT", + t); return error_mark_node; } if (VOID_TYPE_P (t) && c_inhibit_evaluation_warnings == 0) diff --git gcc/testsuite/gcc.dg/pr63543.c gcc/testsuite/gcc.dg/pr63543.c index e69de29..215b62e 100644 --- gcc/testsuite/gcc.dg/pr63543.c +++ gcc/testsuite/gcc.dg/pr63543.c @@ -0,0 +1,21 @@ +/* PR c/63543 */ +/* { dg-do compile } */ + +struct S; +union U; + +int +f1 (struct S *s) +{ + return s->a /* { dg-error "dereferencing pointer to incomplete type .struct S." } */ + + s->b + + s->c; +} + +int +f2 (union U *u) +{ + return u->a /* { dg-error "dereferencing pointer to incomplete type .union U." } */ + + u->a + + u->a; +}