| Message ID | e2627959-892d-9abe-636d-880b4180d47d@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Drop MALLOC attribute for void functions. | expand |
Hi, On Mon, Feb 17 2020, Martin Liška wrote: > Hello. > > As mentioned in the PR, we end up with a void function > call that has set MALLOC attribute. That causes problems in RTL > expansion. > > I believe a proper fix is to drop the attribute when a callgraph > clone with void type is created. > > I would like to see Martin's comment about the hunk in propagate_malloc > which is maybe suboptimal and one can find a better way? > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > It fixes LTO bootstrap on ppc64le. > It looks sensible to me. I did not quite took into consideration that ipa-pure-const can add the attribute and only dropped in tree-inline's tree_function_versioning. Just the hunk in ipa_param_adjustments::adjust_decl could be simplified a tiny bit... > Thanks, > Martin > > gcc/ChangeLog: > > 2020-02-17 Martin Liska <mliska@suse.cz> > > PR ipa/93583 > * cgraph.c (cgraph_node::verify_node): Verify MALLOC attribute > and return type of functions. > * ipa-param-manipulation.c (ipa_param_adjustments::adjust_decl): > Drop MALLOC attribute for void functions. > * ipa-pure-const.c (propagate_malloc): Do not set malloc flag > for void functions. > > gcc/testsuite/ChangeLog: > > 2020-02-17 Martin Liska <mliska@suse.cz> > > PR ipa/93583 > * gcc.dg/ipa/pr93583.c: New test. > --- > gcc/cgraph.c | 6 ++++++ > gcc/ipa-param-manipulation.c | 4 ++++ > gcc/ipa-pure-const.c | 9 ++++++--- > gcc/testsuite/gcc.dg/ipa/pr93583.c | 15 +++++++++++++++ > 4 files changed, 31 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93583.c > [...] > diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c > index 19ec87358fa..5f6f0575b06 100644 > --- a/gcc/ipa-param-manipulation.c > +++ b/gcc/ipa-param-manipulation.c > @@ -410,6 +410,10 @@ ipa_param_adjustments::adjust_decl (tree orig_decl) > DECL_VIRTUAL_P (new_decl) = 0; > DECL_LANG_SPECIFIC (new_decl) = NULL; > > + /* Drop MALLOC attribute for a void function. */ > + if (VOID_TYPE_P (TREE_TYPE (TREE_TYPE (new_decl)))) > + DECL_IS_MALLOC (new_decl) = 0; by just testing m_skip_return here (but it is no big deal). Thanks! Martin
On Mon, Feb 17, 2020 at 1:16 PM Martin Liška <mliska@suse.cz> wrote: > > Hello. > > As mentioned in the PR, we end up with a void function > call that has set MALLOC attribute. That causes problems in RTL > expansion. > > I believe a proper fix is to drop the attribute when a callgraph > clone with void type is created. > > I would like to see Martin's comment about the hunk in propagate_malloc > which is maybe suboptimal and one can find a better way? > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > It fixes LTO bootstrap on ppc64le. Is the ipa-pure-const.c hunk really necessary? malloc_candidate_p shouldn't see any such function as candidate. If so it should be fixed there? Thanks, Richard. > Thanks, > Martin > > gcc/ChangeLog: > > 2020-02-17 Martin Liska <mliska@suse.cz> > > PR ipa/93583 > * cgraph.c (cgraph_node::verify_node): Verify MALLOC attribute > and return type of functions. > * ipa-param-manipulation.c (ipa_param_adjustments::adjust_decl): > Drop MALLOC attribute for void functions. > * ipa-pure-const.c (propagate_malloc): Do not set malloc flag > for void functions. > > gcc/testsuite/ChangeLog: > > 2020-02-17 Martin Liska <mliska@suse.cz> > > PR ipa/93583 > * gcc.dg/ipa/pr93583.c: New test. > --- > gcc/cgraph.c | 6 ++++++ > gcc/ipa-param-manipulation.c | 4 ++++ > gcc/ipa-pure-const.c | 9 ++++++--- > gcc/testsuite/gcc.dg/ipa/pr93583.c | 15 +++++++++++++++ > 4 files changed, 31 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93583.c > >
On 2/17/20 3:25 PM, Richard Biener wrote: > On Mon, Feb 17, 2020 at 1:16 PM Martin Liška <mliska@suse.cz> wrote: >> >> Hello. >> >> As mentioned in the PR, we end up with a void function >> call that has set MALLOC attribute. That causes problems in RTL >> expansion. >> >> I believe a proper fix is to drop the attribute when a callgraph >> clone with void type is created. >> >> I would like to see Martin's comment about the hunk in propagate_malloc >> which is maybe suboptimal and one can find a better way? >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> It fixes LTO bootstrap on ppc64le. > > Is the ipa-pure-const.c hunk really necessary? Yes. > malloc_candidate_p shouldn't > see any such function as candidate. If so it should be fixed there? I've got hopefully a proper fix. Which is where we clone funct_state_summary_t summary for the newly cloned IPA SRA function. I'm going to test the patch. Martin > > Thanks, > Richard. > >> Thanks, >> Martin >> >> gcc/ChangeLog: >> >> 2020-02-17 Martin Liska <mliska@suse.cz> >> >> PR ipa/93583 >> * cgraph.c (cgraph_node::verify_node): Verify MALLOC attribute >> and return type of functions. >> * ipa-param-manipulation.c (ipa_param_adjustments::adjust_decl): >> Drop MALLOC attribute for void functions. >> * ipa-pure-const.c (propagate_malloc): Do not set malloc flag >> for void functions. >> >> gcc/testsuite/ChangeLog: >> >> 2020-02-17 Martin Liska <mliska@suse.cz> >> >> PR ipa/93583 >> * gcc.dg/ipa/pr93583.c: New test. >> --- >> gcc/cgraph.c | 6 ++++++ >> gcc/ipa-param-manipulation.c | 4 ++++ >> gcc/ipa-pure-const.c | 9 ++++++--- >> gcc/testsuite/gcc.dg/ipa/pr93583.c | 15 +++++++++++++++ >> 4 files changed, 31 insertions(+), 3 deletions(-) >> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93583.c >> >>
Sorry, I attached wrong patch. Martin
Hi, On Mon, Feb 17 2020, Richard Biener wrote: > On Mon, Feb 17, 2020 at 1:16 PM Martin Liška <mliska@suse.cz> wrote: >> >> Hello. >> >> As mentioned in the PR, we end up with a void function >> call that has set MALLOC attribute. That causes problems in RTL >> expansion. >> >> I believe a proper fix is to drop the attribute when a callgraph >> clone with void type is created. >> >> I would like to see Martin's comment about the hunk in propagate_malloc >> which is maybe suboptimal and one can find a better way? >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> It fixes LTO bootstrap on ppc64le. > > Is the ipa-pure-const.c hunk really necessary? malloc_candidate_p shouldn't > see any such function as candidate. If so it should be fixed there? malloc_candidate_p runs at summary building phase (LTO compile time) and IPA-SRA WPA phase, which actually removes the return value, runs between that and ipa-pure-const WPA phase. So without the hunk, the pass could now happily re-introduce the flag. I have not analyzed the original bug report but I actually believe that the ipa-pure-const hunk is actually the one fixing the core issue there. Otherwise, removal of the attribute in tree-inline.c would have been enough. If we don't like adding another look at the trees in a WPA phase of an IPA pass, ipa-pure const could alternatively test: if (node->param_adjustments && node->param_adjustments->m_skip_return) continue; Martin > > Thanks, > Richard. > >> Thanks, >> Martin >> >> gcc/ChangeLog: >> >> 2020-02-17 Martin Liska <mliska@suse.cz> >> >> PR ipa/93583 >> * cgraph.c (cgraph_node::verify_node): Verify MALLOC attribute >> and return type of functions. >> * ipa-param-manipulation.c (ipa_param_adjustments::adjust_decl): >> Drop MALLOC attribute for void functions. >> * ipa-pure-const.c (propagate_malloc): Do not set malloc flag >> for void functions. >> >> gcc/testsuite/ChangeLog: >> >> 2020-02-17 Martin Liska <mliska@suse.cz> >> >> PR ipa/93583 >> * gcc.dg/ipa/pr93583.c: New test. >> --- >> gcc/cgraph.c | 6 ++++++ >> gcc/ipa-param-manipulation.c | 4 ++++ >> gcc/ipa-pure-const.c | 9 ++++++--- >> gcc/testsuite/gcc.dg/ipa/pr93583.c | 15 +++++++++++++++ >> 4 files changed, 31 insertions(+), 3 deletions(-) >> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93583.c >> >>
On Mon, Feb 17, 2020 at 03:44:53PM +0100, Martin Liška wrote:
> + error ("MALLOC attribute set on a void function");
Why the capitals? Either malloc or %<malloc%> IMSHO.
What is special about void functions, missing lhs? That can be missing
for other functions (where you just don't use the return value, or e.g.
noreturn even if you do)? And otherwise, shouldn't the test be rather
whether the return type is a pointer type? E.g. float or int return
type for malloc attribute isn't very meaningful.
Jakub
On 2/17/20 3:48 PM, Jakub Jelinek wrote: > On Mon, Feb 17, 2020 at 03:44:53PM +0100, Martin Liška wrote: >> + error ("MALLOC attribute set on a void function"); > > Why the capitals? Either malloc or %<malloc%> IMSHO. Sure, I'll fix it. > What is special about void functions, missing lhs? That can be missing > for other functions (where you just don't use the return value, or e.g. > noreturn even if you do)? And otherwise, shouldn't the test be rather > whether the return type is a pointer type? E.g. float or int return > type for malloc attribute isn't very meaningful. Hopefully one can't set malloc attribute to a symbol that does not return a pointer type: head malloc.c float bar(const char*); static float __attribute__((malloc,noinline)) foo(const char *p) { return bar (p); } $ gcc malloc.c -c -O2 malloc.c:3:1: warning: ‘malloc’ attribute ignored [-Wattributes] 3 | { Martin > > Jakub >
On Mon, Feb 17, 2020 at 3:47 PM Martin Liška <mliska@suse.cz> wrote: > > Sorry, I attached wrong patch. Ah yeah, that looks like a better place (the IPA pure-const one). OK with Jakubs suggested changes. Richard. > Martin
On Mon, Feb 17, 2020 at 3:48 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Feb 17, 2020 at 03:44:53PM +0100, Martin Liška wrote: > > + error ("MALLOC attribute set on a void function"); > > Why the capitals? Either malloc or %<malloc%> IMSHO. > What is special about void functions, missing lhs? That can be missing > for other functions (where you just don't use the return value, or e.g. > noreturn even if you do)? And otherwise, shouldn't the test be rather > whether the return type is a pointer type? E.g. float or int return > type for malloc attribute isn't very meaningful. It surely would be better if the expansion code would deal with a "bogus" ECF_MALLOC (ERF_RETURNS_ARG is probably another candidate that can get "wrong" ...). But it doesn't seem as easy as fixing the segfault (see the PR, somehow we fail to emit some insns). Richard. > Jakub >
On 2/18/20 11:09 AM, Richard Biener wrote: > On Mon, Feb 17, 2020 at 3:47 PM Martin Liška <mliska@suse.cz> wrote: >> >> Sorry, I attached wrong patch. > > Ah yeah, that looks like a better place (the IPA pure-const one). OK > with Jakubs > suggested changes. > > Richard. > >> Martin All right. There's hopefully a final version of the patch that includes Jakub's notes. I've just tested that ppc64le-linux-gnu LTO bootstrap. I'll install it if there are no objections. Martin
diff --git a/gcc/cgraph.c b/gcc/cgraph.c index 294b2d392fd..6e99fbb45f4 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -3374,6 +3374,12 @@ cgraph_node::verify_node (void) error ("calls_comdat_local is set outside of a comdat group"); error_found = true; } + if (DECL_IS_MALLOC (decl) + && VOID_TYPE_P (TREE_TYPE (TREE_TYPE (decl)))) + { + error ("MALLOC attribute set on a void function"); + error_found = true; + } for (e = indirect_calls; e; e = e->next_callee) { if (e->aux) diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c index 19ec87358fa..5f6f0575b06 100644 --- a/gcc/ipa-param-manipulation.c +++ b/gcc/ipa-param-manipulation.c @@ -410,6 +410,10 @@ ipa_param_adjustments::adjust_decl (tree orig_decl) DECL_VIRTUAL_P (new_decl) = 0; DECL_LANG_SPECIFIC (new_decl) = NULL; + /* Drop MALLOC attribute for a void function. */ + if (VOID_TYPE_P (TREE_TYPE (TREE_TYPE (new_decl)))) + DECL_IS_MALLOC (new_decl) = 0; + return new_decl; } diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c index ccd0918c120..315134d2a94 100644 --- a/gcc/ipa-pure-const.c +++ b/gcc/ipa-pure-const.c @@ -1973,9 +1973,12 @@ propagate_malloc (void) node->dump_name ()); bool malloc_decl_p = DECL_IS_MALLOC (node->decl); - node->set_malloc_flag (true); - if (!malloc_decl_p && warn_suggest_attribute_malloc) - warn_function_malloc (node->decl); + if (!VOID_TYPE_P (TREE_TYPE (TREE_TYPE (node->decl)))) + { + node->set_malloc_flag (true); + if (!malloc_decl_p && warn_suggest_attribute_malloc) + warn_function_malloc (node->decl); + } } } diff --git a/gcc/testsuite/gcc.dg/ipa/pr93583.c b/gcc/testsuite/gcc.dg/ipa/pr93583.c new file mode 100644 index 00000000000..30ef506553d --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr93583.c @@ -0,0 +1,15 @@ +/* PR ipa/93583 */ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +void *bar(const char*); +static void *__attribute__((malloc,noinline)) foo(const char *p) +{ + return bar (p); +} + +int main(int argc, char **argv) +{ + foo (argv[0]); + return 0; +}