Fix PR78333

Since GCC 4.6 we aggressively prune bodies of GNU extern inline functions
which means that instrumenting them via -finstrument-functions doesn't
work because that takes the address of the function.  Fixed by not
instrumenting those functions (we still instrument regular always-inline
functions and that works as expected).

In the PR this affects intrinsic header functions but it will also
affect fortify wrappers and in both cases instrumenting is undesirable
I think.

Bootstrap / regtest in progress on x86_64-unknown-linux-gnu.

Richard.

2016-11-16  Richard Biener  <rguenther@suse.de>

	PR middle-end/78333
	* gimplify.c (gimplify_function_tree): Do not instrument
	GNU extern inline functions.

	* gcc.dg/pr78333.c: New testcase.

On 16 November 2016 at 10:35, Richard Biener <rguenther@suse.de> wrote:
>
> Since GCC 4.6 we aggressively prune bodies of GNU extern inline functions
> which means that instrumenting them via -finstrument-functions doesn't
> work because that takes the address of the function.  Fixed by not
> instrumenting those functions (we still instrument regular always-inline
> functions and that works as expected).
>
> In the PR this affects intrinsic header functions but it will also
> affect fortify wrappers and in both cases instrumenting is undesirable
> I think.
>
> Bootstrap / regtest in progress on x86_64-unknown-linux-gnu.
>
> Richard.
>
> 2016-11-16  Richard Biener  <rguenther@suse.de>
>
>         PR middle-end/78333
>         * gimplify.c (gimplify_function_tree): Do not instrument
>         GNU extern inline functions.
>
>         * gcc.dg/pr78333.c: New testcase.
>
> Index: gcc/gimplify.c
> ===================================================================
> --- gcc/gimplify.c      (revision 242408)
> +++ gcc/gimplify.c      (working copy)
> @@ -12547,6 +12559,10 @@ gimplify_function_tree (tree fndecl)
>    /* ??? Add some way to ignore exceptions for this TFE.  */
>    if (flag_instrument_function_entry_exit
>        && !DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (fndecl)
> +      /* Do not instrument extern inline functions.  */
> +      && !(DECL_DECLARED_INLINE_P (fndecl)
> +          && DECL_EXTERNAL (fndecl)
> +          && DECL_DISREGARD_INLINE_LIMITS (fndecl))
>        && !flag_instrument_functions_exclude_p (fndecl))
>      {
>        tree x;
> Index: gcc/testsuite/gcc.dg/pr78333.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/pr78333.c      (revision 0)
> +++ gcc/testsuite/gcc.dg/pr78333.c      (working copy)
> @@ -0,0 +1,9 @@
> +/* { dg-do link } */
> +/* { dg-options "-finstrument-functions" } */
> +
> +extern inline __attribute__((gnu_inline, always_inline)) int foo () { }
> +int main()
> +{
> +  foo ();
> +  return 0;
> +}

Hi Richard,

The new testcase fails to link on bare-metal targets (arm/aarch64):
/cc4ckSGA.o: In function `main':
pr78333.c:(.text+0x1c): undefined reference to `__cyg_profile_func_enter'
pr78333.c:(.text+0x34): undefined reference to `__cyg_profile_func_exit'
collect2: error: ld returned 1 exit status

I understand that we really want this test to have dg-do link unlike most
of the other tests using -finstrument-functions, so we do not want to
switch to dg-do compile.

Do we want to skip this test in bare-metal configs, or is it a new bug?

Thanks,

Christophe

Hi Christophe,

> The new testcase fails to link on bare-metal targets (arm/aarch64):
> /cc4ckSGA.o: In function `main':
> pr78333.c:(.text+0x1c): undefined reference to `__cyg_profile_func_enter'
> pr78333.c:(.text+0x34): undefined reference to `__cyg_profile_func_exit'
> collect2: error: ld returned 1 exit status
>
> I understand that we really want this test to have dg-do link unlike most
> of the other tests using -finstrument-functions, so we do not want to
> switch to dg-do compile.
>
> Do we want to skip this test in bare-metal configs, or is it a new bug?

it's not only bare-metal targets, but probably most non-glibc targets.
E.g the test also FAILs on Solaris in the same way, unlike Linux/x86_64
where glibc provides those functions.

Probably providing dummy implemenations as in the original testcase in
the PR is enough?

	Rainer

On Thu, 17 Nov 2016, Christophe Lyon wrote:

> On 16 November 2016 at 10:35, Richard Biener <rguenther@suse.de> wrote:
> >
> > Since GCC 4.6 we aggressively prune bodies of GNU extern inline functions
> > which means that instrumenting them via -finstrument-functions doesn't
> > work because that takes the address of the function.  Fixed by not
> > instrumenting those functions (we still instrument regular always-inline
> > functions and that works as expected).
> >
> > In the PR this affects intrinsic header functions but it will also
> > affect fortify wrappers and in both cases instrumenting is undesirable
> > I think.
> >
> > Bootstrap / regtest in progress on x86_64-unknown-linux-gnu.
> >
> > Richard.
> >
> > 2016-11-16  Richard Biener  <rguenther@suse.de>
> >
> >         PR middle-end/78333
> >         * gimplify.c (gimplify_function_tree): Do not instrument
> >         GNU extern inline functions.
> >
> >         * gcc.dg/pr78333.c: New testcase.
> >
> > Index: gcc/gimplify.c
> > ===================================================================
> > --- gcc/gimplify.c      (revision 242408)
> > +++ gcc/gimplify.c      (working copy)
> > @@ -12547,6 +12559,10 @@ gimplify_function_tree (tree fndecl)
> >    /* ??? Add some way to ignore exceptions for this TFE.  */
> >    if (flag_instrument_function_entry_exit
> >        && !DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (fndecl)
> > +      /* Do not instrument extern inline functions.  */
> > +      && !(DECL_DECLARED_INLINE_P (fndecl)
> > +          && DECL_EXTERNAL (fndecl)
> > +          && DECL_DISREGARD_INLINE_LIMITS (fndecl))
> >        && !flag_instrument_functions_exclude_p (fndecl))
> >      {
> >        tree x;
> > Index: gcc/testsuite/gcc.dg/pr78333.c
> > ===================================================================
> > --- gcc/testsuite/gcc.dg/pr78333.c      (revision 0)
> > +++ gcc/testsuite/gcc.dg/pr78333.c      (working copy)
> > @@ -0,0 +1,9 @@
> > +/* { dg-do link } */
> > +/* { dg-options "-finstrument-functions" } */
> > +
> > +extern inline __attribute__((gnu_inline, always_inline)) int foo () { }
> > +int main()
> > +{
> > +  foo ();
> > +  return 0;
> > +}
> 
> Hi Richard,
> 
> The new testcase fails to link on bare-metal targets (arm/aarch64):
> /cc4ckSGA.o: In function `main':
> pr78333.c:(.text+0x1c): undefined reference to `__cyg_profile_func_enter'
> pr78333.c:(.text+0x34): undefined reference to `__cyg_profile_func_exit'
> collect2: error: ld returned 1 exit status
> 
> I understand that we really want this test to have dg-do link unlike most
> of the other tests using -finstrument-functions, so we do not want to
> switch to dg-do compile.

Yes, we need a link test (maybe a very carefully crafted scan-assembler
test would also work...)

> Do we want to skip this test in bare-metal configs, or is it a new bug?

Is there some dg- target for "bare metal"?  Can we use ! freestanding?
 
Otherwise does abusing sth like { dg-require-profiling 
"-finstrument-functions" } work?  (seems to just test if -pg works)

Thanks,
Richard.

On Thu, 17 Nov 2016, Rainer Orth wrote:

> Hi Christophe,
> 
> > The new testcase fails to link on bare-metal targets (arm/aarch64):
> > /cc4ckSGA.o: In function `main':
> > pr78333.c:(.text+0x1c): undefined reference to `__cyg_profile_func_enter'
> > pr78333.c:(.text+0x34): undefined reference to `__cyg_profile_func_exit'
> > collect2: error: ld returned 1 exit status
> >
> > I understand that we really want this test to have dg-do link unlike most
> > of the other tests using -finstrument-functions, so we do not want to
> > switch to dg-do compile.
> >
> > Do we want to skip this test in bare-metal configs, or is it a new bug?
> 
> it's not only bare-metal targets, but probably most non-glibc targets.
> E.g the test also FAILs on Solaris in the same way, unlike Linux/x86_64
> where glibc provides those functions.
> 
> Probably providing dummy implemenations as in the original testcase in
> the PR is enough?

Maybe { dg-require weak } plus weak definitions of the cyg_profile funcs?
Or simply restrict the test to { target *-*-linux* }?

Richard.

Hi Richard,

>> Probably providing dummy implemenations as in the original testcase in
>> the PR is enough?
>
> Maybe { dg-require weak } plus weak definitions of the cyg_profile funcs?
> Or simply restrict the test to { target *-*-linux* }?

the only existing dg-do run testcase (gcc.dg/20001117-1.c) just has

void __attribute__((no_instrument_function))
__cyg_profile_func_enter(void *this_fn, void *call_site)
{
  if (call_site == (void *)0)
    abort ();
}

void __attribute__((no_instrument_function))
__cyg_profile_func_exit(void *this_fn, void *call_site)
{
  if (call_site == (void *)0)
    abort ();
}

In the case at hand, we could do with empty implementations.  This
certainly works on Solaris.

	Rainer

On Thu, 17 Nov 2016, Rainer Orth wrote:

> Hi Richard,
> 
> >> Probably providing dummy implemenations as in the original testcase in
> >> the PR is enough?
> >
> > Maybe { dg-require weak } plus weak definitions of the cyg_profile funcs?
> > Or simply restrict the test to { target *-*-linux* }?
> 
> the only existing dg-do run testcase (gcc.dg/20001117-1.c) just has
> 
> void __attribute__((no_instrument_function))
> __cyg_profile_func_enter(void *this_fn, void *call_site)
> {
>   if (call_site == (void *)0)
>     abort ();
> }
> 
> void __attribute__((no_instrument_function))
> __cyg_profile_func_exit(void *this_fn, void *call_site)
> {
>   if (call_site == (void *)0)
>     abort ();
> }
> 
> In the case at hand, we could do with empty implementations.  This
> certainly works on Solaris.

Ok.  Christophe, can you add the above and verify it works for you
(and then commit)?

Thanks,
Richard.

On 17 November 2016 at 10:53, Richard Biener <rguenther@suse.de> wrote:
> On Thu, 17 Nov 2016, Rainer Orth wrote:
>
>> Hi Richard,
>>
>> >> Probably providing dummy implemenations as in the original testcase in
>> >> the PR is enough?
>> >
>> > Maybe { dg-require weak } plus weak definitions of the cyg_profile funcs?
>> > Or simply restrict the test to { target *-*-linux* }?
>>
>> the only existing dg-do run testcase (gcc.dg/20001117-1.c) just has
>>
>> void __attribute__((no_instrument_function))
>> __cyg_profile_func_enter(void *this_fn, void *call_site)
>> {
>>   if (call_site == (void *)0)
>>     abort ();
>> }
>>
>> void __attribute__((no_instrument_function))
>> __cyg_profile_func_exit(void *this_fn, void *call_site)
>> {
>>   if (call_site == (void *)0)
>>     abort ();
>> }
>>
>> In the case at hand, we could do with empty implementations.  This
>> certainly works on Solaris.
>
> Ok.  Christophe, can you add the above and verify it works for you
> (and then commit)?
>
OK, I'm taking a look.

> Thanks,
> Richard.