| Message ID | 20221112234543.95441-3-aldot@gcc.gnu.org |
|---|---|
| State | New |
| Headers | show |
| Series | function result decl location; type demotion | expand |
On 11/12/22 13:45, Bernhard Reutner-Fischer wrote: > gcc/cp/ChangeLog: > > * decl.cc (start_function): Set the result decl source location to > the location of the typespec. > > --- > Bootstrapped and regtested on x86_86-unknown-linux with no regressions. > Ok for trunk? > > Cc: Nathan Sidwell <nathan@acm.org> > Cc: Jason Merrill <jason@redhat.com> > --- > gcc/cp/decl.cc | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index 6e98ea35a39..ed40815e645 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -17449,6 +17449,8 @@ start_function (cp_decl_specifier_seq *declspecs, > tree attrs) > { > tree decl1; > + tree result; > + bool ret; We now prefer to declare new variables as late as possible, usually when they are initialized. > decl1 = grokdeclarator (declarator, declspecs, FUNCDEF, 1, &attrs); > invoke_plugin_callbacks (PLUGIN_START_PARSE_FUNCTION, decl1); > @@ -17461,7 +17463,18 @@ start_function (cp_decl_specifier_seq *declspecs, > gcc_assert (same_type_p (TREE_TYPE (TREE_TYPE (decl1)), > integer_type_node)); > > - return start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); > + ret = start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); > + > + /* decl1 might be ggc_freed here. */ > + decl1 = current_function_decl; > + > + /* Set the result decl source location to the location of the typespec. */ > + if (TREE_CODE (decl1) == FUNCTION_DECL > + && declspecs->locations[ds_type_spec] != UNKNOWN_LOCATION > + && (result = DECL_RESULT (decl1)) != NULL_TREE > + && DECL_SOURCE_LOCATION (result) == input_location) > + DECL_SOURCE_LOCATION (result) = declspecs->locations[ds_type_spec]; One way to handle the template case would be for the code in start_preparsed_function that sets DECL_RESULT to check whether decl1 is a template instantiation, and in that case copy the location from the template's DECL_RESULT, i.e. DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1))) > + return ret; > } > > /* Returns true iff an EH_SPEC_BLOCK should be created in the body of
On Tue, 15 Nov 2022 18:52:41 -0500 Jason Merrill <jason@redhat.com> wrote: > On 11/12/22 13:45, Bernhard Reutner-Fischer wrote: > > gcc/cp/ChangeLog: > > > > * decl.cc (start_function): Set the result decl source location to > > the location of the typespec. > > > > --- > > Bootstrapped and regtested on x86_86-unknown-linux with no regressions. > > Ok for trunk? > > > > Cc: Nathan Sidwell <nathan@acm.org> > > Cc: Jason Merrill <jason@redhat.com> > > --- > > gcc/cp/decl.cc | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > > index 6e98ea35a39..ed40815e645 100644 > > --- a/gcc/cp/decl.cc > > +++ b/gcc/cp/decl.cc > > @@ -17449,6 +17449,8 @@ start_function (cp_decl_specifier_seq *declspecs, > > tree attrs) > > { > > tree decl1; > > + tree result; > > + bool ret; > > We now prefer to declare new variables as late as possible, usually when > they are initialized. Moved. Ok like attached? Bootstrapped and regtested fine. > > decl1 = grokdeclarator (declarator, declspecs, FUNCDEF, 1, &attrs); > > invoke_plugin_callbacks (PLUGIN_START_PARSE_FUNCTION, decl1); > > @@ -17461,7 +17463,18 @@ start_function (cp_decl_specifier_seq *declspecs, > > gcc_assert (same_type_p (TREE_TYPE (TREE_TYPE (decl1)), > > integer_type_node)); > > > > - return start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); > > + ret = start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); > > + > > + /* decl1 might be ggc_freed here. */ > > + decl1 = current_function_decl; > > + > > + /* Set the result decl source location to the location of the typespec. */ > > + if (TREE_CODE (decl1) == FUNCTION_DECL > > + && declspecs->locations[ds_type_spec] != UNKNOWN_LOCATION > > + && (result = DECL_RESULT (decl1)) != NULL_TREE > > + && DECL_SOURCE_LOCATION (result) == input_location) > > + DECL_SOURCE_LOCATION (result) = declspecs->locations[ds_type_spec]; > > One way to handle the template case would be for the code in > start_preparsed_function that sets DECL_RESULT to check whether decl1 is > a template instantiation, and in that case copy the location from the > template's DECL_RESULT, i.e. > > DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1))) Well, that would probably work if something would set the location of that template result decl properly, which nothing does out of the box. diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index ed7226b82f0..65d78c82a2d 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -17230,6 +17231,17 @@ start_preparsed_function (tree decl1, tree attrs, int flags) cp_apply_type_quals_to_decl (cp_type_quals (restype), resdecl); } + /* Set the result decl source location to the location of the typespec. */ + if (DECL_RESULT (decl1) + && !DECL_USE_TEMPLATE (decl1) + && DECL_TEMPLATE_INFO (decl1) + && DECL_TI_TEMPLATE (decl1) + && DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)) + && DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)))) + DECL_SOURCE_LOCATION (DECL_RESULT (decl1)) + = DECL_SOURCE_LOCATION ( + DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)))); + /* Record the decl so that the function name is defined. If we already have a decl for this name, and it is a FUNCTION_DECL, use the old decl. */ (gdb) call inform(DECL_SOURCE_LOCATION (DECL_RESULT (decl1)), "decl1 result locus before") ../tmp4/return-narrow-2.cc:7:3: note: decl1 result locus before 7 | { return _M_finish != 0; } | ^ (gdb) n (gdb) call inform(DECL_SOURCE_LOCATION (DECL_RESULT (decl1)), "decl1 result locus from TI") ../tmp4/return-narrow-2.cc:7:3: note: decl1 result locus from TI (gdb) p DECL_SOURCE_LOCATION (DECL_RESULT (decl1)) $1 = 267168 I'm leaving the template case alone for now, maybe i'm motivated later on to again look at grokfndecl and/or grokmethod to fill in the proper location. For starters i only need normal functions. But many thanks for the hint on where the template stuff is, i thought i would not need it at all but had hoped that there is a spot where both declspec are at hand and something is "derived" from the templates. > > > + return ret; > > } > > > > /* Returns true iff an EH_SPEC_BLOCK should be created in the body of >
On 11/17/22 03:56, Bernhard Reutner-Fischer wrote: > On Tue, 15 Nov 2022 18:52:41 -0500 > Jason Merrill <jason@redhat.com> wrote: > >> On 11/12/22 13:45, Bernhard Reutner-Fischer wrote: >>> gcc/cp/ChangeLog: >>> >>> * decl.cc (start_function): Set the result decl source location to >>> the location of the typespec. >>> >>> --- >>> Bootstrapped and regtested on x86_86-unknown-linux with no regressions. >>> Ok for trunk? >>> >>> Cc: Nathan Sidwell <nathan@acm.org> >>> Cc: Jason Merrill <jason@redhat.com> >>> --- >>> gcc/cp/decl.cc | 15 ++++++++++++++- >>> 1 file changed, 14 insertions(+), 1 deletion(-) >>> >>> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc >>> index 6e98ea35a39..ed40815e645 100644 >>> --- a/gcc/cp/decl.cc >>> +++ b/gcc/cp/decl.cc >>> @@ -17449,6 +17449,8 @@ start_function (cp_decl_specifier_seq *declspecs, >>> tree attrs) >>> { >>> tree decl1; >>> + tree result; >>> + bool ret; >> >> We now prefer to declare new variables as late as possible, usually when >> they are initialized. > > Moved. Ok like attached? Bootstrapped and regtested fine. > >>> decl1 = grokdeclarator (declarator, declspecs, FUNCDEF, 1, &attrs); >>> invoke_plugin_callbacks (PLUGIN_START_PARSE_FUNCTION, decl1); >>> @@ -17461,7 +17463,18 @@ start_function (cp_decl_specifier_seq *declspecs, >>> gcc_assert (same_type_p (TREE_TYPE (TREE_TYPE (decl1)), >>> integer_type_node)); >>> >>> - return start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); >>> + ret = start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); >>> + >>> + /* decl1 might be ggc_freed here. */ >>> + decl1 = current_function_decl; >>> + >>> + /* Set the result decl source location to the location of the typespec. */ >>> + if (TREE_CODE (decl1) == FUNCTION_DECL >>> + && declspecs->locations[ds_type_spec] != UNKNOWN_LOCATION >>> + && (result = DECL_RESULT (decl1)) != NULL_TREE >>> + && DECL_SOURCE_LOCATION (result) == input_location) >>> + DECL_SOURCE_LOCATION (result) = declspecs->locations[ds_type_spec]; >> >> One way to handle the template case would be for the code in >> start_preparsed_function that sets DECL_RESULT to check whether decl1 is >> a template instantiation, and in that case copy the location from the >> template's DECL_RESULT, i.e. >> >> DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1))) > > Well, that would probably work if something would set the location of > that template result decl properly, which nothing does out of the box. Hmm, it should get set by your patch, since templates go through start_function like normal functions. > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index ed7226b82f0..65d78c82a2d 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -17230,6 +17231,17 @@ start_preparsed_function (tree decl1, tree attrs, int flags) > cp_apply_type_quals_to_decl (cp_type_quals (restype), resdecl); > } > > + /* Set the result decl source location to the location of the typespec. */ > + if (DECL_RESULT (decl1) > + && !DECL_USE_TEMPLATE (decl1) > + && DECL_TEMPLATE_INFO (decl1) > + && DECL_TI_TEMPLATE (decl1) > + && DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)) > + && DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)))) This condition is true only for the template definition, for which you haven't gotten to your start_function change yet. Instead, you want to copy the location for instantiations, i.e. check DECL_TEMPLATE_INSTANTIATION instead of !DECL_USE_TEMPLATE. > + DECL_SOURCE_LOCATION (DECL_RESULT (decl1)) > + = DECL_SOURCE_LOCATION ( Open paren goes on the new line. > + DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)))); > /* Record the decl so that the function name is defined. > If we already have a decl for this name, and it is a FUNCTION_DECL, > use the old decl. */ > > (gdb) call inform(DECL_SOURCE_LOCATION (DECL_RESULT (decl1)), "decl1 result locus before") > ../tmp4/return-narrow-2.cc:7:3: note: decl1 result locus before > 7 | { return _M_finish != 0; } > | ^ > (gdb) n > (gdb) call inform(DECL_SOURCE_LOCATION (DECL_RESULT (decl1)), "decl1 result locus from TI") > ../tmp4/return-narrow-2.cc:7:3: note: decl1 result locus from TI > (gdb) p DECL_SOURCE_LOCATION (DECL_RESULT (decl1)) > $1 = 267168 > > I'm leaving the template case alone for now, maybe i'm motivated later > on to again look at grokfndecl and/or grokmethod to fill in the proper > location. For starters i only need normal functions. > But many thanks for the hint on where the template stuff is, i thought > i would not need it at all but had hoped that there is a spot where > both declspec are at hand and something is "derived" from the templates. > >> >>> + return ret; >>> } >>> >>> /* Returns true iff an EH_SPEC_BLOCK should be created in the body of >> >
On Thu, 17 Nov 2022 09:53:32 -0500 Jason Merrill <jason@redhat.com> wrote: > On 11/17/22 03:56, Bernhard Reutner-Fischer wrote: > > On Tue, 15 Nov 2022 18:52:41 -0500 > > Jason Merrill <jason@redhat.com> wrote: > > > >> On 11/12/22 13:45, Bernhard Reutner-Fischer wrote: > >>> gcc/cp/ChangeLog: > >>> > >>> * decl.cc (start_function): Set the result decl source location to > >>> the location of the typespec. > >>> > >>> --- > >>> Bootstrapped and regtested on x86_86-unknown-linux with no regressions. > >>> Ok for trunk? > >>> > >>> Cc: Nathan Sidwell <nathan@acm.org> > >>> Cc: Jason Merrill <jason@redhat.com> > >>> --- > >>> gcc/cp/decl.cc | 15 ++++++++++++++- > >>> 1 file changed, 14 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > >>> index 6e98ea35a39..ed40815e645 100644 > >>> --- a/gcc/cp/decl.cc > >>> +++ b/gcc/cp/decl.cc > >>> @@ -17449,6 +17449,8 @@ start_function (cp_decl_specifier_seq *declspecs, > >>> tree attrs) > >>> { > >>> tree decl1; > >>> + tree result; > >>> + bool ret; > >> > >> We now prefer to declare new variables as late as possible, usually when > >> they are initialized. > > > > Moved. Ok like attached? Bootstrapped and regtested fine. > > > >>> decl1 = grokdeclarator (declarator, declspecs, FUNCDEF, 1, &attrs); > >>> invoke_plugin_callbacks (PLUGIN_START_PARSE_FUNCTION, decl1); > >>> @@ -17461,7 +17463,18 @@ start_function (cp_decl_specifier_seq *declspecs, > >>> gcc_assert (same_type_p (TREE_TYPE (TREE_TYPE (decl1)), > >>> integer_type_node)); > >>> > >>> - return start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); > >>> + ret = start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); > >>> + > >>> + /* decl1 might be ggc_freed here. */ > >>> + decl1 = current_function_decl; > >>> + > >>> + /* Set the result decl source location to the location of the typespec. */ > >>> + if (TREE_CODE (decl1) == FUNCTION_DECL > >>> + && declspecs->locations[ds_type_spec] != UNKNOWN_LOCATION > >>> + && (result = DECL_RESULT (decl1)) != NULL_TREE > >>> + && DECL_SOURCE_LOCATION (result) == input_location) > >>> + DECL_SOURCE_LOCATION (result) = declspecs->locations[ds_type_spec]; > >> > >> One way to handle the template case would be for the code in > >> start_preparsed_function that sets DECL_RESULT to check whether decl1 is > >> a template instantiation, and in that case copy the location from the > >> template's DECL_RESULT, i.e. > >> > >> DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1))) > > > > Well, that would probably work if something would set the location of > > that template result decl properly, which nothing does out of the box. > > Hmm, it should get set by your patch, since templates go through > start_function like normal functions. > > > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > > index ed7226b82f0..65d78c82a2d 100644 > > --- a/gcc/cp/decl.cc > > +++ b/gcc/cp/decl.cc > > @@ -17230,6 +17231,17 @@ start_preparsed_function (tree decl1, tree attrs, int flags) > > cp_apply_type_quals_to_decl (cp_type_quals (restype), resdecl); > > } > > > > + /* Set the result decl source location to the location of the typespec. */ > > + if (DECL_RESULT (decl1) > > + && !DECL_USE_TEMPLATE (decl1) > > + && DECL_TEMPLATE_INFO (decl1) > > + && DECL_TI_TEMPLATE (decl1) > > + && DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)) > > + && DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)))) > > This condition is true only for the template definition, for which you > haven't gotten to your start_function change yet. > > Instead, you want to copy the location for instantiations, i.e. check > DECL_TEMPLATE_INSTANTIATION instead of !DECL_USE_TEMPLATE. No, that makes no difference. But really I'm not interested in the template case, i only mentioned them because they don't work and in case somebody wanted to have correct locations. I remember just frustration when i looked at those a year ago. Is the hunk for normal functions OK for trunk? thanks, > > > + DECL_SOURCE_LOCATION (DECL_RESULT (decl1)) > > + = DECL_SOURCE_LOCATION ( > > Open paren goes on the new line. > > > + DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)))); > /* Record the decl so that the function name is defined. > > If we already have a decl for this name, and it is a FUNCTION_DECL, > > use the old decl. */ > > > > (gdb) call inform(DECL_SOURCE_LOCATION (DECL_RESULT (decl1)), "decl1 result locus before") > > ../tmp4/return-narrow-2.cc:7:3: note: decl1 result locus before > > 7 | { return _M_finish != 0; } > > | ^ > > (gdb) n > > (gdb) call inform(DECL_SOURCE_LOCATION (DECL_RESULT (decl1)), "decl1 result locus from TI") > > ../tmp4/return-narrow-2.cc:7:3: note: decl1 result locus from TI > > (gdb) p DECL_SOURCE_LOCATION (DECL_RESULT (decl1)) > > $1 = 267168 > > > > I'm leaving the template case alone for now, maybe i'm motivated later > > on to again look at grokfndecl and/or grokmethod to fill in the proper > > location. For starters i only need normal functions. > > But many thanks for the hint on where the template stuff is, i thought > > i would not need it at all but had hoped that there is a spot where > > both declspec are at hand and something is "derived" from the templates. > > > >> > >>> + return ret; > >>> } > >>> > >>> /* Returns true iff an EH_SPEC_BLOCK should be created in the body of > >> > > >
On 11/17/22 14:02, Bernhard Reutner-Fischer wrote: > On Thu, 17 Nov 2022 09:53:32 -0500 > Jason Merrill <jason@redhat.com> wrote: > >> On 11/17/22 03:56, Bernhard Reutner-Fischer wrote: >>> On Tue, 15 Nov 2022 18:52:41 -0500 >>> Jason Merrill <jason@redhat.com> wrote: >>> >>>> On 11/12/22 13:45, Bernhard Reutner-Fischer wrote: >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * decl.cc (start_function): Set the result decl source location to >>>>> the location of the typespec. >>>>> >>>>> --- >>>>> Bootstrapped and regtested on x86_86-unknown-linux with no regressions. >>>>> Ok for trunk? >>>>> >>>>> Cc: Nathan Sidwell <nathan@acm.org> >>>>> Cc: Jason Merrill <jason@redhat.com> >>>>> --- >>>>> gcc/cp/decl.cc | 15 ++++++++++++++- >>>>> 1 file changed, 14 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc >>>>> index 6e98ea35a39..ed40815e645 100644 >>>>> --- a/gcc/cp/decl.cc >>>>> +++ b/gcc/cp/decl.cc >>>>> @@ -17449,6 +17449,8 @@ start_function (cp_decl_specifier_seq *declspecs, >>>>> tree attrs) >>>>> { >>>>> tree decl1; >>>>> + tree result; >>>>> + bool ret; >>>> >>>> We now prefer to declare new variables as late as possible, usually when >>>> they are initialized. >>> >>> Moved. Ok like attached? Bootstrapped and regtested fine. >>> >>>>> decl1 = grokdeclarator (declarator, declspecs, FUNCDEF, 1, &attrs); >>>>> invoke_plugin_callbacks (PLUGIN_START_PARSE_FUNCTION, decl1); >>>>> @@ -17461,7 +17463,18 @@ start_function (cp_decl_specifier_seq *declspecs, >>>>> gcc_assert (same_type_p (TREE_TYPE (TREE_TYPE (decl1)), >>>>> integer_type_node)); >>>>> >>>>> - return start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); >>>>> + ret = start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); >>>>> + >>>>> + /* decl1 might be ggc_freed here. */ >>>>> + decl1 = current_function_decl; >>>>> + >>>>> + /* Set the result decl source location to the location of the typespec. */ >>>>> + if (TREE_CODE (decl1) == FUNCTION_DECL >>>>> + && declspecs->locations[ds_type_spec] != UNKNOWN_LOCATION >>>>> + && (result = DECL_RESULT (decl1)) != NULL_TREE >>>>> + && DECL_SOURCE_LOCATION (result) == input_location) >>>>> + DECL_SOURCE_LOCATION (result) = declspecs->locations[ds_type_spec]; >>>> >>>> One way to handle the template case would be for the code in >>>> start_preparsed_function that sets DECL_RESULT to check whether decl1 is >>>> a template instantiation, and in that case copy the location from the >>>> template's DECL_RESULT, i.e. >>>> >>>> DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1))) >>> >>> Well, that would probably work if something would set the location of >>> that template result decl properly, which nothing does out of the box. >> >> Hmm, it should get set by your patch, since templates go through >> start_function like normal functions. >> >>> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc >>> index ed7226b82f0..65d78c82a2d 100644 >>> --- a/gcc/cp/decl.cc >>> +++ b/gcc/cp/decl.cc >>> @@ -17230,6 +17231,17 @@ start_preparsed_function (tree decl1, tree attrs, int flags) >>> cp_apply_type_quals_to_decl (cp_type_quals (restype), resdecl); >>> } >>> >>> + /* Set the result decl source location to the location of the typespec. */ >>> + if (DECL_RESULT (decl1) >>> + && !DECL_USE_TEMPLATE (decl1) >>> + && DECL_TEMPLATE_INFO (decl1) >>> + && DECL_TI_TEMPLATE (decl1) >>> + && DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)) >>> + && DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)))) >> >> This condition is true only for the template definition, for which you >> haven't gotten to your start_function change yet. >> >> Instead, you want to copy the location for instantiations, i.e. check >> DECL_TEMPLATE_INSTANTIATION instead of !DECL_USE_TEMPLATE. > > No, that makes no difference. Hmm, when I stop there when processing the instantiation the template's DECL_RESULT has the right location information, e.g. for template <class T> int f() { return 42; } int main() { f<int>(); } #1 0x0000000000f950e8 in instantiate_body (pattern=<template_decl 0x7ffff7ff5080 f>, args=<tree_vec 0x7fffe9712ae0>, d=<function_decl 0x7fffe971e600 f>, nested_p=false) at /home/jason/gt/gcc/cp/pt.cc:26470 #0 start_preparsed_function (decl1=<function_decl 0x7fffe971e600 f>, attrs=<tree 0x0>, flags=1) at /home/jason/gt/gcc/cp/decl.cc:17252 (gdb) p expand_location (input_location) $13 = {file = 0x4962370 "wa.C", line = 1, column = 24, data = 0x0, sysp = false} (gdb) p expand_location (DECL_SOURCE_LOCATION (DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1))))) $14 = {file = 0x4962370 "wa.C", line = 1, column = 20, data = 0x0, sysp = false} > But really I'm not interested in the template case, i only mentioned > them because they don't work and in case somebody wanted to have correct > locations. > I remember just frustration when i looked at those a year ago. I'd like to get the template case right while we're looking at it. I guess I can add that myself if you're done trying. > Is the hunk for normal functions OK for trunk? You also need a testcase for the desired behavior, with e.g. { dg-error "23:" } > thanks, > >> >>> + DECL_SOURCE_LOCATION (DECL_RESULT (decl1)) >>> + = DECL_SOURCE_LOCATION ( >> >> Open paren goes on the new line. >> >>> + DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)))); > /* Record the decl so that the function name is defined. >>> If we already have a decl for this name, and it is a FUNCTION_DECL, >>> use the old decl. */ >>> >>> (gdb) call inform(DECL_SOURCE_LOCATION (DECL_RESULT (decl1)), "decl1 result locus before") >>> ../tmp4/return-narrow-2.cc:7:3: note: decl1 result locus before >>> 7 | { return _M_finish != 0; } >>> | ^ >>> (gdb) n >>> (gdb) call inform(DECL_SOURCE_LOCATION (DECL_RESULT (decl1)), "decl1 result locus from TI") >>> ../tmp4/return-narrow-2.cc:7:3: note: decl1 result locus from TI >>> (gdb) p DECL_SOURCE_LOCATION (DECL_RESULT (decl1)) >>> $1 = 267168 >>> >>> I'm leaving the template case alone for now, maybe i'm motivated later >>> on to again look at grokfndecl and/or grokmethod to fill in the proper >>> location. For starters i only need normal functions. >>> But many thanks for the hint on where the template stuff is, i thought >>> i would not need it at all but had hoped that there is a spot where >>> both declspec are at hand and something is "derived" from the templates. >>> >>>> >>>>> + return ret; >>>>> } >>>>> >>>>> /* Returns true iff an EH_SPEC_BLOCK should be created in the body of >>>> >>> >> >
On Thu, 17 Nov 2022 18:52:36 -0500 Jason Merrill <jason@redhat.com> wrote: > On 11/17/22 14:02, Bernhard Reutner-Fischer wrote: > > On Thu, 17 Nov 2022 09:53:32 -0500 > > Jason Merrill <jason@redhat.com> wrote: > >> Instead, you want to copy the location for instantiations, i.e. check > >> DECL_TEMPLATE_INSTANTIATION instead of !DECL_USE_TEMPLATE. > > > > No, that makes no difference. > > Hmm, when I stop there when processing the instantiation the template's > DECL_RESULT has the right location information, e.g. for > > template <class T> int f() { return 42; } > > int main() > { > f<int>(); > } > > #1 0x0000000000f950e8 in instantiate_body (pattern=<template_decl > 0x7ffff7ff5080 f>, args=<tree_vec 0x7fffe9712ae0>, d=<function_decl > 0x7fffe971e600 f>, nested_p=false) at /home/jason/gt/gcc/cp/pt.cc:26470 > #0 start_preparsed_function (decl1=<function_decl 0x7fffe971e600 f>, > attrs=<tree 0x0>, flags=1) at /home/jason/gt/gcc/cp/decl.cc:17252 > (gdb) p expand_location (input_location) > $13 = {file = 0x4962370 "wa.C", line = 1, column = 24, data = 0x0, sysp > = false} > (gdb) p expand_location (DECL_SOURCE_LOCATION (DECL_RESULT > (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1))))) > $14 = {file = 0x4962370 "wa.C", line = 1, column = 20, data = 0x0, sysp > = false} Yes, that works. Sorry if i was not clear: The thing in the cover letter in this series does not work, the mini_vector reduced testcase from the libstdc++-v3/include/ext/bitmap_allocator.h. class template member function return type location, would that be it? AFAIR the problem was that that these member functions get their result decl late. When they get them, there are no declspecs->locations[ds_type_spec] around anywhere to tuck that on the resdecl. While the result decl is clear, there is no obvious way where to store the ds_type_spec (somewhere in the template, as you told me). Back then I tried moving the resdecl building from start_preparsed_function to grokfndecl but that did not work out easily IIRC and i ultimately gave up to move stuff around rather blindly. I also tried to find a spot where i could store the ds_type_spec locus somewhere in grokmethod, but i think the problem was the same, i had just the type where i cannot store a locus and did not find a place where i could smuggle the locus along. So, to make that clear. Your template function (?) works: $ XXX=1 ./xg++ -B. -S -o /dev/null ../tmp4/return-narrow-2j.cc ../tmp4/return-narrow-2j.cc: In function ‘int f()’: ../tmp4/return-narrow-2j.cc:1:20: warning: result decl locus sample 1 | template <class T> int f() { return 42; } | ^~~ | the return type ../tmp4/return-narrow-2j.cc: In function ‘int main()’: ../tmp4/return-narrow-2j.cc:3:1: warning: result decl locus sample 3 | int main() | ^~~ | the return type ../tmp4/return-narrow-2j.cc: In instantiation of ‘int f() [with T = int]’: ../tmp4/return-narrow-2j.cc:5:10: required from here ../tmp4/return-narrow-2j.cc:1:20: warning: result decl locus sample 1 | template <class T> int f() { return 42; } | ^~~ | the return type The class member fn not so much (IMHO, see attached): $ XXX=1 ./xg++ -B. -S -o /dev/null ../tmp4/return-narrow-2.cc ../tmp4/return-narrow-2.cc: In member function ‘const long unsigned int __mini_vector< <template-parameter-1-1> >::_M_space_left()’: ../tmp4/return-narrow-2.cc:9:3: warning: result decl locus sample 9 | { return _M_finish != 0; } | ^ | the return type ../tmp4/return-narrow-2.cc: In instantiation of ‘const long unsigned int __mini_vector< <template-parameter-1-1> >::_M_space_left() [with <template-parameter-1-1> = std::pair<long int, long int>]’: ../tmp4/return-narrow-2.cc:11:17: required from here ../tmp4/return-narrow-2.cc:9:3: warning: result decl locus sample 9 | { return _M_finish != 0; } | ^ | the return type ../tmp4/return-narrow-2.cc: In instantiation of ‘const long unsigned int __mini_vector< <template-parameter-1-1> >::_M_space_left() [with <template-parameter-1-1> = int]’: ../tmp4/return-narrow-2.cc:12:17: required from here ../tmp4/return-narrow-2.cc:9:3: warning: result decl locus sample 9 | { return _M_finish != 0; } | ^ | the return type > > > But really I'm not interested in the template case, i only mentioned > > them because they don't work and in case somebody wanted to have correct > > locations. > > I remember just frustration when i looked at those a year ago. > > I'd like to get the template case right while we're looking at it. I > guess I can add that myself if you're done trying. > > > Is the hunk for normal functions OK for trunk? > > You also need a testcase for the desired behavior, with e.g. > { dg-error "23:" } I'd have to think about how to test that with trunk, yes. There are no existing warnings that want to point to the return type, are there? Maybe a g++.dg/plugin/result_decl_plugin.c then. set plugin_test_list [list hmz. That strikes me as not all that flexible. We could glob *_plugin.[cC][c]*, and have foo_plugin.lst contain it's files. Whatever. thanks, diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index d28889ed865..e0f057fc37b 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -17235,6 +17236,17 @@ start_preparsed_function (tree decl1, tree attrs, int flags) cp_apply_type_quals_to_decl (cp_type_quals (restype), resdecl); } + /* Set the result decl source location to the location of the typespec. */ + if (DECL_RESULT (decl1) + && DECL_TEMPLATE_INSTANTIATION (decl1) + && DECL_TEMPLATE_INFO (decl1) + && DECL_TI_TEMPLATE (decl1) + && DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)) + && DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)))) + DECL_SOURCE_LOCATION (DECL_RESULT (decl1)) + = DECL_SOURCE_LOCATION ( + DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1)))); + /* Record the decl so that the function name is defined. If we already have a decl for this name, and it is a FUNCTION_DECL, use the old decl. */ @@ -17532,7 +17544,19 @@ start_function (cp_decl_specifier_seq *declspecs, gcc_assert (same_type_p (TREE_TYPE (TREE_TYPE (decl1)), integer_type_node)); - return start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); + bool ret = start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); + + /* decl1 might be ggc_freed here. */ + decl1 = current_function_decl; + + tree result; + /* Set the result decl source location to the location of the typespec. */ + if (TREE_CODE (decl1) == FUNCTION_DECL + && declspecs->locations[ds_type_spec] != UNKNOWN_LOCATION + && (result = DECL_RESULT (decl1)) != NULL_TREE + && DECL_SOURCE_LOCATION (result) == input_location) + DECL_SOURCE_LOCATION (result) = declspecs->locations[ds_type_spec]; + return ret; } /* Returns true iff an EH_SPEC_BLOCK should be created in the body of @@ -18063,6 +18087,14 @@ finish_function (bool inline_p) suppress_warning (fndecl, OPT_Wreturn_type); } + if (getenv("XXX") != NULL) + { + location_t result_loc = DECL_SOURCE_LOCATION (DECL_RESULT (fndecl)); + gcc_rich_location richloc (result_loc); + richloc.add_fixit_replace (result_loc, "the return type"); + warning_at (&richloc, 0, "result dec%c locus sample", 'l'); + } + /* Lambda closure members are implicitly constexpr if possible. */ if (cxx_dialect >= cxx17 && LAMBDA_TYPE_P (CP_DECL_CONTEXT (fndecl))) namespace std { template < typename, typename > struct pair; } template < typename > struct __mini_vector { int _M_finish; const unsigned long __attribute__((deprecated)) _M_space_left() { return _M_finish != 0; } }; template class __mini_vector< std::pair< long, long > >; template class __mini_vector< int >;
On 11/18/22 05:49, Bernhard Reutner-Fischer wrote: > On Thu, 17 Nov 2022 18:52:36 -0500 > Jason Merrill <jason@redhat.com> wrote: > >> On 11/17/22 14:02, Bernhard Reutner-Fischer wrote: >>> On Thu, 17 Nov 2022 09:53:32 -0500 >>> Jason Merrill <jason@redhat.com> wrote: > >>>> Instead, you want to copy the location for instantiations, i.e. check >>>> DECL_TEMPLATE_INSTANTIATION instead of !DECL_USE_TEMPLATE. >>> >>> No, that makes no difference. >> >> Hmm, when I stop there when processing the instantiation the template's >> DECL_RESULT has the right location information, e.g. for >> >> template <class T> int f() { return 42; } >> >> int main() >> { >> f<int>(); >> } >> >> #1 0x0000000000f950e8 in instantiate_body (pattern=<template_decl >> 0x7ffff7ff5080 f>, args=<tree_vec 0x7fffe9712ae0>, d=<function_decl >> 0x7fffe971e600 f>, nested_p=false) at /home/jason/gt/gcc/cp/pt.cc:26470 >> #0 start_preparsed_function (decl1=<function_decl 0x7fffe971e600 f>, >> attrs=<tree 0x0>, flags=1) at /home/jason/gt/gcc/cp/decl.cc:17252 >> (gdb) p expand_location (input_location) >> $13 = {file = 0x4962370 "wa.C", line = 1, column = 24, data = 0x0, sysp >> = false} >> (gdb) p expand_location (DECL_SOURCE_LOCATION (DECL_RESULT >> (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1))))) >> $14 = {file = 0x4962370 "wa.C", line = 1, column = 20, data = 0x0, sysp >> = false} > > Yes, that works. Sorry if i was not clear: The thing in the cover > letter in this series does not work, the mini_vector reduced testcase > from the libstdc++-v3/include/ext/bitmap_allocator.h. > class template member function return type location, would that be it? > > AFAIR the problem was that that these member functions get their result > decl late. When they get them, there are no > declspecs->locations[ds_type_spec] around anywhere to tuck that on the > resdecl. While the result decl is clear, there is no obvious way where > to store the ds_type_spec (somewhere in the template, as you told me). > > Back then I tried moving the resdecl building from > start_preparsed_function to grokfndecl but that did not work out easily > IIRC and i ultimately gave up to move stuff around rather blindly. > I also tried to find a spot where i could store the ds_type_spec locus > somewhere in grokmethod, but i think the problem was the same, i had > just the type where i cannot store a locus and did not find a place > where i could smuggle the locus along. Ah, so the problem is deferred parsing of methods, rather than templates. Building the DECL_RESULT sooner does seem like the right approach to handling that, whether that's in grokfndecl or grokmethod. > So, to make that clear. Your template function (?) works: > > $ XXX=1 ./xg++ -B. -S -o /dev/null ../tmp4/return-narrow-2j.cc > ../tmp4/return-narrow-2j.cc: In function ‘int f()’: > ../tmp4/return-narrow-2j.cc:1:20: warning: result decl locus sample > 1 | template <class T> int f() { return 42; } > | ^~~ > | the return type > ../tmp4/return-narrow-2j.cc: In function ‘int main()’: > ../tmp4/return-narrow-2j.cc:3:1: warning: result decl locus sample > 3 | int main() > | ^~~ > | the return type > ../tmp4/return-narrow-2j.cc: In instantiation of ‘int f() [with T = int]’: > ../tmp4/return-narrow-2j.cc:5:10: required from here > ../tmp4/return-narrow-2j.cc:1:20: warning: result decl locus sample > 1 | template <class T> int f() { return 42; } > | ^~~ > | the return type > > > The class member fn not so much (IMHO, see attached): > > $ XXX=1 ./xg++ -B. -S -o /dev/null ../tmp4/return-narrow-2.cc > ../tmp4/return-narrow-2.cc: In member function ‘const long unsigned int __mini_vector< <template-parameter-1-1> >::_M_space_left()’: > ../tmp4/return-narrow-2.cc:9:3: warning: result decl locus sample > 9 | { return _M_finish != 0; } > | ^ > | the return type > ../tmp4/return-narrow-2.cc: In instantiation of ‘const long unsigned int __mini_vector< <template-parameter-1-1> >::_M_space_left() [with <template-parameter-1-1> = std::pair<long int, long int>]’: > ../tmp4/return-narrow-2.cc:11:17: required from here > ../tmp4/return-narrow-2.cc:9:3: warning: result decl locus sample > 9 | { return _M_finish != 0; } > | ^ > | the return type > ../tmp4/return-narrow-2.cc: In instantiation of ‘const long unsigned int __mini_vector< <template-parameter-1-1> >::_M_space_left() [with <template-parameter-1-1> = int]’: > ../tmp4/return-narrow-2.cc:12:17: required from here > ../tmp4/return-narrow-2.cc:9:3: warning: result decl locus sample > 9 | { return _M_finish != 0; } > | ^ > | the return type > > >> >>> But really I'm not interested in the template case, i only mentioned >>> them because they don't work and in case somebody wanted to have correct >>> locations. >>> I remember just frustration when i looked at those a year ago. >> >> I'd like to get the template case right while we're looking at it. I >> guess I can add that myself if you're done trying. >> >>> Is the hunk for normal functions OK for trunk? >> >> You also need a testcase for the desired behavior, with e.g. >> { dg-error "23:" } > > I'd have to think about how to test that with trunk, yes. > There are no existing warnings that want to point to the return type, > are there? Good point. Do any of your later patches add such a warning? > Maybe a g++.dg/plugin/result_decl_plugin.c then. > > set plugin_test_list [list > hmz. That strikes me as not all that flexible. > We could glob *_plugin.[cC][c]*, and have foo_plugin.lst contain it's > files. Whatever. > > thanks,
On Fri, 18 Nov 2022 11:06:29 -0500 Jason Merrill <jason@redhat.com> wrote: > Ah, so the problem is deferred parsing of methods, rather than > templates. Building the DECL_RESULT sooner does seem like the right > approach to handling that, whether that's in grokfndecl or grokmethod. > >> I'd like to get the template case right while we're looking at it. I > >> guess I can add that myself if you're done trying. Please do, i'd be glad if you could take care of these locations. It icks me that they are wrong, and be it just for the sake of QOI :) > >>> Is the hunk for normal functions OK for trunk? > >> > >> You also need a testcase for the desired behavior, with e.g. > >> { dg-error "23:" } > > > > I'd have to think about how to test that with trunk, yes. > > There are no existing warnings that want to point to the return type, > > are there? > > Good point. Do any of your later patches add such a warning? I didn't mean to have that -Wtype-demotion applied in it's current form, or at all, so no. I was curious if anybody liked the idea of pointing out such code though. I've had no feedback but everybody is or was busy with end of stage3 and real work, so that's expected. The only real purpose i had for it was to find places in the Fortran FE that could use narrower types, bools for the most part. IMHO it would be a nice thing to have, but then, embedded software usually is cautious to use sensible types in the first place and the rest doesn't really care anyway, supposedly. Maybe it would have made more sense to just do an IPA pass that does the demotion silently where it's feasable. As to the test, i don't think these locations in the c++ FE are changed all that often, so chances are rather low that they would be broken once in. So, short of trying to use the result decl locus for any existing -Wreturn-type, -Waggregate-return, -Wno-return-local-addr, -Wsuggest-attribute=[pure|const|noreturn|format|malloc] or another existing warning that would be concerned, we could, as said, have a plugin with fix-it hints and ideally -fdiagnostics-generate-patch to test these bits. Patch generation has the advantage that it will ICE more often than not if asked to generate patches for locations that have a negative relative start (think: memcpy(...,..., -7)), which you can get easily if the locations are off IMHO. > > Maybe a g++.dg/plugin/result_decl_plugin.c then.
Hi Jason!
Possible test.
An existing test might be to equip the existing warning for
bool unsigned double meh(void) {return 0;}
with a fix-it hint instead of the brief
error: two or more data types in declaration of ‘meh’.
Likewise for
bool unsigned meh(void) {return 0;}
error: ‘unsigned’ specified with ‘bool’
so we wouldn't need a plugin, and it might even be useful? ;)
cheers,
* g++.dg/plugin/plugin.exp: Add new test.
* g++.dg/plugin/result-decl-plugin-test-1.C: New test.
* g++.dg/plugin/result-decl-plugin-test-2.C: New test.
* g++.dg/plugin/result_decl_plugin.C: New test.
---
gcc/testsuite/g++.dg/plugin/plugin.exp | 3 +
.../g++.dg/plugin/result-decl-plugin-test-1.C | 28 +++++++++
.../g++.dg/plugin/result-decl-plugin-test-2.C | 61 +++++++++++++++++++
.../g++.dg/plugin/result_decl_plugin.C | 57 +++++++++++++++++
4 files changed, 149 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-1.C
create mode 100644 gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-2.C
create mode 100644 gcc/testsuite/g++.dg/plugin/result_decl_plugin.C
diff --git a/gcc/testsuite/g++.dg/plugin/plugin.exp b/gcc/testsuite/g++.dg/plugin/plugin.exp
index b5fb42fa77a..f2b526b4704 100644
--- a/gcc/testsuite/g++.dg/plugin/plugin.exp
+++ b/gcc/testsuite/g++.dg/plugin/plugin.exp
@@ -80,6 +80,9 @@ set plugin_test_list [list \
show-template-tree-color-labels.C \
show-template-tree-color-no-elide-type.C } \
{ comment_plugin.c comments-1.C } \
+ { result_decl_plugin.C \
+ result-decl-plugin-test-1.C \
+ result-decl-plugin-test-2.C } \
]
foreach plugin_test $plugin_test_list {
diff --git a/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-1.C b/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-1.C
new file mode 100644
index 00000000000..bd323181d70
--- /dev/null
+++ b/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-1.C
@@ -0,0 +1,28 @@
+/* Verify that class member functions result decl have the correct location. */
+// { dg-options "-fdiagnostics-generate-patch" }
+namespace std { template < typename, typename > struct pair; }
+template < typename > struct __mini_vector
+{
+ int _M_finish;
+ const
+ unsigned long
+ __attribute__((deprecated))
+ _M_space_left()
+ { return _M_finish != 0; }
+};
+ template class __mini_vector< std::pair< long, long > >;
+ template class __mini_vector< int >;
+#if 0
+{ dg-begin-multiline-output "" }
+@@ -5,7 +5,7 @@ template < typename > struct __mini_vect
+ {
+ int _M_finish;
+ const
+- unsigned long
++ bool
+ __attribute__((deprecated))
+ _M_space_left()
+ { return _M_finish != 0; }
+
+{ dg-end-multiline-output "" }
+#endif
diff --git a/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-2.C b/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-2.C
new file mode 100644
index 00000000000..385a7ef482f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-2.C
@@ -0,0 +1,61 @@
+/* Verify that template functions result decl have the correct location. */
+// { dg-options "-fdiagnostics-generate-patch" }
+template <class T>
+int
+f()
+{
+ return 42;
+}
+int main()
+{
+ f<int>();
+}
+unsigned long long huh(void)
+{
+ return 1ULL;
+}
+#if 0
+{ dg-begin-multiline-output "" }
+g++.dg/plugin/result-decl-plugin-test-2.C:4:1: warning: Function ‘f’ result location
+ 4 | int
+ | ^~~
+ | bool
+g++.dg/plugin/result-decl-plugin-test-2.C:9:1: warning: Function ‘main’ result location
+ 9 | int main()
+ | ^~~
+ | bool
+g++.dg/plugin/result-decl-plugin-test-2.C:13:28: warning: Function ‘huh’ result location
+ 13 | unsigned long long huh(void)
+ | ^
+ | bool
+g++.dg/plugin/result-decl-plugin-test-2.C: In instantiation of ‘int f() [with T = int]’:
+g++.dg/plugin/result-decl-plugin-test-2.C:11:10: required from here
+g++.dg/plugin/result-decl-plugin-test-2.C:4:1: warning: Function ‘f’ result location
+ 4 | int
+ | ^~~
+ | bool
+--- g++.dg/plugin/result-decl-plugin-test-2.C
++++ g++.dg/plugin/result-decl-plugin-test-2.C
+@@ -1,16 +1,16 @@
+ /* Verify that template functions result decl have the correct location. */
+ // { dg-options "-fdiagnostics-generate-patch" }
+ template <class T>
+-int
++bool
+ f()
+ {
+ return 42;
+ }
+-int main()
++bool main()
+ {
+ f<int>();
+ }
+-unsigned long long huh(void)
++unsigned long long huh(voidbool
+ {
+ return 1ULL;
+ }
+{ dg-end-multiline-output "" }
+#endif
+// Note: f() should not +bbool with an off-by-one for the start 'b' !
diff --git a/gcc/testsuite/g++.dg/plugin/result_decl_plugin.C b/gcc/testsuite/g++.dg/plugin/result_decl_plugin.C
new file mode 100644
index 00000000000..40f54a6acfe
--- /dev/null
+++ b/gcc/testsuite/g++.dg/plugin/result_decl_plugin.C
@@ -0,0 +1,57 @@
+/* A plugin example that points at the location of function decl result decl */
+/* This file is part of GCC */
+/* { dg-options "-O" } */
+#include "gcc-plugin.h"
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tree.h"
+#include "tree-pass.h"
+#include "intl.h"
+#include "diagnostic.h"
+#include "gcc-rich-location.h"
+#include "langhooks.h"
+#include "plugin-version.h"
+
+static struct plugin_info _this_info = {
+ .version = "1",
+ .help = "None",
+};
+/* Callback function to invoke after GCC finishes a declaration. */
+
+static void plugin_finish_decl (void *event_data, void *data)
+{
+ tree decl = (tree) event_data;
+ if (!decl || TREE_CODE (decl) != FUNCTION_DECL)
+ return;
+
+ tree logical_1_t = boolean_type_node;
+ tree logical_1_n = DECL_NAME (TYPE_NAME (logical_1_t));
+ location_t result_loc = DECL_SOURCE_LOCATION (DECL_RESULT (decl));
+ gcc_rich_location richloc (result_loc);
+ richloc.add_fixit_replace (result_loc, IDENTIFIER_POINTER (logical_1_n));
+
+ warning_at (&richloc, 0, G_("Function %qs result location"),
+ IDENTIFIER_POINTER (DECL_NAME (decl)));
+}
+
+int
+plugin_init (struct plugin_name_args *plugin_info,
+ struct plugin_gcc_version *version)
+{
+ if (!plugin_default_version_check (version, &gcc_version))
+ {
+ error(G_("incompatible gcc/plugin versions"));
+ return 1;
+ }
+
+ const char *plugin_name = plugin_info->base_name;
+
+ register_callback(plugin_name, PLUGIN_INFO, NULL, &_this_info);
+ register_callback (plugin_name, PLUGIN_FINISH_PARSE_FUNCTION,
+ plugin_finish_decl, NULL);
+ return 0;
+}
+
+/* Announce that we are GPL. */
+int plugin_is_GPL_compatible;
Hi Jason!
The "meh" of result-decl-plugin-test-2.C should likely be omitted,
grokdeclarator would need some changes to add richloc hints and we would not
be able to make a reliable guess what to remove precisely.
C.f. /* Check all other uses of type modifiers. */
Furthermore it is unrelated to DECL_RESULT so not of direct interest
here. The other tests in test-2.C, f() and huh() should work though.
I don't know if it's acceptable to change ipa-pure-const to make the
missing noreturn warning more precise and emit a fixit-hint. At least it
would be a real test for the DECL_RESULT and would spare us the plugin.
HTH,
gcc/cp/ChangeLog:
* decl.cc (start_preparsed_function): Set the result decl source
location to the location of the typespec.
(start_function): Likewise.
gcc/ChangeLog:
* ipa-pure-const.cc (suggest_attribute): Add fixit-hint for the
noreturn attribute.
gcc/testsuite/ChangeLog:
* c-c++-common/pr68833-1.c: Adjust noreturn warning line number.
* gcc.dg/noreturn-1.c: Likewise.
* g++.dg/plugin/plugin.exp: Add new plugin test.
* g++.dg/other/resultdecl-1.C: New test.
* g++.dg/plugin/result-decl-plugin-test-1.C: New test.
* g++.dg/plugin/result-decl-plugin-test-2.C: New test.
* g++.dg/plugin/result_decl_plugin.C: New test.
---
gcc/cp/decl.cc | 26 +++++++-
gcc/ipa-pure-const.cc | 14 ++++-
gcc/testsuite/c-c++-common/pr68833-1.c | 2 +-
gcc/testsuite/g++.dg/other/resultdecl-1.C | 32 ++++++++++
gcc/testsuite/g++.dg/plugin/plugin.exp | 3 +
.../g++.dg/plugin/result-decl-plugin-test-1.C | 31 ++++++++++
.../g++.dg/plugin/result-decl-plugin-test-2.C | 59 +++++++++++++++++++
.../g++.dg/plugin/result_decl_plugin.C | 53 +++++++++++++++++
gcc/testsuite/gcc.dg/noreturn-1.c | 2 +-
9 files changed, 218 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/other/resultdecl-1.C
create mode 100644 gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-1.C
create mode 100644 gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-2.C
create mode 100644 gcc/testsuite/g++.dg/plugin/result_decl_plugin.C
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index d28889ed865..0c053b6d19f 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -17235,6 +17235,17 @@ start_preparsed_function (tree decl1, tree attrs, int flags)
cp_apply_type_quals_to_decl (cp_type_quals (restype), resdecl);
}
+ /* Set the result decl source location to the location of the typespec. */
+ if (DECL_RESULT (decl1)
+ && DECL_TEMPLATE_INSTANTIATION (decl1)
+ && DECL_TEMPLATE_INFO (decl1)
+ && DECL_TI_TEMPLATE (decl1)
+ && DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1))
+ && DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1))))
+ DECL_SOURCE_LOCATION (DECL_RESULT (decl1))
+ = DECL_SOURCE_LOCATION (
+ DECL_RESULT (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (decl1))));
+
/* Record the decl so that the function name is defined.
If we already have a decl for this name, and it is a FUNCTION_DECL,
use the old decl. */
@@ -17532,7 +17543,20 @@ start_function (cp_decl_specifier_seq *declspecs,
gcc_assert (same_type_p (TREE_TYPE (TREE_TYPE (decl1)),
integer_type_node));
- return start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT);
+ bool ret = start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT);
+
+ /* decl1 might be ggc_freed here. */
+ decl1 = current_function_decl;
+
+ tree result;
+ /* Set the result decl source location to the location of the typespec. */
+ if (ret
+ && TREE_CODE (decl1) == FUNCTION_DECL
+ && declspecs->locations[ds_type_spec] != UNKNOWN_LOCATION
+ && (result = DECL_RESULT (decl1)) != NULL_TREE
+ && DECL_SOURCE_LOCATION (result) == input_location)
+ DECL_SOURCE_LOCATION (result) = declspecs->locations[ds_type_spec];
+ return ret;
}
/* Returns true iff an EH_SPEC_BLOCK should be created in the body of
diff --git a/gcc/ipa-pure-const.cc b/gcc/ipa-pure-const.cc
index 572a6da274f..1c80034f38d 100644
--- a/gcc/ipa-pure-const.cc
+++ b/gcc/ipa-pure-const.cc
@@ -63,6 +63,7 @@ along with GCC; see the file COPYING3. If not see
#include "ipa-fnsummary.h"
#include "symtab-thunks.h"
#include "dbgcnt.h"
+#include "gcc-rich-location.h"
/* Lattice values for const and pure functions. Everything starts out
being const, then may drop to pure and then neither depending on
@@ -212,7 +213,18 @@ suggest_attribute (int option, tree decl, bool known_finite,
if (warned_about->contains (decl))
return warned_about;
warned_about->add (decl);
- warning_at (DECL_SOURCE_LOCATION (decl),
+
+ gcc_rich_location richloc (option == OPT_Wsuggest_attribute_noreturn
+ ? DECL_SOURCE_LOCATION (DECL_RESULT (decl))
+ : DECL_SOURCE_LOCATION (decl));
+ if (option == OPT_Wsuggest_attribute_noreturn)
+ {
+ richloc.add_fixit_replace (IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (
+ void_type_node))));
+ richloc.add_fixit_insert_before (DECL_SOURCE_LOCATION (decl),
+ " __attribute__((__noreturn__)) ");
+ }
+ warning_at (&richloc,
option,
known_finite
? G_("function might be candidate for attribute %qs")
diff --git a/gcc/testsuite/c-c++-common/pr68833-1.c b/gcc/testsuite/c-c++-common/pr68833-1.c
index a6aefad5c98..b27b783d61a 100644
--- a/gcc/testsuite/c-c++-common/pr68833-1.c
+++ b/gcc/testsuite/c-c++-common/pr68833-1.c
@@ -15,7 +15,7 @@ f1 (const char *fmt)
extern void f2 (void);
void
-f2 (void) /* { dg-error "candidate for attribute 'noreturn'" "detect noreturn candidate" } */
+f2 (void) /* { dg-error "candidate for attribute 'noreturn'" "detect noreturn candidate" { target *-*-* } .-1 } */
{
__builtin_exit (0);
}
diff --git a/gcc/testsuite/g++.dg/other/resultdecl-1.C b/gcc/testsuite/g++.dg/other/resultdecl-1.C
new file mode 100644
index 00000000000..e3951c339a9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/other/resultdecl-1.C
@@ -0,0 +1,32 @@
+/* Verify correct result decl location. */
+// { dg-options "-fdiagnostics-generate-patch -O1 -fipa-pure-const -Wsuggest-attribute=noreturn" }
+//
+// foo4: rem int, add: void attribute((noreturn))
+
+
+
+int
+foo4(void)
+{ while (1); }
+
+
+#if 0
+// { dg-warning "function might be candidate for attribute .noreturn." "" { target *-*-* } .-6 } // the fnname is at line .-5 though
+// prune the diff path..
+/* { dg-regexp "\\-\\-\\- .*" } */
+/* { dg-regexp "\\+\\+\\+ .*" } */
+{ dg-begin-multiline-output "" }
+@@ -5,8 +5,8 @@
+
+
+
+-int
+-foo4(void)
++void
++ __attribute__((__noreturn__)) foo4(void)
+ { while (1); }
+
+
+{ dg-end-multiline-output "" }
+#endif
+
diff --git a/gcc/testsuite/g++.dg/plugin/plugin.exp b/gcc/testsuite/g++.dg/plugin/plugin.exp
index b5fb42fa77a..21d7ef0313e 100644
--- a/gcc/testsuite/g++.dg/plugin/plugin.exp
+++ b/gcc/testsuite/g++.dg/plugin/plugin.exp
@@ -80,6 +80,9 @@ set plugin_test_list [list \
show-template-tree-color-labels.C \
show-template-tree-color-no-elide-type.C } \
{ comment_plugin.c comments-1.C } \
+ { result_decl_plugin.C \
+ result-decl-plugin-test-1.C \
+ result-decl-plugin-test-2.C } \
]
foreach plugin_test $plugin_test_list {
diff --git a/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-1.C b/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-1.C
new file mode 100644
index 00000000000..1bbdc4c8c44
--- /dev/null
+++ b/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-1.C
@@ -0,0 +1,31 @@
+/* Verify that class member functions result decl have the correct location. */
+// { dg-options "-fdiagnostics-generate-patch" }
+namespace std { template < typename, typename > struct pair; }
+template < typename > struct __mini_vector
+{
+ int _M_finish;
+ const
+ unsigned long
+ __attribute__((deprecated))
+ _M_space_left()
+ { return _M_finish != 0; }
+};
+ template class __mini_vector< std::pair< long, long > >;
+ template class __mini_vector< int >;
+#if 0
+// { dg-warning "Function ._M_space_left. result location" "" { target *-*-* } .-5 }
+// prune the diff path..
+/* { dg-regexp "\\-\\-\\- .*" } */
+/* { dg-regexp "\\+\\+\\+ .*" } */
+{ dg-begin-multiline-output "" }
+@@ -8,7 +8,7 @@ template < typename > struct __mini_vect
+ {
+ int _M_finish;
+ const
+- unsigned long
++ bool
+ __attribute__((deprecated))
+ _M_space_left()
+ { return _M_finish != 0; }
+{ dg-end-multiline-output "" }
+#endif
diff --git a/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-2.C b/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-2.C
new file mode 100644
index 00000000000..c67388195a1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/plugin/result-decl-plugin-test-2.C
@@ -0,0 +1,59 @@
+/* Verify that template functions result decl have the correct location. */
+// { dg-options "-fdiagnostics-generate-patch" }
+
+
+
+template <class T>
+int
+f()
+{
+ return 42;
+}
+int main()
+{
+ f<int>();
+}
+unsigned long long huh(void)
+{
+ return 1ULL;
+}
+bool unsigned meh(void) {return 0;}
+
+
+
+
+#if 0
+// { dg-warning "Function .f. result location" "" { target *-*-* } .-19 }
+// { dg-warning "Function .huh. result location" "" { target *-*-* } .-11 }
+// { dg-warning "Function .meh. result location" "" { target *-*-* } .-8 }
+// prune the diff path..
+/* { dg-regexp "\\-\\-\\- .*" } */
+/* { dg-regexp "\\+\\+\\+ .*" } */
+// { dg-prune ".unsigned. specified with .bool." }
+{ dg-begin-multiline-output "" }
+@@ -4,7 +4,7 @@
+
+
+ template <class T>
+-int
++bool
+ f()
+ {
+ return 42;
+@@ -13,11 +13,11 @@
+ {
+ f<int>();
+ }
+-unsigned long long huh(void)
++bool huh(void)
+ {
+ return 1ULL;
+ }
+-bool unsigned meh(void) {return 0;}
++bool meh(void) {return 0;}
+
+
+
+{ dg-end-multiline-output "" }
+#endif
+// Note: f() should NOT +bbool but just +bool
diff --git a/gcc/testsuite/g++.dg/plugin/result_decl_plugin.C b/gcc/testsuite/g++.dg/plugin/result_decl_plugin.C
new file mode 100644
index 00000000000..9ab408e8a65
--- /dev/null
+++ b/gcc/testsuite/g++.dg/plugin/result_decl_plugin.C
@@ -0,0 +1,53 @@
+/* A plugin example that points at the location of function result decl */
+/* This file is part of GCC */
+/* { dg-options "-O" } */
+#include "gcc-plugin.h"
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tree.h"
+#include "intl.h"
+#include "diagnostic.h"
+#include "gcc-rich-location.h"
+#include "langhooks.h"
+#include "plugin-version.h"
+
+/* Callback function to invoke after GCC finishes a function definition. */
+
+static void plugin_finish_decl (void *event_data, void *data)
+{
+ tree decl = (tree) event_data;
+ if (!decl || TREE_CODE (decl) != FUNCTION_DECL)
+ return;
+ if (MAIN_NAME_P (DECL_NAME (decl)))
+ return;
+
+ tree logical_1_t = boolean_type_node;
+ tree logical_1_n = DECL_NAME (TYPE_NAME (logical_1_t));
+ location_t result_loc = DECL_SOURCE_LOCATION (DECL_RESULT (decl));
+ gcc_rich_location richloc (result_loc);
+ richloc.add_fixit_replace (result_loc, IDENTIFIER_POINTER (logical_1_n));
+
+ warning_at (&richloc, 0, G_("Function %qs result location"),
+ IDENTIFIER_POINTER (DECL_NAME (decl)));
+}
+
+int
+plugin_init (struct plugin_name_args *plugin_info,
+ struct plugin_gcc_version *version)
+{
+ if (!plugin_default_version_check (version, &gcc_version))
+ {
+ error(G_("incompatible gcc/plugin versions"));
+ return 1;
+ }
+
+ const char *plugin_name = plugin_info->base_name;
+
+ register_callback (plugin_name, PLUGIN_FINISH_PARSE_FUNCTION,
+ plugin_finish_decl, NULL);
+ return 0;
+}
+
+/* Announce that we are GPL. */
+int plugin_is_GPL_compatible;
diff --git a/gcc/testsuite/gcc.dg/noreturn-1.c b/gcc/testsuite/gcc.dg/noreturn-1.c
index cdbfb8dd667..a713ee924fc 100644
--- a/gcc/testsuite/gcc.dg/noreturn-1.c
+++ b/gcc/testsuite/gcc.dg/noreturn-1.c
@@ -25,7 +25,7 @@ foo3(void)
extern void foo4(void);
void
-foo4(void) /* { dg-warning "candidate for attribute 'noreturn'" "detect noreturn candidate" } */
+foo4(void) /* { dg-warning "candidate for attribute 'noreturn'" "detect noreturn candidate" { target *-*-* } .-1 } */
{
exit(0);
}
On 11/20/22 12:06, Bernhard Reutner-Fischer wrote: > Hi Jason! > > The "meh" of result-decl-plugin-test-2.C should likely be omitted, > grokdeclarator would need some changes to add richloc hints and we would not > be able to make a reliable guess what to remove precisely. > C.f. /* Check all other uses of type modifiers. */ > Furthermore it is unrelated to DECL_RESULT so not of direct interest > here. The other tests in test-2.C, f() and huh() should work though. > > I don't know if it's acceptable to change ipa-pure-const to make the > missing noreturn warning more precise and emit a fixit-hint. At least it > would be a real test for the DECL_RESULT and would spare us the plugin. The main problem I see with that change is that the syntax of the fixit might be wrong for non-C-family front-ends. Here's a version of the patch that fixes template/method handling, and adjusts -Waggregate-return as well:
On 11/22/22 15:25, Jason Merrill wrote: > On 11/20/22 12:06, Bernhard Reutner-Fischer wrote: >> Hi Jason! >> >> The "meh" of result-decl-plugin-test-2.C should likely be omitted, >> grokdeclarator would need some changes to add richloc hints and we >> would not >> be able to make a reliable guess what to remove precisely. >> C.f. /* Check all other uses of type modifiers. */ >> Furthermore it is unrelated to DECL_RESULT so not of direct interest >> here. The other tests in test-2.C, f() and huh() should work though. >> >> I don't know if it's acceptable to change ipa-pure-const to make the >> missing noreturn warning more precise and emit a fixit-hint. At least it >> would be a real test for the DECL_RESULT and would spare us the plugin. > > The main problem I see with that change is that the syntax of the fixit > might be wrong for non-C-family front-ends. > > Here's a version of the patch that fixes template/method handling, and > adjusts -Waggregate-return as well: Actually, that broke some of the spaceship tests, fixed by this version:
On 11/23/22 10:28, Jason Merrill wrote: > On 11/22/22 15:25, Jason Merrill wrote: >> On 11/20/22 12:06, Bernhard Reutner-Fischer wrote: >>> Hi Jason! >>> >>> The "meh" of result-decl-plugin-test-2.C should likely be omitted, >>> grokdeclarator would need some changes to add richloc hints and we >>> would not >>> be able to make a reliable guess what to remove precisely. >>> C.f. /* Check all other uses of type modifiers. */ >>> Furthermore it is unrelated to DECL_RESULT so not of direct interest >>> here. The other tests in test-2.C, f() and huh() should work though. >>> >>> I don't know if it's acceptable to change ipa-pure-const to make the >>> missing noreturn warning more precise and emit a fixit-hint. At least it >>> would be a real test for the DECL_RESULT and would spare us the plugin. >> >> The main problem I see with that change is that the syntax of the >> fixit might be wrong for non-C-family front-ends. >> >> Here's a version of the patch that fixes template/method handling, and >> adjusts -Waggregate-return as well: > > Actually, that broke some of the spaceship tests, fixed by this version: Here's what I'm applying:
On 2 December 2022 20:30:56 CET, Jason Merrill <jason@redhat.com> wrote:
>Here's what I'm applying:
I meant to play with it during the weekend,
looks good, many thanks for taking care of this!
cheers,
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index 6e98ea35a39..ed40815e645 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -17449,6 +17449,8 @@ start_function (cp_decl_specifier_seq *declspecs, tree attrs) { tree decl1; + tree result; + bool ret; decl1 = grokdeclarator (declarator, declspecs, FUNCDEF, 1, &attrs); invoke_plugin_callbacks (PLUGIN_START_PARSE_FUNCTION, decl1); @@ -17461,7 +17463,18 @@ start_function (cp_decl_specifier_seq *declspecs, gcc_assert (same_type_p (TREE_TYPE (TREE_TYPE (decl1)), integer_type_node)); - return start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); + ret = start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT); + + /* decl1 might be ggc_freed here. */ + decl1 = current_function_decl; + + /* Set the result decl source location to the location of the typespec. */ + if (TREE_CODE (decl1) == FUNCTION_DECL + && declspecs->locations[ds_type_spec] != UNKNOWN_LOCATION + && (result = DECL_RESULT (decl1)) != NULL_TREE + && DECL_SOURCE_LOCATION (result) == input_location) + DECL_SOURCE_LOCATION (result) = declspecs->locations[ds_type_spec]; + return ret; } /* Returns true iff an EH_SPEC_BLOCK should be created in the body of