Re: [C PATCH] Fix debug info locus of enum with previous forward declaration (PR c/79969)

[Marek Polacek <polacek at redhat dot com>]

On Thu, Mar 09, 2017 at 04:57:42PM +0100, Jakub Jelinek wrote:
> On Thu, Mar 09, 2017 at 04:49:31PM +0100, Marek Polacek wrote:
> > On Thu, Mar 09, 2017 at 10:24:43AM +0100, Jakub Jelinek wrote:
> > > Hi!
> > > 
> > > Similarly to https://gcc.gnu.org/ml/gcc-patches/2009-09/msg01161.html
> > > the C FE gets wrong the location of DW_TAG_enumeral_type if there is a
> > > forward declaration.  If we e.g. have a variable that is first declared
> > > extern and then defined, we emit DW_TAG_variable with the location of the
> > > first declaration and then another DW_TAG_variable with DW_AT_specification
> > > pointing to the previous one for the definition, with locus of the
> > > definition.  That is not what we do for enums, there is just one DIE, so we
> > > should use the more descriptive from the locations, which is the definition.
> > > 
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > > 
> > > 2017-03-09  Jakub Jelinek  <jakub@redhat.com>
> > > 
> > > 	PR c/79969
> > > 	* c-decl.c (start_enum): Adjust DECL_SOURCE_LOCATION of
> > > 	TYPE_STUB_DECL.
> > 
> > How about doing this in finish_enum, similarly to finish_struct?  You'd need to
> > pass a location from the parser, but that's easy:
> 
> That would be just for consistency with finish_struct, right?

Yeah.

> Not sure if we need such consistency, but I don't care that much.  The point
> to put it into start_enum was that we don't really care about the location
> of the forward declaration after that.
> 
> That said, there is one thing I'm wondering about:
> 
>   if (name != NULL_TREE)
>     enumtype = lookup_tag (ENUMERAL_TYPE, name, true, &enumloc);
> 
>   if (enumtype == NULL_TREE || TREE_CODE (enumtype) != ENUMERAL_TYPE)
>     {
>       enumtype = make_node (ENUMERAL_TYPE);
>       pushtag (loc, name, enumtype);
>     }
> 
> with the optional patched spot after this.  Now, if somebody does:
> enum E;
> enum E { A, B, C };
> enum E { D, F };
> then I think we'll complain about line 3 overriding previous definition
> at line 1 (which is not right).  Maybe if there is TYPE_STUB_DECL (do we
> have it always?), we can override enumloc = DECL_SOURCE_LOCATION
> (TYPE_STUB_DECL (enumtype));?  I bet trying to change the binding ->locus
> would be too much work.

So if we set the DECL_SOURCE_LOCATION in finish_enum, we can make use of
TYPE_STUB_DECL in start_enum for better diagnostics (it's set anytime we
pushtag so it should always be available), so I guess I'd slightly prefer
the finish_enum variant.  But if you don't want to do it, that's fine with
me too.

And if we decide to improve the diagnostic, we also need to fix the struct
case:

struct S;
struct S { int i; };
struct S { int i, j; };

gives suboptimal
ll.c:3:8: error: redefinition of ‘struct S’
 struct S { int i, j; };
        ^
ll.c:1:8: note: originally defined here
 struct S;
        ^

while clang gets it right:
ll.c:3:8: error: redefinition of 'S'
struct S { int i, j; };
       ^
ll.c:2:8: note: previous definition is here
struct S { int i; };
       ^

Of course, feel free to leave those diagnostic bits for me.

	Marek