| Message ID | 52CD14C8.5050305@st.com |
|---|---|
| State | New |
| Headers | show |
On 01/08/14 02:05, Laurent Alfonsi wrote: > All, > > I was looking at PR49718. I have enclosed a simple fix for this bug report. > > 2014-01-07 Laurent Alfonsi <laurent.alfonsi@st.com> > > * c-family/c-common.c (handle_no_instrument_function_attribute): Allow > no_instrument_function attribute in class member > definition/declaration. > > > Looking at the implementation of the function attributes, I see no > reason anymore to keep this error message. > Let me know if I missed something. > I have also added a testcase in the enclosed patch. > > 2014-01-07 Laurent Alfonsi <laurent.alfonsi@st.com> > > PR c++/49718 > * g++.dg/pr49718.C: New Isn't the idea here that if we've already generated the function body (presumably with instrumentation) that a no-instrument attribute appearing on a later declaration won't do anything useful? jeff
On 01/09/14 06:02, Jeff Law wrote: > On 01/08/14 02:05, Laurent Alfonsi wrote: >> All, >> >> I was looking at PR49718. I have enclosed a simple fix for this bug report. >> >> 2014-01-07 Laurent Alfonsi <laurent.alfonsi@st.com> >> >> * c-family/c-common.c (handle_no_instrument_function_attribute): Allow >> no_instrument_function attribute in class member >> definition/declaration. >> >> >> Looking at the implementation of the function attributes, I see no >> reason anymore to keep this error message. >> Let me know if I missed something. >> I have also added a testcase in the enclosed patch. >> >> 2014-01-07 Laurent Alfonsi <laurent.alfonsi@st.com> >> >> PR c++/49718 >> * g++.dg/pr49718.C: New > Isn't the idea here that if we've already generated the function body > (presumably with instrumentation) that a no-instrument attribute > appearing on a later declaration won't do anything useful? > > jeff > > Jeff, You are right. That's probably the reason. From what i can see, the code instrumentation is performed in the gimplification pass (gimplify_function_tree), and the function attribute is handled and attached earlier in the parsing phase. I ve checked with an example like : ---8<------8<------8<------8<------8<--- int foo () { return 2; } int bar () { return 1; } int foo () __attribute__((no_instrument_function)); ---8<------8<------8<------8<------8<--- The attribute is well honored on foo function. I might need to add this test case too. Let me know if fix is ok. Thanks Laurent
On 01/09/14 07:17, Laurent Alfonsi wrote: > On 01/09/14 06:02, Jeff Law wrote: >> On 01/08/14 02:05, Laurent Alfonsi wrote: >>> All, >>> >>> I was looking at PR49718. I have enclosed a simple fix for this bug >>> report. >>> >>> 2014-01-07 Laurent Alfonsi <laurent.alfonsi@st.com> >>> >>> * c-family/c-common.c >>> (handle_no_instrument_function_attribute): Allow >>> no_instrument_function attribute in class member >>> definition/declaration. >>> >>> >>> Looking at the implementation of the function attributes, I see no >>> reason anymore to keep this error message. >>> Let me know if I missed something. >>> I have also added a testcase in the enclosed patch. >>> >>> 2014-01-07 Laurent Alfonsi <laurent.alfonsi@st.com> >>> >>> PR c++/49718 >>> * g++.dg/pr49718.C: New >> Isn't the idea here that if we've already generated the function body >> (presumably with instrumentation) that a no-instrument attribute >> appearing on a later declaration won't do anything useful? >> >> jeff >> >> > Jeff, > > You are right. That's probably the reason. > From what i can see, the code instrumentation is performed in the > gimplification pass (gimplify_function_tree), and the function attribute > is handled and attached earlier in the parsing phase. > > I ve checked with an example like : > ---8<------8<------8<------8<------8<--- > int foo () { > return 2; > } > > int bar () { > return 1; > } > > int foo () __attribute__((no_instrument_function)); > ---8<------8<------8<------8<------8<--- > The attribute is well honored on foo function. > I might need to add this test case too. Thanks. I checked with Jakub and he confirmed that all the flags to disable unit-at-a-time mode are gone and there's no way to generate gimple for a function prior to parsing the entire TU. That means this patch should be OK. I don't see you in the MAINTAINERS file or with an account on gcc.gnu.org, so I'll check it in for you shortly. Thanks for your patience, Jeff
Perfect. Thanks very much for the commit. Regards, Laurent On 01/15/14 20:25, Jeff Law wrote: > On 01/09/14 07:17, Laurent Alfonsi wrote: >> On 01/09/14 06:02, Jeff Law wrote: >>> On 01/08/14 02:05, Laurent Alfonsi wrote: >>>> All, >>>> >>>> I was looking at PR49718. I have enclosed a simple fix for this bug >>>> report. >>>> >>>> 2014-01-07 Laurent Alfonsi <laurent.alfonsi@st.com> >>>> >>>> * c-family/c-common.c >>>> (handle_no_instrument_function_attribute): Allow >>>> no_instrument_function attribute in class member >>>> definition/declaration. >>>> >>>> >>>> Looking at the implementation of the function attributes, I see no >>>> reason anymore to keep this error message. >>>> Let me know if I missed something. >>>> I have also added a testcase in the enclosed patch. >>>> >>>> 2014-01-07 Laurent Alfonsi <laurent.alfonsi@st.com> >>>> >>>> PR c++/49718 >>>> * g++.dg/pr49718.C: New >>> Isn't the idea here that if we've already generated the function body >>> (presumably with instrumentation) that a no-instrument attribute >>> appearing on a later declaration won't do anything useful? >>> >>> jeff >>> >>> >> Jeff, >> >> You are right. That's probably the reason. >> From what i can see, the code instrumentation is performed in the >> gimplification pass (gimplify_function_tree), and the function attribute >> is handled and attached earlier in the parsing phase. >> >> I ve checked with an example like : >> ---8<------8<------8<------8<------8<--- >> int foo () { >> return 2; >> } >> >> int bar () { >> return 1; >> } >> >> int foo () __attribute__((no_instrument_function)); >> ---8<------8<------8<------8<------8<--- >> The attribute is well honored on foo function. >> I might need to add this test case too. > Thanks. I checked with Jakub and he confirmed that all the flags to > disable unit-at-a-time mode are gone and there's no way to generate > gimple for a function prior to parsing the entire TU. > > That means this patch should be OK. I don't see you in the MAINTAINERS > file or with an account on gcc.gnu.org, so I'll check it in for you shortly. > > Thanks for your patience, > > Jeff > > . >
From 141d2bcfeab5e0635c7f4e362387fd5b1b9494e6 Mon Sep 17 00:00:00 2001 From: Laurent ALFONSI <laurent.alfonsi@st.com> Date: Tue, 7 Jan 2014 16:26:04 +0100 Subject: [PATCH] Fix PR49718 : allow no_instrument_function attribute in class member definition/declaration --- gcc/c-family/c-common.c | 6 ------ gcc/testsuite/g++.dg/pr49718.C | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/g++.dg/pr49718.C diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 8ecb70c..17fcb0d 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -7929,12 +7929,6 @@ handle_no_instrument_function_attribute (tree *node, tree name, "%qE attribute applies only to functions", name); *no_add_attrs = true; } - else if (DECL_INITIAL (decl)) - { - error_at (DECL_SOURCE_LOCATION (decl), - "can%'t set %qE attribute after definition", name); - *no_add_attrs = true; - } else DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (decl) = 1; diff --git a/gcc/testsuite/g++.dg/pr49718.C b/gcc/testsuite/g++.dg/pr49718.C new file mode 100644 index 0000000..07cac8c --- /dev/null +++ b/gcc/testsuite/g++.dg/pr49718.C @@ -0,0 +1,41 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -finstrument-functions" } */ +/* { dg-final { scan-assembler-times "__cyg_profile_func_enter" 1} } */ + +#define NOINSTR __attribute__((no_instrument_function)) + +struct t +{ + public: + /* Function code should be instrumented */ + __attribute__((noinline)) t() {} + + /* Function t::a() should not be instrumented */ + NOINSTR void a(){ + } + /* Function t::b() should not be instrumented */ + void NOINSTR b(){ + } + /* Function t::c() should not be instrumented */ + void c() NOINSTR { + } + /* Function t::d() should not be instrumented */ + void d() NOINSTR; +}; + +void t::d() +{ +} + +/* Function call_all_functions() should not be instrumented */ +struct t call_all_functions() __attribute__((no_instrument_function)); +struct t call_all_functions() +{ + struct t a; /* Constructor not inlined */ + a.a(); /* Inlined t::a() should not be instrumented */ + a.b(); /* Inlined t::b() should not be instrumented */ + a.c(); /* Inlined t::c() should not be instrumented */ + a.d(); /* Inlined t::d() should not be instrumented */ + return a; +} + -- 1.8.4.1
All, I was looking at PR49718. I have enclosed a simple fix for this bug report. 2014-01-07 Laurent Alfonsi <laurent.alfonsi@st.com> * c-family/c-common.c (handle_no_instrument_function_attribute): Allow no_instrument_function attribute in class member definition/declaration. Looking at the implementation of the function attributes, I see no reason anymore to keep this error message. Let me know if I missed something. I have also added a testcase in the enclosed patch. 2014-01-07 Laurent Alfonsi <laurent.alfonsi@st.com> PR c++/49718 * g++.dg/pr49718.C: New gcc/g++/libstdc++ testsuites are ok on x86-64. Ok for trunk ? Regards, Laurent