diff mbox series

add -Wmismatched-tags (PR 61339)

Message ID 0c037e9a-d26e-7557-7b0b-225b7ff70f48@gmail.com
State New
Headers show
Series add -Wmismatched-tags (PR 61339) | expand

Commit Message

Martin Sebor Dec. 3, 2019, 9:49 p.m. UTC
On 8/5/19 4:30 PM, Jason Merrill wrote:
> On Mon, Aug 5, 2019 at 5:50 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 8/5/19 1:25 PM, Jason Merrill wrote:
>>> On 8/1/19 7:35 PM, Martin Sebor wrote:
>>>> On 8/1/19 12:09 PM, Jason Merrill wrote:
>>>>> On 7/22/19 12:34 PM, Martin Sebor wrote:
>>>>>> Ping: https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00622.html
...
>>>>> -Wmismatched-tags is useful to have, given the MSVC/clang situation,
>>>>> but I wonder about memory consumption from all the record keeping.
>>>>> Do you have any overhead measurements?
>>>>
>>>> I did think about the overhead but not knowing if the patch would
>>>> even be considered I didn't spend time optimizing it.
>>>>
>>>> In an instrumented build of GCC with the patch I just did I have
>>>> collected the following stats for the number of elements in
>>>> the rec2loc hash table (for 998 files):
>>>>
>>>>     rec2loc elements   locvec elements
>>>>     min:           0                 0
>>>>     max:       2,785             3,303
>>>>     mean:        537             1,043
>>>>     median:      526             1,080
>>>>
>>>> The locvec data are based on totals for the whole hash table, so
>>>> in the worst case the hash_map has 2,785 elements, and the sum of
>>>> all locvec lengths in all those elements is 3,303.  Each rec2loc
>>>> element takes up 16 bytes, plus the size of the locvec data.
>>>>
>>>> If my math is right (which doesn't happen too often) in the worst
>>>> case the bookkeeping overhead is 43KB for the hash_map plus on
>>>> the order of 140KB for the vectors (just doubling the number of
>>>> elements to account for capacity = 2 X size, times 24 for
>>>> the flag_func_loc_t element type).  So 183K in the worst case
>>>> in GCC.
>>>
>>> 183k cumulative over all the GCC sources doesn't sound excessive, but...
>>>
>>>> There are a few ways to reduce that if it seems excessive.
>>>>
>>>> One is by avoiding some waste in flag_func_loc_t which is
>>>>
>>>>     pair<tag_types, pair<bool, pair<tree, location_t>>>
>>>>
>>>> tree could come first and tag_types and the bool could share
>>>> space.  That should bring it down to 16 in LP64, for about
>>>> 30% off the 183K.
>>>>
>>>> Beyond that, we could change the algorithm to discard records
>>>> for prior declarations after the first definition has been seen
>>>> (and any mismatches diagnosed).
>>>>
>>>> We could also only collect one record for each definition
>>>> in system headers rather than one for every declaration and
>>>> reference.
>>>
>>> ...these all sound worthwhile.
>>
>> Okay, I'll look into it.
>>
>> To be clear: it was 183K maximum for a single compilation, not
>> aggregate for all of them.
> 
> Aha.  Thanks.

Attached is a revised patch that implements just -Wmismatched-tags
without the other two warnings (-Wclass-not-pod and -Wstruct-is-pod).
It also implements the optimizations mentioned above.  To make it
easier to do the tag cleanup by simply dropping the class-key when
it isn't necessary (suggested during the review of the cleanup patch)
I added another warning: -Wredundant-tags to point out instances
where the class-key or enum-key can safely be dropped. Both warnings
are off by default.

With the optimizations in place, the biggest space overhead of
using the option I measured in a GCC build was 990 elements of
the record_to_locs_t hash table, plus 2756 elements of the locvec
vector.  In LP64, each record_to_locs_t element type is 16 bytes
and each element of the locvec vector is 24 bytes, so the maximum
space overhead is on the order of 80K.  The average overhead per
GCC translation unit was about 30K.

The patch depends on fixes for a few bugs in GCC hash_tables (PR
92761 and 92762).  I will post those separately.

Martin

PS Independently of this patch I will propose updating the GCC
Coding Conventions to remove the guideline to use the struct
class-key for PODs and class for non-PODs:
   https://gcc.gnu.org/codingconventions.html#Class_Use

Comments

Jason Merrill Dec. 4, 2019, 11:37 p.m. UTC | #1
On 12/3/19 4:49 PM, Martin Sebor wrote:
> On 8/5/19 4:30 PM, Jason Merrill wrote:
>> On Mon, Aug 5, 2019 at 5:50 PM Martin Sebor <msebor@gmail.com> wrote:
>>>
>>> On 8/5/19 1:25 PM, Jason Merrill wrote:
>>>> On 8/1/19 7:35 PM, Martin Sebor wrote:
>>>>> On 8/1/19 12:09 PM, Jason Merrill wrote:
>>>>>> On 7/22/19 12:34 PM, Martin Sebor wrote:
>>>>>>> Ping: https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00622.html
> ...
>>>>>> -Wmismatched-tags is useful to have, given the MSVC/clang situation,
>>>>>> but I wonder about memory consumption from all the record keeping.
>>>>>> Do you have any overhead measurements?
>>>>>
>>>>> I did think about the overhead but not knowing if the patch would
>>>>> even be considered I didn't spend time optimizing it.
>>>>>
>>>>> In an instrumented build of GCC with the patch I just did I have
>>>>> collected the following stats for the number of elements in
>>>>> the rec2loc hash table (for 998 files):
>>>>>
>>>>>     rec2loc elements   locvec elements
>>>>>     min:           0                 0
>>>>>     max:       2,785             3,303
>>>>>     mean:        537             1,043
>>>>>     median:      526             1,080
>>>>>
>>>>> The locvec data are based on totals for the whole hash table, so
>>>>> in the worst case the hash_map has 2,785 elements, and the sum of
>>>>> all locvec lengths in all those elements is 3,303.  Each rec2loc
>>>>> element takes up 16 bytes, plus the size of the locvec data.
>>>>>
>>>>> If my math is right (which doesn't happen too often) in the worst
>>>>> case the bookkeeping overhead is 43KB for the hash_map plus on
>>>>> the order of 140KB for the vectors (just doubling the number of
>>>>> elements to account for capacity = 2 X size, times 24 for
>>>>> the flag_func_loc_t element type).  So 183K in the worst case
>>>>> in GCC.
>>>>
>>>> 183k cumulative over all the GCC sources doesn't sound excessive, 
>>>> but...
>>>>
>>>>> There are a few ways to reduce that if it seems excessive.
>>>>>
>>>>> One is by avoiding some waste in flag_func_loc_t which is
>>>>>
>>>>>     pair<tag_types, pair<bool, pair<tree, location_t>>>
>>>>>
>>>>> tree could come first and tag_types and the bool could share
>>>>> space.  That should bring it down to 16 in LP64, for about
>>>>> 30% off the 183K.
>>>>>
>>>>> Beyond that, we could change the algorithm to discard records
>>>>> for prior declarations after the first definition has been seen
>>>>> (and any mismatches diagnosed).
>>>>>
>>>>> We could also only collect one record for each definition
>>>>> in system headers rather than one for every declaration and
>>>>> reference.
>>>>
>>>> ...these all sound worthwhile.
>>>
>>> Okay, I'll look into it.
>>>
>>> To be clear: it was 183K maximum for a single compilation, not
>>> aggregate for all of them.
>>
>> Aha.  Thanks.
> 
> Attached is a revised patch that implements just -Wmismatched-tags
> without the other two warnings (-Wclass-not-pod and -Wstruct-is-pod).
> It also implements the optimizations mentioned above.  To make it
> easier to do the tag cleanup by simply dropping the class-key when
> it isn't necessary (suggested during the review of the cleanup patch)
> I added another warning: -Wredundant-tags to point out instances
> where the class-key or enum-key can safely be dropped. Both warnings
> are off by default.
> 
> With the optimizations in place, the biggest space overhead of
> using the option I measured in a GCC build was 990 elements of
> the record_to_locs_t hash table, plus 2756 elements of the locvec
> vector.  In LP64, each record_to_locs_t element type is 16 bytes
> and each element of the locvec vector is 24 bytes, so the maximum
> space overhead is on the order of 80K.  The average overhead per
> GCC translation unit was about 30K.
> 
> The patch depends on fixes for a few bugs in GCC hash_tables (PR
> 92761 and 92762).  I will post those separately.
> 
> Martin
> 
> PS Independently of this patch I will propose updating the GCC
> Coding Conventions to remove the guideline to use the struct
> class-key for PODs and class for non-PODs:
>    https://gcc.gnu.org/codingconventions.html#Class_Use

> +class rec_decl_loc_t

Let's use "class" instead of "record" generally in this patch.

> +/* A mapping between a TYPE_DECL for a class and the rec_decl_loc_t
> +   description above.  */
> +typedef hash_map<tree, rec_decl_loc_t> record_to_locs_t;

You might want to use hash_map<tree_decl_hash, rec_decl_loc_t> so you 
get the decl-specific hash function rather than the generic pointer hash.

It's hard to distinguish between this type and the previous one by name; 
this one should probably have "map" in its name.

> +static GTY (()) record_to_locs_t *rec2loc;
...
> +    rec2loc = new record_to_locs_t ();

If this isn't GC-allocated, marking it with GTY(()) seems wrong.  How do 
you imagine this warning interacting with PCH?

> +  /* A prior declaration of TYPE_DECL has been seen.  */

The rest of this function from this point on seems like it would be 
better as a member function of rec_decl_loc_t.

> +static void
> +diag_mismatched_tags (tree type_decl, rec_decl_loc_t &recloc)

This could also be a member function.

> +  if (rdl->idxdef < UINT_MAX && rdl->def_class_key == class_key)

Maybe != rather than < ?

> +@item -Wmismatched-tags @r{(C++ and Objective-C++ only)}

The documentation of this flag should mention that the main reason 
someone might want to use it is for compatibility with MSVC++, where 
struct and class are improperly mangled differently.

Jason
Martin Sebor Dec. 5, 2019, 11:33 p.m. UTC | #2
On 12/4/19 4:37 PM, Jason Merrill wrote:
> On 12/3/19 4:49 PM, Martin Sebor wrote:
>> On 8/5/19 4:30 PM, Jason Merrill wrote:
>>> On Mon, Aug 5, 2019 at 5:50 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>
>>>> On 8/5/19 1:25 PM, Jason Merrill wrote:
>>>>> On 8/1/19 7:35 PM, Martin Sebor wrote:
>>>>>> On 8/1/19 12:09 PM, Jason Merrill wrote:
>>>>>>> On 7/22/19 12:34 PM, Martin Sebor wrote:
>>>>>>>> Ping: https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00622.html
>> ...
>>>>>>> -Wmismatched-tags is useful to have, given the MSVC/clang situation,
>>>>>>> but I wonder about memory consumption from all the record keeping.
>>>>>>> Do you have any overhead measurements?
>>>>>>
>>>>>> I did think about the overhead but not knowing if the patch would
>>>>>> even be considered I didn't spend time optimizing it.
>>>>>>
>>>>>> In an instrumented build of GCC with the patch I just did I have
>>>>>> collected the following stats for the number of elements in
>>>>>> the rec2loc hash table (for 998 files):
>>>>>>
>>>>>>     rec2loc elements   locvec elements
>>>>>>     min:           0                 0
>>>>>>     max:       2,785             3,303
>>>>>>     mean:        537             1,043
>>>>>>     median:      526             1,080
>>>>>>
>>>>>> The locvec data are based on totals for the whole hash table, so
>>>>>> in the worst case the hash_map has 2,785 elements, and the sum of
>>>>>> all locvec lengths in all those elements is 3,303.  Each rec2loc
>>>>>> element takes up 16 bytes, plus the size of the locvec data.
>>>>>>
>>>>>> If my math is right (which doesn't happen too often) in the worst
>>>>>> case the bookkeeping overhead is 43KB for the hash_map plus on
>>>>>> the order of 140KB for the vectors (just doubling the number of
>>>>>> elements to account for capacity = 2 X size, times 24 for
>>>>>> the flag_func_loc_t element type).  So 183K in the worst case
>>>>>> in GCC.
>>>>>
>>>>> 183k cumulative over all the GCC sources doesn't sound excessive, 
>>>>> but...
>>>>>
>>>>>> There are a few ways to reduce that if it seems excessive.
>>>>>>
>>>>>> One is by avoiding some waste in flag_func_loc_t which is
>>>>>>
>>>>>>     pair<tag_types, pair<bool, pair<tree, location_t>>>
>>>>>>
>>>>>> tree could come first and tag_types and the bool could share
>>>>>> space.  That should bring it down to 16 in LP64, for about
>>>>>> 30% off the 183K.
>>>>>>
>>>>>> Beyond that, we could change the algorithm to discard records
>>>>>> for prior declarations after the first definition has been seen
>>>>>> (and any mismatches diagnosed).
>>>>>>
>>>>>> We could also only collect one record for each definition
>>>>>> in system headers rather than one for every declaration and
>>>>>> reference.
>>>>>
>>>>> ...these all sound worthwhile.
>>>>
>>>> Okay, I'll look into it.
>>>>
>>>> To be clear: it was 183K maximum for a single compilation, not
>>>> aggregate for all of them.
>>>
>>> Aha.  Thanks.
>>
>> Attached is a revised patch that implements just -Wmismatched-tags
>> without the other two warnings (-Wclass-not-pod and -Wstruct-is-pod).
>> It also implements the optimizations mentioned above.  To make it
>> easier to do the tag cleanup by simply dropping the class-key when
>> it isn't necessary (suggested during the review of the cleanup patch)
>> I added another warning: -Wredundant-tags to point out instances
>> where the class-key or enum-key can safely be dropped. Both warnings
>> are off by default.
>>
>> With the optimizations in place, the biggest space overhead of
>> using the option I measured in a GCC build was 990 elements of
>> the record_to_locs_t hash table, plus 2756 elements of the locvec
>> vector.  In LP64, each record_to_locs_t element type is 16 bytes
>> and each element of the locvec vector is 24 bytes, so the maximum
>> space overhead is on the order of 80K.  The average overhead per
>> GCC translation unit was about 30K.
>>
>> The patch depends on fixes for a few bugs in GCC hash_tables (PR
>> 92761 and 92762).  I will post those separately.
>>
>> Martin
>>
>> PS Independently of this patch I will propose updating the GCC
>> Coding Conventions to remove the guideline to use the struct
>> class-key for PODs and class for non-PODs:
>>    https://gcc.gnu.org/codingconventions.html#Class_Use
> 
>> +class rec_decl_loc_t
> 
> Let's use "class" instead of "record" generally in this patch.

Okay.

> 
>> +/* A mapping between a TYPE_DECL for a class and the rec_decl_loc_t
>> +   description above.  */
>> +typedef hash_map<tree, rec_decl_loc_t> record_to_locs_t;
> 
> You might want to use hash_map<tree_decl_hash, rec_decl_loc_t> so you 
> get the decl-specific hash function rather than the generic pointer hash.

Done.

> 
> It's hard to distinguish between this type and the previous one by name; 
> this one should probably have "map" in its name.
> 
>> +static GTY (()) record_to_locs_t *rec2loc;
> ...
>> +    rec2loc = new record_to_locs_t ();
> 
> If this isn't GC-allocated, marking it with GTY(()) seems wrong.  How do 
> you imagine this warning interacting with PCH?

I have to confess I know too little about PCH to have an idea how
it might interact.  Is there something you suggest I try testing?

> 
>> +  /* A prior declaration of TYPE_DECL has been seen.  */
> 
> The rest of this function from this point on seems like it would be 
> better as a member function of rec_decl_loc_t.
> 
>> +static void
>> +diag_mismatched_tags (tree type_decl, rec_decl_loc_t &recloc)
> 
> This could also be a member function.

I like that idea.  I've made more use of member functions in
the revised patch.  That let me make the implementation details
of the new class private.  (I'd like it even better if I could
move it into a file of its own, and avoid growing the already
sizable parser.c even more.)

> 
>> +  if (rdl->idxdef < UINT_MAX && rdl->def_class_key == class_key)
> 
> Maybe != rather than < ?
> 
>> +@item -Wmismatched-tags @r{(C++ and Objective-C++ only)}
> 
> The documentation of this flag should mention that the main reason 
> someone might want to use it is for compatibility with MSVC++, where 
> struct and class are improperly mangled differently.

I've added a sentence about the mangling.

Attached is the revised patch.

Martin
Jakub Jelinek Dec. 5, 2019, 11:47 p.m. UTC | #3
On Thu, Dec 05, 2019 at 04:33:10PM -0700, Martin Sebor wrote:
> > It's hard to distinguish between this type and the previous one by name;
> > this one should probably have "map" in its name.
> > 
> > > +static GTY (()) record_to_locs_t *rec2loc;
> > ...
> > > +    rec2loc = new record_to_locs_t ();
> > 
> > If this isn't GC-allocated, marking it with GTY(()) seems wrong.  How do
> > you imagine this warning interacting with PCH?
> 
> I have to confess I know too little about PCH to have an idea how
> it might interact.  Is there something you suggest I try testing?

For your patch, obviously some struct/class forward declarations or
definitions in a header that you compile into PCH and then the main testcase
that contains the mismatched pairs.

If there is something that you need to record during parsing of the
precompiled header and use later on, everything needs to be GGC allocated.
So, the hash_map needs to be created with something like
hash_map<something, something_else>::create_ggc (nnn)
and it really can't use pointer hashing, but has to use some different one
(say on DECL_UID, TYPE_UID etc.), because the addresses are remapped during
PCH save/restore cycle, but hash tables aren't rehashed.
See e.g. PR92458.

	Jakub
Jason Merrill Dec. 6, 2019, 7:08 p.m. UTC | #4
On 12/5/19 6:47 PM, Jakub Jelinek wrote:
> On Thu, Dec 05, 2019 at 04:33:10PM -0700, Martin Sebor wrote:
>>> It's hard to distinguish between this type and the previous one by name;
>>> this one should probably have "map" in its name.
>>>
>>>> +static GTY (()) record_to_locs_t *rec2loc;
>>> ...
>>>> +    rec2loc = new record_to_locs_t ();
>>>
>>> If this isn't GC-allocated, marking it with GTY(()) seems wrong.  How do
>>> you imagine this warning interacting with PCH?
>>
>> I have to confess I know too little about PCH to have an idea how
>> it might interact.  Is there something you suggest I try testing?
> 
> For your patch, obviously some struct/class forward declarations or
> definitions in a header that you compile into PCH and then the main testcase
> that contains the mismatched pairs.
> 
> If there is something that you need to record during parsing of the
> precompiled header and use later on, everything needs to be GGC allocated.
> So, the hash_map needs to be created with something like
> hash_map<something, something_else>::create_ggc (nnn)
> and it really can't use pointer hashing, but has to use some different one
> (say on DECL_UID, TYPE_UID etc.), because the addresses are remapped during
> PCH save/restore cycle, but hash tables aren't rehashed.
> See e.g. PR92458.

Alternately you can decide that this information will not be saved to 
PCH, and rely on CLASSTYPE_DECLARED_CLASS for classes loaded from a PCH.

Jason
Martin Sebor Dec. 10, 2019, 12:29 a.m. UTC | #5
On 12/6/19 12:08 PM, Jason Merrill wrote:
> On 12/5/19 6:47 PM, Jakub Jelinek wrote:
>> On Thu, Dec 05, 2019 at 04:33:10PM -0700, Martin Sebor wrote:
>>>> It's hard to distinguish between this type and the previous one by 
>>>> name;
>>>> this one should probably have "map" in its name.
>>>>
>>>>> +static GTY (()) record_to_locs_t *rec2loc;
>>>> ...
>>>>> +    rec2loc = new record_to_locs_t ();
>>>>
>>>> If this isn't GC-allocated, marking it with GTY(()) seems wrong.  
>>>> How do
>>>> you imagine this warning interacting with PCH?
>>>
>>> I have to confess I know too little about PCH to have an idea how
>>> it might interact.  Is there something you suggest I try testing?
>>
>> For your patch, obviously some struct/class forward declarations or
>> definitions in a header that you compile into PCH and then the main 
>> testcase
>> that contains the mismatched pairs.
>>
>> If there is something that you need to record during parsing of the
>> precompiled header and use later on, everything needs to be GGC 
>> allocated.
>> So, the hash_map needs to be created with something like
>> hash_map<something, something_else>::create_ggc (nnn)
>> and it really can't use pointer hashing, but has to use some different 
>> one
>> (say on DECL_UID, TYPE_UID etc.), because the addresses are remapped 
>> during
>> PCH save/restore cycle, but hash tables aren't rehashed.
>> See e.g. PR92458.
> 
> Alternately you can decide that this information will not be saved to 
> PCH, and rely on CLASSTYPE_DECLARED_CLASS for classes loaded from a PCH.

This seems like the right approach to me.  Mismatches in
a precompiled header should be diagnosed when the header is
being compiled, so the only ones involving its uses should
be between classes defined in it and declared or referenced
outside it.  I've implemented this in the attached revision.

Martin
Martin Sebor Dec. 16, 2019, 4:27 p.m. UTC | #6
Ping: https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00642.html

Jason, I'm on PTO until 1/6 starting this Thursday, with no
connectivity.  If there are any further changes to make to
the patch I will need to make them before then, or otherwise
after I get back in January.

On 12/9/19 5:29 PM, Martin Sebor wrote:
> On 12/6/19 12:08 PM, Jason Merrill wrote:
>> On 12/5/19 6:47 PM, Jakub Jelinek wrote:
>>> On Thu, Dec 05, 2019 at 04:33:10PM -0700, Martin Sebor wrote:
>>>>> It's hard to distinguish between this type and the previous one by 
>>>>> name;
>>>>> this one should probably have "map" in its name.
>>>>>
>>>>>> +static GTY (()) record_to_locs_t *rec2loc;
>>>>> ...
>>>>>> +    rec2loc = new record_to_locs_t ();
>>>>>
>>>>> If this isn't GC-allocated, marking it with GTY(()) seems wrong. 
>>>>> How do
>>>>> you imagine this warning interacting with PCH?
>>>>
>>>> I have to confess I know too little about PCH to have an idea how
>>>> it might interact.  Is there something you suggest I try testing?
>>>
>>> For your patch, obviously some struct/class forward declarations or
>>> definitions in a header that you compile into PCH and then the main 
>>> testcase
>>> that contains the mismatched pairs.
>>>
>>> If there is something that you need to record during parsing of the
>>> precompiled header and use later on, everything needs to be GGC 
>>> allocated.
>>> So, the hash_map needs to be created with something like
>>> hash_map<something, something_else>::create_ggc (nnn)
>>> and it really can't use pointer hashing, but has to use some 
>>> different one
>>> (say on DECL_UID, TYPE_UID etc.), because the addresses are remapped 
>>> during
>>> PCH save/restore cycle, but hash tables aren't rehashed.
>>> See e.g. PR92458.
>>
>> Alternately you can decide that this information will not be saved to 
>> PCH, and rely on CLASSTYPE_DECLARED_CLASS for classes loaded from a PCH.
> 
> This seems like the right approach to me.  Mismatches in
> a precompiled header should be diagnosed when the header is
> being compiled, so the only ones involving its uses should
> be between classes defined in it and declared or referenced
> outside it.  I've implemented this in the attached revision.
> 
> Martin
Jason Merrill Dec. 16, 2019, 10:51 p.m. UTC | #7
On 12/9/19 7:29 PM, Martin Sebor wrote:

Just a few nits:

> +/* A mapping between a TYPE_DECL for a class and the class_decl_loc_t
> +   description above.  */
> +typedef hash_map<tree_decl_hash, class_decl_loc_t> class_to_loc_map_t;
> +static class_to_loc_map_t class2loc;

I think we can make these members of class_decl_loc_t, now that we 
aren't marking them with GTY.

> +  class_decl_loc_t *rdl = class2loc.get (type_decl);
> +  if (!rdl)
> +    {
> +      tree type = TREE_TYPE (type_decl);
> +      if (def_p || !COMPLETE_TYPE_P (type))
> +	{
> +	  /* TYPE_DECL is the first declaration or definition of the type
> +	     (outside precompiled headers -- see below).  Just create
> +	     a new entry for it.  */
> +	  class2loc.put (type_decl, class_decl_loc_t (class_key, false, def_p));
> +	  return;
> +	}
> +
> +      /* TYPE was previously defined in some unknown precompiled hdeader.
> +	 Simply add a record of its definition at an unknown location and
> +	 proceed below to add a reference to it at the current location.
> +	 (Declarations in precompiled headers that are not definitions
> +	 are ignored.)  */
> +      tag_types def_key
> +	= CLASSTYPE_DECLARED_CLASS (type) ? class_type : record_type;
> +      location_t def_loc = DECL_SOURCE_LOCATION (type_decl);
> +      rdl = &class2loc.get_or_insert (type_decl);
> +      *rdl = class_decl_loc_t (def_key, false, true, def_loc);
> +    }

It seems that you could use get_or_insert at the top, since you always 
end up creating an element if there wasn't one already.

> +  /* A prior declaration of TYPE_DECL has been seen.  */
> +
> +  if (rdl->idxdef != UINT_MAX && rdl->def_class_key == class_key)
> +    /* Do nothing if the class-key in this declaration matches
> +       the definition.  */
> +    return;
...

I still think that the rest of this function could be a non-static 
member function to avoid so many "rdl->".

Jason
Martin Sebor Dec. 16, 2019, 11:31 p.m. UTC | #8
On 12/16/19 3:51 PM, Jason Merrill wrote:
> On 12/9/19 7:29 PM, Martin Sebor wrote:
> 
> Just a few nits:
> 
>> +/* A mapping between a TYPE_DECL for a class and the class_decl_loc_t
>> +   description above.  */
>> +typedef hash_map<tree_decl_hash, class_decl_loc_t> class_to_loc_map_t;
>> +static class_to_loc_map_t class2loc;
> 
> I think we can make these members of class_decl_loc_t, now that we 
> aren't marking them with GTY.
> 
>> +  class_decl_loc_t *rdl = class2loc.get (type_decl);
>> +  if (!rdl)
>> +    {
>> +      tree type = TREE_TYPE (type_decl);
>> +      if (def_p || !COMPLETE_TYPE_P (type))
>> +    {
>> +      /* TYPE_DECL is the first declaration or definition of the type
>> +         (outside precompiled headers -- see below).  Just create
>> +         a new entry for it.  */
>> +      class2loc.put (type_decl, class_decl_loc_t (class_key, false, 
>> def_p));
>> +      return;
>> +    }
>> +
>> +      /* TYPE was previously defined in some unknown precompiled 
>> hdeader.
>> +     Simply add a record of its definition at an unknown location and
>> +     proceed below to add a reference to it at the current location.
>> +     (Declarations in precompiled headers that are not definitions
>> +     are ignored.)  */
>> +      tag_types def_key
>> +    = CLASSTYPE_DECLARED_CLASS (type) ? class_type : record_type;
>> +      location_t def_loc = DECL_SOURCE_LOCATION (type_decl);
>> +      rdl = &class2loc.get_or_insert (type_decl);
>> +      *rdl = class_decl_loc_t (def_key, false, true, def_loc);
>> +    }
> 
> It seems that you could use get_or_insert at the top, since you always 
> end up creating an element if there wasn't one already.
> 
>> +  /* A prior declaration of TYPE_DECL has been seen.  */
>> +
>> +  if (rdl->idxdef != UINT_MAX && rdl->def_class_key == class_key)
>> +    /* Do nothing if the class-key in this declaration matches
>> +       the definition.  */
>> +    return;
> ...
> 
> I still think that the rest of this function could be a non-static 
> member function to avoid so many "rdl->".

Attached is another revision with these changes.

Martin
Jason Merrill Dec. 17, 2019, 7:28 p.m. UTC | #9
On 12/16/19 6:31 PM, Martin Sebor wrote:
> +  class_decl_loc_t *rdl = class2loc.get (type_decl);
> +  if (!rdl)
> +    {
> +      rdl = &class2loc.get_or_insert (type_decl);

I was thinking

class_decl_loc_t *rdl = &class2loc.get_or_insert (type_decl);

OK with that change.

Jason
Stephan Bergmann Feb. 18, 2020, 8:41 a.m. UTC | #10
On 03/12/2019 22:49, Martin Sebor wrote:
> I added another warning: -Wredundant-tags to point out instances
> where the class-key or enum-key can safely be dropped. Both warnings
> are off by default.

I think -Wredundant-tags would be more useful if it did not warn about 
occurrences within extern "C" (at least not within included files), as a 
heuristic to filter out code that is shared between C and C++ and where 
the tags are thus necessary.
Martin Sebor Feb. 18, 2020, 3:57 p.m. UTC | #11
On 2/18/20 1:41 AM, Stephan Bergmann wrote:
> On 03/12/2019 22:49, Martin Sebor wrote:
>> I added another warning: -Wredundant-tags to point out instances
>> where the class-key or enum-key can safely be dropped. Both warnings
>> are off by default.
> 
> I think -Wredundant-tags would be more useful if it did not warn about 
> occurrences within extern "C" (at least not within included files), as a 
> heuristic to filter out code that is shared between C and C++ and where 
> the tags are thus necessary.

I agree.  Exempting C code in headers does seem like the right thing
to do.  I opened bug 93804 to remember to get back to it for GCC 11.

Thanks
Martin
diff mbox series

Patch

PR c++/61339 - add warning for mismatch between struct and class

gcc/c-family/ChangeLog:

	PR c++/61339
	* c.opt (-Wmismatched-tags, -Wredundant-tags): New options.

gcc/cp/ChangeLog:

	PR c++/61339
	* parser.c (cp_parser_maybe_warn_enum_key): New function.
	(rec_decl_loc_t): New class.
	(record_to_locs_t): New typedef.
	(rec2loc): New global variable.
	(cp_parser_elaborated_type_specifier): Call it.
	(cp_parser_class_head): Call cp_parser_check_class_key.
	(cp_parser_check_class_key): Add arguments.
	(diag_mismatched_tags): New function.
	(c_parse_file): Call diag_mismatched_tags.

gcc/testsuite/ChangeLog:

	PR c++/61339
	* g++.dg/warn/Wmismatched-tags.C: New test.
	* g++.dg/warn/Wredundant-tags.C: New test.

gcc/ChangeLog:

	PR c++/61339
	* doc/invoke.texi (-Wmismatched-tags, -Wredundant-tags): Document
	new C++ options.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 914a2f0ef44..4f3d3cf0d43 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -755,6 +755,10 @@  Wmisleading-indentation
 C C++ Common Var(warn_misleading_indentation) Warning LangEnabledBy(C C++,Wall)
 Warn when the indentation of the code does not reflect the block structure.
 
+Wmismatched-tags
+C++ Objc++ Var(warn_mismatched_tags) Warning
+Warn when a class is redeclared or referenced using a mismatched class-key.
+
 Wmissing-braces
 C ObjC C++ ObjC++ Var(warn_missing_braces) Warning LangEnabledBy(C ObjC,Wall)
 Warn about possibly missing braces around initializers.
@@ -783,6 +787,10 @@  Wpacked-not-aligned
 C ObjC C++ ObjC++ Var(warn_packed_not_aligned) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
 Warn when fields in a struct with the packed attribute are misaligned.
 
+Wredundant-tags
+C++ Objc++ Var(warn_redundant_tags) Warning
+Warn when a class or enumerated type is referenced using a redundant class-key.
+
 Wsized-deallocation
 C++ ObjC++ Var(warn_sized_deallocation) Warning EnabledBy(Wextra)
 Warn about missing sized deallocation functions.
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index fb030022627..da9aa42e7a8 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -2599,8 +2599,9 @@  static enum tag_types cp_parser_token_is_class_key
   (cp_token *);
 static enum tag_types cp_parser_token_is_type_parameter_key
   (cp_token *);
+static void cp_parser_maybe_warn_enum_key (cp_parser *, location_t, tree, rid);
 static void cp_parser_check_class_key
-  (enum tag_types, tree type);
+(cp_parser *, location_t, enum tag_types, tree type, bool, bool);
 static void cp_parser_check_access_in_redeclaration
   (tree type, location_t location);
 static bool cp_parser_optional_template_keyword
@@ -18480,6 +18481,11 @@  cp_parser_elaborated_type_specifier (cp_parser* parser,
   tree globalscope;
   cp_token *token = NULL;
 
+  /* For class and enum types the location of the class-key or enum-key.  */
+  location_t key_loc = cp_lexer_peek_token (parser->lexer)->location;
+  /* For a scoped enum, the 'class' or 'struct' keyword id.  */
+  rid scoped_key = RID_MAX;
+
   /* See if we're looking at the `enum' keyword.  */
   if (cp_lexer_next_token_is_keyword (parser->lexer, RID_ENUM))
     {
@@ -18490,10 +18496,11 @@  cp_parser_elaborated_type_specifier (cp_parser* parser,
       /* Issue a warning if the `struct' or `class' key (for C++0x scoped
 	 enums) is used here.  */
       cp_token *token = cp_lexer_peek_token (parser->lexer);
-      if (cp_parser_is_keyword (token, RID_CLASS)
-	  || cp_parser_is_keyword (token, RID_STRUCT))
+      if (cp_parser_is_keyword (token, scoped_key = RID_CLASS)
+	  || cp_parser_is_keyword (token, scoped_key = RID_STRUCT))
 	{
-	  gcc_rich_location richloc (token->location);
+	  location_t loc = token->location;
+	  gcc_rich_location richloc (loc);
 	  richloc.add_range (input_location);
 	  richloc.add_fixit_remove ();
 	  pedwarn (&richloc, 0, "elaborated-type-specifier for "
@@ -18501,7 +18508,12 @@  cp_parser_elaborated_type_specifier (cp_parser* parser,
 		   token->u.value);
 	  /* Consume the `struct' or `class' and parse it anyway.  */
 	  cp_lexer_consume_token (parser->lexer);
+	  /* Create a combined location for the whole scoped-enum-key.  */
+	  key_loc = make_location (key_loc, key_loc, loc);
 	}
+      else
+	scoped_key = RID_MAX;
+
       /* Parse the attributes.  */
       attributes = cp_parser_attributes_opt (parser);
     }
@@ -18517,6 +18529,7 @@  cp_parser_elaborated_type_specifier (cp_parser* parser,
   /* Otherwise it must be a class-key.  */
   else
     {
+      key_loc = cp_lexer_peek_token (parser->lexer)->location;
       tag_type = cp_parser_class_key (parser);
       if (tag_type == none_type)
 	return error_mark_node;
@@ -18827,13 +18840,18 @@  cp_parser_elaborated_type_specifier (cp_parser* parser,
 		 "attributes ignored on elaborated-type-specifier that is not a forward declaration");
     }
 
-  if (tag_type != enum_type)
+  if (tag_type == enum_type)
+    cp_parser_maybe_warn_enum_key (parser, key_loc, type, scoped_key);
+  else
     {
+      /* Diagnose class/struct/union mismatches.  */
+      cp_parser_check_class_key (parser, key_loc, tag_type, type, false,
+				 cp_parser_declares_only_class_p (parser));
+
       /* Indicate whether this class was declared as a `class' or as a
 	 `struct'.  */
-      if (CLASS_TYPE_P (type))
+      if (CLASS_TYPE_P (type) && !currently_open_class (type))
 	CLASSTYPE_DECLARED_CLASS (type) = (tag_type == class_type);
-      cp_parser_check_class_key (tag_type, type);
     }
 
   /* A "<" cannot follow an elaborated type specifier.  If that
@@ -24371,11 +24389,14 @@  cp_parser_class_head (cp_parser* parser,
 		       parser->num_template_parameter_lists);
     }
 
+  /* Diagnose class/struct/union mismatches.  */
+  cp_parser_check_class_key (parser, UNKNOWN_LOCATION, class_key, type,
+			     true, true);
+
   /* Indicate whether this class was declared as a `class' or as a
      `struct'.  */
   if (TREE_CODE (type) == RECORD_TYPE)
-    CLASSTYPE_DECLARED_CLASS (type) = (class_key == class_type);
-  cp_parser_check_class_key (class_key, type);
+    CLASSTYPE_DECLARED_CLASS (type) = class_key == class_type;
 
   /* If this type was already complete, and we see another definition,
      that's an error.  Likewise if the type is already being defined:
@@ -30598,14 +30619,154 @@  cp_parser_token_is_type_parameter_key (cp_token* token)
     }
 }
 
-/* Issue an error message if the CLASS_KEY does not match the TYPE.  */
+/* Diagnose redundant enum-keys.  */
+
+static void
+cp_parser_maybe_warn_enum_key (cp_parser *parser, location_t key_loc,
+			       tree type, rid scoped_key)
+{
+  tree type_decl = TYPE_MAIN_DECL (type);
+  tree name = DECL_NAME (type_decl);
+  /* Look up the NAME to see if it unambiguously refers to the TYPE
+     and set KEY_REDUNDANT if so.  */
+  tree decl = cp_parser_lookup_name_simple (parser, name, input_location);
+
+  /* The enum-key is redundant for uses of the TYPE that are not
+     declarations and for which name lookup returns just the type
+     itself.  */
+  if (decl == type_decl)
+    {
+      gcc_rich_location richloc (key_loc);
+      richloc.add_fixit_remove (key_loc);
+      warning_at (&richloc, OPT_Wredundant_tags,
+		  "redundant enum-key %<enum%s%> in reference to %q#T",
+		  (scoped_key == RID_CLASS ? " class"
+		   : scoped_key == RID_STRUCT ? " struct" : ""), type);
+    }
+}
+
+/* Describes the set of declarations of a struct, class, or class template
+   or its specializations.  Used for -Wmismatched-tags.  */
+
+class rec_decl_loc_t
+{
+ public:
+
+  rec_decl_loc_t ()
+    : locvec (), idxdef (), def_class_key ()
+  {
+    locvec.create (4);
+  }
+
+  /* Constructs an object for a single declaration of a class with
+     CLASS_KEY at the current location in the current function (or
+     at another scope).  KEY_REDUNDANT is true if the class-key may
+     be omitted in the current context without an ambiguity with
+     another symbol with the same name.
+     DEF_P is true for a class declaration that is a definition.  */
+  rec_decl_loc_t (tag_types class_key, bool key_redundant, bool def_p)
+    : locvec (), idxdef (def_p ? 0 : UINT_MAX), def_class_key (class_key)
+  {
+    locvec.create (4);
+    class_key_loc_t ckl (current_function_decl, input_location,
+			 class_key, key_redundant);
+    locvec.quick_push (ckl);
+  }
+
+  /* Copy, assign, and destroy the object.  Necessary because LOCVEC
+     isn't safely copyable and assignable and doesn't release storage
+     on its own.  */
+  rec_decl_loc_t (const rec_decl_loc_t &rhs)
+    : locvec (rhs.locvec.copy ()), idxdef (rhs.idxdef),
+      def_class_key (rhs.def_class_key)
+  { }
+
+  rec_decl_loc_t& operator= (const rec_decl_loc_t &rhs)
+  {
+    if (this == &rhs)
+      return *this;
+    locvec.release ();
+    locvec = rhs.locvec.copy ();
+    idxdef = rhs.idxdef;
+    def_class_key = rhs.def_class_key;
+    return *this;
+  }
+
+  ~rec_decl_loc_t ()
+  {
+    locvec.release ();
+  }
+
+  tree function (unsigned i) const
+  {
+    return locvec[i].func;
+  }
+
+  location_t location (unsigned i) const
+  {
+    return locvec[i].loc;
+  }
+
+  bool key_redundant (unsigned i) const
+  {
+    return locvec[i].key_redundant;
+  }
+
+  tag_types class_key (unsigned i) const
+  {
+    return locvec[i].class_key;
+  }
+
+  /* The location of a single mention of a class type with the given
+     class-key.  */
+  struct class_key_loc_t
+  {
+    class_key_loc_t (tree func, location_t loc, tag_types key, bool redundant)
+      : func (func), loc (loc), class_key (key), key_redundant (redundant) { }
+
+    /* The function the type is mentioned in.  */
+    tree func;
+    /* The exact location.  */
+    location_t loc;
+    /* The class-key used in the mention of the type.  */
+    tag_types class_key;
+    /* True when the class-key could be omitted at this location
+       without an ambiguity with another symbol of the same name.  */
+    bool key_redundant;
+  };
+  /* Avoid using auto_vec here since it's not safe to copy due to pr90904.  */
+  vec <class_key_loc_t> locvec;
+  /* LOCVEC index of the definition or UINT_MAX if none exists.  */
+  unsigned idxdef;
+  /* The class-key the class was last declared with or none_type when
+     it has been declared with a mismatched key.  */
+  tag_types def_class_key;
+};
+
+
+/* A mapping between a TYPE_DECL for a class and the rec_decl_loc_t
+   description above.  */
+typedef hash_map<tree, rec_decl_loc_t> record_to_locs_t;
+static GTY (()) record_to_locs_t *rec2loc;
 
 static void
-cp_parser_check_class_key (enum tag_types class_key, tree type)
+diag_mismatched_tags (tree, rec_decl_loc_t &);
+
+/* Issue an error message if the CLASS_KEY does not match the TYPE.
+   DEF_P is expected to be set for a definition of class TYPE.  DECL_P
+   is set for a declaration of class TYPE and clear for a reference to
+   it that is not a declaration of it.  */
+
+static void
+cp_parser_check_class_key (cp_parser *parser, location_t key_loc,
+			   tag_types class_key, tree type, bool def_p,
+			   bool decl_p)
 {
   if (type == error_mark_node)
     return;
-  if ((TREE_CODE (type) == UNION_TYPE) != (class_key == union_type))
+
+  bool seen_as_union = TREE_CODE (type) == UNION_TYPE;
+  if (seen_as_union != (class_key == union_type))
     {
       if (permerror (input_location, "%qs tag used in naming %q#T",
 		     class_key == union_type ? "union"
@@ -30613,7 +30774,212 @@  cp_parser_check_class_key (enum tag_types class_key, tree type)
 		     type))
 	inform (DECL_SOURCE_LOCATION (TYPE_NAME (type)),
 		"%q#T was previously declared here", type);
+      return;
+    }
+
+  if (!warn_mismatched_tags && !warn_redundant_tags)
+    return;
+
+  tree type_decl = TYPE_MAIN_DECL (type);
+  tree name = DECL_NAME (type_decl);
+  /* Look up the NAME to see if it unambiguously refers to the TYPE
+     and set KEY_REDUNDANT if so.  */
+  tree decl = cp_parser_lookup_name_simple (parser, name, input_location);
+
+  /* The class-key is redundant for uses of the CLASS_TYPE that are
+     neither definitions of it nor declarations, and for which name
+     lookup returns just the type itself.  */
+  bool key_redundant = !def_p && !decl_p && decl == type_decl;
+  if (key_redundant)
+    {
+      gcc_rich_location richloc (key_loc);
+      richloc.add_fixit_remove (key_loc);
+      warning_at (&richloc, OPT_Wredundant_tags,
+		"redundant class-key %qs in reference to %q#T",
+		class_key == union_type ? "union"
+		: class_key == record_type ? "struct" : "class",
+		type);
+    }
+
+  if (seen_as_union || !warn_mismatched_tags)
+    return;
+
+  if (!rec2loc)
+    rec2loc = new record_to_locs_t ();
+
+  rec_decl_loc_t *rdl = rec2loc->get (type_decl);
+  if (!rdl)
+    {
+      /* TYPE_DECL is the first declaration of the type.  Just create
+	 a new entry for it.  */
+      rec2loc->put (type_decl, rec_decl_loc_t (class_key, false, def_p));
+      return;
+    }
+
+  /* A prior declaration of TYPE_DECL has been seen.  */
+
+  if (rdl->idxdef < UINT_MAX && rdl->def_class_key == class_key)
+    /* Do nothing if the class-key in this declaration matches
+       the definition.  */
+    return;
+
+  /* Reset the CLASS_KEY associated with this type on mismatch.
+     This is an optimization that lets the diagnostic code skip
+     over classes that use the same class-key in all declarations.  */
+  if (rdl->def_class_key != class_key)
+    rdl->def_class_key = none_type;
+
+  /* Set IDXDEF to the index of the vector corresponding to
+     the definition.  */
+  if (def_p)
+    rdl->idxdef = rdl->locvec.length ();
+
+  /* Append a record of this declaration to the vector.  */
+  rec_decl_loc_t::class_key_loc_t
+    ckl (current_function_decl, input_location,
+	 class_key, key_redundant);
+  rdl->locvec.safe_push (ckl);
+
+  if (rdl->idxdef < UINT_MAX)
+    {
+      /* As a space optimization diagnose declarations of a class
+	 whose definition has been seen and purge the LOCVEC of
+	 all entries except the definition.  */
+      diag_mismatched_tags (decl, *rdl);
+      if (rdl->idxdef)
+	{
+	  rec_decl_loc_t::class_key_loc_t ent = rdl->locvec[rdl->idxdef];
+	  rdl->locvec.release ();
+	  rdl->locvec.reserve (2);
+	  rdl->locvec.safe_push (ent);
+	  rdl->idxdef = 0;
+	}
+      else
+	/* Pop the entry pushed above for this declaration.  */
+	rdl->locvec.pop ();
+    }
+}
+
+/* Issues -Wmismatched-tags for a single class described by RECLOC.  */
+
+static void
+diag_mismatched_tags (tree type_decl, rec_decl_loc_t &recloc)
+{
+  unsigned ndecls = recloc.locvec.length ();
+
+  /* Skip a declaration that consistently uses the same class-key
+     or one with just a solitary declaration (i.e., TYPE_DECL). */
+  if (recloc.def_class_key != none_type || ndecls < 2)
+    return;
+
+  /* Save the current function before changing it below.  */
+  tree save_func = current_function_decl;
+  /* Set if a class definition for RECLOC has been seen.  */
+  bool def_p = recloc.idxdef < ndecls;
+  unsigned idxguide = def_p ? recloc.idxdef : 0;
+  unsigned idx = 0;
+  /* Advance IDX to the first declaration that either is not
+     a definition or that doesn't match the first declaration
+     if no definition is provided.  */
+  while (recloc.class_key (idx) == recloc.class_key (idxguide))
+    if (++idx == ndecls)
+      return;
+
+  /* The class-key the class is expected to be declared with: it's
+     either the key used in its definition or the first declaration
+     if no definition has been provided.  */
+  tag_types xpect_key = recloc.class_key (def_p ? idxguide : 0);
+  const char *xmatchkstr = xpect_key == record_type ? "class" : "struct";
+  const char *xpectkstr = xpect_key == record_type ? "struct" : "class";
+  /* Set the function declaration to print in diagnostic context.  */
+  current_function_decl = recloc.function (idx);
+
+  location_t loc = recloc.location (idx);
+  bool key_redundant = recloc.key_redundant (idx);
+  auto_diagnostic_group d;
+  /* Issue a warning for the first mismatched declaration.
+     Avoid using "%#qT" since the class-key for the same type will
+     be the same regardless of which one was used in the declaraion.  */
+  warning_at (loc, OPT_Wmismatched_tags,
+	      "%qT declared with a mismatched class-key %qs",
+	      type_decl, xmatchkstr);
+
+  /* Suggest how to avoid the warning for each instance since
+     the guidance may be different depending on context.  */
+  inform (loc,
+	  (key_redundant
+	   ? G_("remove the class-key or replace it with %qs")
+	   : G_("replace the class-key with %qs")),
+	  xpectkstr);
+
+  /* Mention the first declaration or definition that guided
+     the decision to issue the warning above.  */
+  tag_types class_key = recloc.class_key (idxguide);
+  inform (recloc.location (idxguide),
+	  (def_p
+	   ? G_("%qT defined as %qs here")
+	   : G_("%qT first declared as %qs here")),
+	  type_decl, xpectkstr);
+
+  /* Issue warnings for the remaining inconsistent declarations.  */
+  for (unsigned i = idx + 1; i != ndecls; ++i)
+    {
+      class_key = recloc.class_key (i);
+      /* Skip over the declarations that match either the definition
+	 if one was provided or the first declaration.  */
+      if (class_key == xpect_key)
+	continue;
+
+      loc = recloc.location (i);
+      key_redundant = recloc.key_redundant (i);
+      /* Set the function declaration to print in diagnostic context.  */
+      current_function_decl = recloc.function (i);
+      warning_at (loc, OPT_Wmismatched_tags,
+		  "%qT declared with a mismatched class-key %qs",
+		  type_decl, xmatchkstr);
+      /* Suggest how to avoid the warning for each instance since
+	 the guidance may be different depending on context.  */
+      inform (loc,
+	      (key_redundant
+	       ? G_("remove the class-key or replace it with %qs")
+	       : G_("replace the class-key with %qs")),
+	      xpectkstr);
     }
+
+  /* Restore the current function in case it was replaced above.  */
+  current_function_decl = save_func;
+}
+
+/* Issues -Wmismatched-tags for all classes.  Called at the end
+   of processing a translation unit, after declarations of all class
+   types and their uses have been recorded.  */
+
+static void
+diag_mismatched_tags ()
+{
+  /* REC2LOC should be empty if -Wmismatched-tags is disabled.  */
+  gcc_assert (warn_mismatched_tags || !rec2loc || rec2loc->is_empty ());
+  if (!rec2loc)
+    return;
+
+  /* Save the current function before changing it below.  It should
+     be null at this point.  */
+  tree save_func = current_function_decl;
+
+  /* Iterate over the collected class/struct declarations.  */
+  typedef record_to_locs_t::iterator iter_t;
+  for (iter_t it = rec2loc->begin (); it != rec2loc->end (); ++it)
+    {
+      tree type_decl = (*it).first;
+      rec_decl_loc_t &recloc = (*it).second;
+      diag_mismatched_tags (type_decl, recloc);
+    }
+
+  rec2loc->empty ();
+  delete rec2loc;
+  rec2loc = NULL;
+  /* Restore the current function.  */
+  current_function_decl = save_func;
 }
 
 /* Issue an error message if DECL is redeclared with different
@@ -43046,6 +43412,8 @@  c_parse_file (void)
   push_deferring_access_checks (flag_access_control
 				? dk_no_deferred : dk_no_check);
   cp_parser_translation_unit (the_parser);
+  diag_mismatched_tags ();
+
   the_parser = NULL;
 
   finish_translation_unit ();
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index d165f31a865..1cd6b7ca8ad 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -236,7 +236,7 @@  in the following sections.
 -Wliteral-suffix @gol
 -Wmultiple-inheritance  -Wno-init-list-lifetime @gol
 -Wnamespaces  -Wnarrowing @gol
--Wpessimizing-move  -Wredundant-move @gol
+-Wpessimizing-move  -Wredundant-move -Wredundant-tags @gol
 -Wnoexcept  -Wnoexcept-type  -Wclass-memaccess @gol
 -Wnon-virtual-dtor  -Wreorder  -Wregister @gol
 -Weffc++  -Wstrict-null-sentinel  -Wtemplates @gol
@@ -3323,6 +3323,21 @@  treats the return value as if it were designated by an rvalue.
 
 This warning is enabled by @option{-Wextra}.
 
+@item -Wredundant-tags @r{(C++ and Objective-C++ only)}
+@opindex Wredundant-tags
+@opindex Wno-redundant-tags
+Warn about redundant class-key and enum-key in references to class types
+and enumerated types in contexts where the key can be eliminated without
+causing an ambiguity.  For example
+
+@smallexample
+struct foo;
+struct foo *p;   // -Wredundant-tags, keyword struct can be eliminated
+
+void foo ();   // "hides" struct foo
+void bar (struct foo&);   // no warning, keyword struct cannot be eliminated
+@end smallexample
+
 @item -fext-numeric-literals @r{(C++ and Objective-C++ only)}
 @opindex fext-numeric-literals
 @opindex fno-ext-numeric-literals
@@ -3458,6 +3473,29 @@  The warning is inactive inside a system header file, such as the STL, so
 one can still use the STL.  One may also instantiate or specialize
 templates.
 
+@item -Wmismatched-tags @r{(C++ and Objective-C++ only)}
+@opindex Wmismatched-tags
+@opindex Wno-mismatched-tags
+Warn for declarations of structs, classes, and class templates and their
+specializations with a class-key that does not match either the definition
+or the first declaration if no definition is provided.
+
+For example, the declaration of @code{struct Object} in the argument list
+of @code{draw} triggers the warning.  To avoid it, either remove the redundant
+class-key @code{struct} or replace it with @code{class} to match its definition.
+@smallexample
+class Object @{
+public:
+  virtual ~Object () = 0;
+@};
+void draw (struct Object*);
+@end smallexample
+
+It is not wrong to declare a class with the class-key @code{struct} as
+the example above shows.  The @option{-Wmismatched-tags} option is intended
+to help achieve a consistent style of class declarations.  It can be used
+either on its own or in conjunction with option @option{-Wredundant-tags}.
+
 @item -Wmultiple-inheritance @r{(C++ and Objective-C++ only)}
 @opindex Wmultiple-inheritance
 @opindex Wno-multiple-inheritance
diff --git a/gcc/testsuite/g++.dg/warn/Wmismatched-tags.C b/gcc/testsuite/g++.dg/warn/Wmismatched-tags.C
new file mode 100644
index 00000000000..af827ff5cec
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wmismatched-tags.C
@@ -0,0 +1,278 @@ 
+/* PR c++/61339 - add mismatch between struct and class
+   Test to verify that -Wmismatched-tags is issued for declarations
+   of the same class using different class-ids.
+   { dg-do compile }
+   { dg-options "-Wmismatched-tags" } */
+
+namespace Classes
+{
+class A;
+class A;
+
+struct B;
+struct B;
+
+union C;
+union C;
+
+struct D;                   // { dg-warning "Classes::D' declared with a mismatched class-key 'struct'" }
+class D { };                // { dg-message "Classes::D' defined as 'class' here" }
+
+class E;                    // { dg-warning "Classes::E' declared with a mismatched class-key 'class'" }
+struct E { };               // { dg-message "Classes::E' defined as 'struct' here" }
+
+class D;
+struct E;
+
+class D;
+struct E;
+
+struct D;                   // { dg-warning "Classes::D' declared with a mismatched class-key" }
+
+class E;                    // { dg-warning "Classes::E' declared with a mismatched class-key" }
+
+class F;                    // { dg-message "Classes::F' first declared as 'class' here" }
+class F;
+
+struct G { };               // { dg-message "Classes::G' defined as 'struct' here" }
+}   // namespace Classes
+
+
+namespace Classes
+{
+class A;
+struct B;
+union C;
+class D;
+struct E;
+
+struct F;                   // { dg-warning "Classes::F' declared with a mismatched class-key" }
+
+struct G;
+}
+
+// Verify that the correct hint is provided, one to remove the class-key
+// when it's redundant, and one to (only) replace it with the correct one
+// when it's needed to disambiguate the reference to the class type.
+namespace RemoveOrReplace
+{
+struct Func;
+class Func;                 // { dg-warning "RemoveOrReplace::Func' declared with a mismatched class-key 'class'" }
+                            // { dg-message "replace the class-key with 'struct'" "hint to remove" { target *-*-* } .-1 }
+
+void Func ();
+
+class Func;                 // { dg-warning "RemoveOrReplace::Func' declared with a mismatched class-key 'class'" }
+                            // { dg-message "replace the class-key with 'struct'" "hint to replace" { target *-*-* } .-1 }
+
+class Var;
+struct Var;                  // { dg-warning "RemoveOrReplace::Var' declared with a mismatched class-key 'struct'" }
+                            // { dg-message "replace the class-key with 'class'" "hint to remove" { target *-*-* } .-1 }
+void f (struct Var*);       // { dg-warning "RemoveOrReplace::Var' declared with a mismatched class-key 'struct'" }
+                            // { dg-message "remove the class-key or replace it with 'class'" "hint to remove" { target *-*-* } .-1 }
+
+int Var;
+
+struct Var;                  // { dg-warning "RemoveOrReplace::Var' declared with a mismatched class-key 'struct'" }
+                            // { dg-message "replace the class-key with 'class'" "hint to replace" { target *-*-* } .-1 }
+}
+
+namespace GlobalObjects
+{
+class A;                    // { dg-message "'GlobalObjects::A' first declared as 'class' here" }
+struct B;                   // { dg-message "'GlobalObjects::B' first declared as 'struct' here" }
+class C { };                // { dg-message "'GlobalObjects::C' defined as 'class' here" }
+
+extern A a0;
+extern class A a1;
+extern class A a2;
+
+extern B b0;
+extern struct B b1;
+extern struct B b2;
+
+extern struct A a3;         // { dg-warning "GlobalObjects::A' declared with a mismatched class-key" }
+extern class A a4;
+
+extern class B b3;          // { dg-warning "GlobalObjects::B' declared with a mismatched class-key" }
+extern struct B b4;
+
+extern struct C c[];        // { dg-warning "GlobalObjects::C' declared with a mismatched class-key" }
+                            // { dg-message "remove the class-key or replace it with 'class'" "hint to remove" { target *-*-* } .-1 }
+
+extern char
+arr[sizeof (struct C)];     // { dg-warning "GlobalObjects::C' declared with a mismatched class-key" }
+                            // { dg-message "remove the class-key or replace it with 'class'" "hint to remove" { target *-*-* } .-1 }
+}   // namespace GlobalObjects
+
+
+namespace LocalObjects
+{
+class A;                    // { dg-message "LocalObjects::A' first declared as 'class' here" }
+struct B;                   // { dg-message "LocalObjects::B' first declared as 'struct' here" }
+
+void f (A*, B&)
+{
+  class A *a1;
+  class A *a2;
+
+  struct B *b1;
+  struct B *b2;
+
+  struct A *a3;             // { dg-warning "LocalObjects::A' declared with a mismatched class-key" }
+  class A *a4;
+
+  class B *b3;              // { dg-warning "LocalObjects::B' declared with a mismatched class-key" }
+  struct B *b4;
+}
+
+void g (struct A*);         // { dg-warning "LocalObjects::A' declared with a mismatched class-key" }
+
+}   // namespace LocalObjects
+
+
+namespace MemberClasses
+{
+struct A { struct B; };
+struct C { struct D; struct D; struct D { }; };
+struct E { class F; class F { }; class F; };
+
+struct G {
+  struct H;                 // { dg-message "MemberClasses::G::H' first declared as 'struct' here" }
+  class H;                  // { dg-warning "MemberClasses::G::H' declared with a mismatched class-key" }
+  class I { };              // { dg-message "MemberClasses::G::I' defined as 'class' here" }
+  struct I;                 // { dg-warning "MemberClasses::G::I' declared with a mismatched class-key" }
+};
+}   // namespace MemberClasses
+
+
+namespace DataMembers
+{
+struct A { struct B *p; };
+struct C { struct D *p; struct D *q; struct D { } d; };
+struct E { class F &r; class F { } f; class F *p; };
+
+class G;                    // { dg-message "DataMembers::G' first declared as 'class' here" }
+struct H;                   // { dg-message "DataMembers::H' first declared as 'struct' here" }
+
+struct I {
+  struct G *p0;             // { dg-warning "DataMembers::G' declared with a mismatched class-key" }
+  class G *p1;
+
+  struct H &r0;
+  class H &r1;              // { dg-warning "DataMembers::H' declared with a mismatched class-key" }
+
+  class J { };              // { dg-message "DataMembers::I::J' defined as 'class' here" }
+  struct K { };             // { dg-message "DataMembers::I::K' defined as 'struct' here" }
+
+  class J j0;
+  class K k0;               // { dg-warning "DataMembers::I::K' declared with a mismatched class-key" }
+
+  struct J j1;              // { dg-warning "DataMembers::I::J' declared with a mismatched class-key" }
+  struct K k1;
+};
+}   // namespace DataMembers
+
+
+namespace Templates
+{
+template <int> class A;
+template <int> class A;
+
+template <int> struct B;
+template <int> struct B;
+
+template <int> union C;
+template <int> union C;
+
+template <int> struct D;    // { dg-warning "Templates::D' declared with a mismatched class-key" }
+template <int>
+class D                     // { dg-message "Templates::D' defined as 'class' here" }
+{ public: D (); };
+
+template <int> class E;     // { dg-warning "Templates::E' declared with a mismatched class-key" }
+template <int>
+struct E                    // { dg-message "Templates::E' defined as 'struct' here" }
+{ int i; };
+
+template <int> class D;
+template <int> struct E;
+
+template <int>
+struct D;                   // { dg-warning "Templates::D' declared with a mismatched class-key" }
+                            // { dg-message "replace the class-key with 'class'" "hint" { target *-*-* } .-1 }
+}   // namespace Templates
+
+
+namespace ExplicitSpecializations
+{
+template <int> class A;
+template <> class A<0>;
+template <> struct A<1>;
+template <> struct A<1> { };
+
+template <int> struct B;
+template <> struct B<0>;
+template <> class B<1>;
+template <> class B<2> { public: B (); };
+
+template <int> union C;
+template <> union C<0>;
+
+template <int> class D;
+template <> class D<0>;     // { dg-warning "ExplicitSpecializations::D' declared with a mismatched class-key " }
+template <>
+struct D<0> { };            // { dg-message "ExplicitSpecializations::D' defined as 'struct' here" }
+
+template <int> struct E;
+template <> struct E<0>;    // { dg-warning "ExplicitSpecializations::E' declared with a mismatched class-key" }
+template <>
+class E<0> { };             // { dg-message "ExplicitSpecializations::E' defined as 'class' here" }
+
+template <int> struct F;
+template <> class F<0> { }; // { dg-message "ExplicitSpecializations::F' defined as 'class' here" }
+
+template <>
+struct F<0>;                // { dg-warning "ExplicitSpecializations::F' declared with a mismatched class-key" }
+}   // namespace ExplicitSpecializations
+
+
+namespace PartialSpecializations
+{
+template <class> class A;
+template <class T> struct A<const T>;
+template <class T> struct A<volatile T>;
+
+template <class> struct B;
+template <class T> class B<const T>;
+template <class T> class B<volatile T>;
+
+template <class> class C { };
+template <class T> struct C<const T> { };
+template <class T> struct C<volatile T> { };
+
+template <class> struct D { };
+template <class T> class D<const T> { };
+template <class T> class D<volatile T> { };
+
+template <class> class E;
+template <class T>
+struct E<const T>;          // { dg-message "PartialSpecializations::E<const T>' first declared as 'struct' here" }
+
+template <class T>
+class E<const T>;           // { dg-warning "PartialSpecializations::E<const T>' declared with a mismatched class-key" }
+
+template <class> class F;
+template <class T>
+class F<const T>;           // { dg-message "PartialSpecializations::F<const T>' first declared as 'class' here" }
+template <class T>
+struct F<const T>;          // { dg-warning "PartialSpecializations::F<const T>' declared with a mismatched class-key" }
+}   // namespace PartialSpecializations
+
+
+namespace Classes
+{
+struct G;
+
+class G;                    // { dg-warning "Classes::G' declared with a mismatched class-key 'class'" }
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wredundant-tags.C b/gcc/testsuite/g++.dg/warn/Wredundant-tags.C
new file mode 100644
index 00000000000..ac5afa912a1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wredundant-tags.C
@@ -0,0 +1,128 @@ 
+/* PR c++/61339 - add mismatch between struct and class
+   Test to verify that -Wredundant-tags is issued for references to class
+   types that use the class-key even though they don't need to.
+   { dg-do compile }
+   { dg-options "-Wredundant-tags" } */
+
+struct A;
+
+extern A *pa;
+extern struct A *pa;        // { dg-warning "redundant class-key 'struct' in reference to 'struct A'" }
+
+extern A aa[];
+extern struct A aa[];       // { dg-warning "redundant class-key 'struct' in reference to 'struct A'" }
+
+void func (A*);
+void func (struct A*);      // { dg-warning "redundant class-key 'struct' in reference to 'struct A'" }
+
+int A;
+
+extern struct A *pa;
+extern struct A aa[];
+void func (struct A*);
+
+
+class B;
+
+extern B *pb;
+extern class B *pb;         // { dg-warning "redundant class-key 'class' in reference to 'class B'" }
+
+extern B ab[];
+extern class B ab[];        // { dg-warning "redundant class-key 'class' in reference to 'class B'" }
+
+void func (B*);
+void func (class B*);       // { dg-warning "redundant class-key 'class' in reference to 'class B'" }
+
+int B;
+
+extern class B *pb;
+extern class B ab[];
+void func (class B*);
+
+
+enum C { c0 };
+
+extern C *pc;
+extern enum C *pc;          // { dg-warning "redundant enum-key 'enum' in reference to 'enum C'" }
+
+extern C ac[];
+extern enum C ac[];         // { dg-warning "redundant enum-key 'enum' in reference to 'enum C'" }
+
+void func (C*);
+void func (enum C*);        // { dg-warning "redundant enum-key 'enum' in reference to 'enum C'" }
+
+int C;
+
+extern enum C *pc;
+extern enum C ac[];
+void func (enum C*);
+
+
+#if __cplusplus > 199711L
+
+enum class D1 { d1 };
+enum struct D2 { d2 };
+
+#else
+
+enum D1 { d1 };
+enum D2 { d2 };
+
+#endif
+
+extern D1 *pd1;
+extern D2 *pd2;
+extern enum D1 *pd1;        // { dg-warning "redundant enum-key 'enum' in reference to 'enum class D1'" "C++ 11 and above" { target c++11 } }
+                            // { dg-warning "redundant enum-key 'enum' in reference to 'enum D1'" "C++ 98" { target c++98_only } .-1 }
+
+extern enum D2 *pd2;        // { dg-warning "redundant enum-key 'enum' in reference to 'enum class D2'" "C++ 11 and above" { target c++11 } }
+                            // { dg-warning "redundant enum-key 'enum' in reference to 'enum D2'" "C++ 98" { target c++98_only } .-1 }
+
+extern D1 ad1[];
+extern D2 ad2[];
+
+#if __cplusplus > 199711L
+extern enum class D1 ad1[]; // { dg-warning "redundant enum-key 'enum class' in reference to 'enum class D1'" "C++ 11 and above" { target c++11 } }
+                            // { dg-warning "elaborated-type-specifier for a scoped enum must not use the 'class' keyword" "C++ 11 and above" { target c++11 } .-1 }
+/* The pretty printer cannot differentiate between enum class and enum struct
+   because the C++ front-end doesn't encode it so allow for both in the text
+   of the warning below.  */
+extern enum struct D2 ad2[]; // { dg-warning "redundant enum-key 'enum struct' in reference to 'enum \(class|struct\) D2'" "C++ 11 and above" { target c++11 } }
+                            // { dg-warning "elaborated-type-specifier for a scoped enum must not use the 'struct' keyword" "C++ 11 and above" { target c++11 } .-1 }
+#else
+extern enum D1 ad1[];       // { dg-warning "redundant enum-key 'enum' in reference to 'enum D1'" "C++ 98" { target c++98_only } }
+#endif
+
+void func (D1*);
+void func (enum D1*);       // { dg-warning "redundant enum-key 'enum' in reference to 'enum " }
+
+void func (D2*);
+void func (enum D2*);       // { dg-warning "redundant enum-key 'enum' in reference to 'enum " }
+
+int D1, D2;
+
+extern enum D1 *pd1;
+extern enum D1 ad1[];
+void func (enum D1*);
+
+extern enum D2 *pd2;
+extern enum D2 ad2[];
+void func (enum D2*);
+
+
+union U;
+
+extern U *pu;
+extern union U *pu;         // { dg-warning "redundant class-key 'union' in reference to 'union U'" }
+
+extern U au[];
+extern union U au[];        // { dg-warning "redundant class-key 'union' in reference to 'union U'" }
+
+void func (U*);
+void func (union U*);       // { dg-warning "redundant class-key 'union' in reference to 'union U'" }
+
+int U;
+
+extern union U *pu;
+extern union U au[];
+void func (union U*);