| Message ID | 20170309154931.GD3172@redhat.com |
|---|---|
| State | New |
| Headers | show |
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? 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. Jakub
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
On Thu, Mar 09, 2017 at 05:27:33PM +0100, Marek Polacek wrote: > > 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. We can do it the same if done in start_enum, because it just uses enumloc variable for that. So e.g. else if (TYPE_STUB_DECL (enumtype)) { enumloc = DECL_SOURCE_LOCATION (TYPE_STUB_DECL (enumtype)); DECL_SOURCE_LOCATION (TYPE_STUB_DECL (enumtype)) = loc; } would achieve it too. Given that finish_enum doesn't have the location_t argument, that looks simpler to me, but it is not a big deal either way. > 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. Yeah, I'll happily defer that to you? ;) Jakub
On Thu, Mar 09, 2017 at 05:34:43PM +0100, Jakub Jelinek wrote: > On Thu, Mar 09, 2017 at 05:27:33PM +0100, Marek Polacek wrote: > > > 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. > > We can do it the same if done in start_enum, because it just uses enumloc > variable for that. > So e.g. > else if (TYPE_STUB_DECL (enumtype)) > { > enumloc = DECL_SOURCE_LOCATION (TYPE_STUB_DECL (enumtype)); > DECL_SOURCE_LOCATION (TYPE_STUB_DECL (enumtype)) = loc; > } > would achieve it too. Given that finish_enum doesn't have the location_t > argument, that looks simpler to me, but it is not a big deal either way. Your patch is OK as-is then. > > 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. > > Yeah, I'll happily defer that to you? ;) Ok, I'll open a PR. Marek
diff --git gcc/c/c-decl.c gcc/c/c-decl.c index 645304a..5df41dd 100644 --- gcc/c/c-decl.c +++ gcc/c/c-decl.c @@ -8246,7 +8246,7 @@ start_enum (location_t loc, struct c_enum_contents *the_enum, tree name) Returns ENUMTYPE. */ tree -finish_enum (tree enumtype, tree values, tree attributes) +finish_enum (location_t loc, tree enumtype, tree values, tree attributes) { tree pair, tem; tree minnode = 0, maxnode = 0; @@ -8382,6 +8382,11 @@ finish_enum (tree enumtype, tree values, tree attributes) TYPE_LANG_SPECIFIC (tem) = TYPE_LANG_SPECIFIC (enumtype); } + /* Update type location to the one of the definition, instead of e.g. + a forward declaration. */ + if (TYPE_STUB_DECL (enumtype)) + DECL_SOURCE_LOCATION (TYPE_STUB_DECL (enumtype)) = loc; + /* Finish debugging output for this type. */ rest_of_type_compilation (enumtype, toplevel); diff --git gcc/c/c-parser.c gcc/c/c-parser.c index 8330e65..2d9c0d3 100644 --- gcc/c/c-parser.c +++ gcc/c/c-parser.c @@ -2779,7 +2779,7 @@ c_parser_enum_specifier (c_parser *parser) } } postfix_attrs = c_parser_attributes (parser); - ret.spec = finish_enum (type, nreverse (values), + ret.spec = finish_enum (enum_loc, type, nreverse (values), chainon (attrs, postfix_attrs)); ret.kind = ctsk_tagdef; ret.expr = NULL_TREE; diff --git gcc/c/c-tree.h gcc/c/c-tree.h index 13e40e6..3800256 100644 --- gcc/c/c-tree.h +++ gcc/c/c-tree.h @@ -534,7 +534,7 @@ extern void c_release_switch_bindings (struct c_spot_bindings *); extern bool c_check_switch_jump_warnings (struct c_spot_bindings *, location_t, location_t); extern void finish_decl (tree, location_t, tree, tree, tree); -extern tree finish_enum (tree, tree, tree); +extern tree finish_enum (location_t, tree, tree, tree); extern void finish_function (void); extern tree finish_struct (location_t, tree, tree, tree, struct c_struct_parse_info *);