diff mbox

Add -fno-instrument-function

Message ID 1409603138-18755-1-git-send-email-andi@firstfloor.org
State New
Headers show

Commit Message

Andi Kleen Sept. 1, 2014, 8:25 p.m. UTC 
From: Andi Kleen <ak@linux.intel.com>

[This was an old patch of mine that has been posted before,
but never made it in]

This adds a new C/C++ option to force
__attribute__((no_instrument_function)) on every function compiled.

This is useful together with LTO. You may want to have the whole
program compiled with -pg and have to specify that in the LTO
link, but want to disable it for some specific files. As the
option works on the frontend level it is already passed through
properly by LTO.

Without LTO it is equivalent to not specifing -pg or -mfentry.

This fixes some missing functionality in the Linux kernel LTO port,
in particular it allows using the function tracer with LTO kernels.

Longer term it would be nicer if all suitable options were handled
like this for LTO by turning them into attributes, but that would
be a much larger project.

Passed bootstrap and test suite on x86_64-linux. Ok?

gcc/:

2014-09-01  Andi Kleen <ak@linux.intel.com>

	* c.opt (fno-instrument-function): Document.

gcc/c:

2014-09-01  Andi Kleen <ak@linux.intel.com>

	* c-decl.c (start_function): Handle force_no_instrument_function

gcc/cp:

2014-09-01  Andi Kleen <ak@linux.intel.com>

	* decl.c (start_preparsed_function): Handle
  	force_no_instrument_function

gcc/testsuite:

2014-09-01  Andi Kleen <ak@linux.intel.com>

	* g++.dg/fno-instrument-function.C: Add.
	* gcc.dg/fno-instrument-function.c: Add.
---
 gcc/c-family/c.opt                             |  4 ++++
 gcc/c/c-decl.c                                 |  3 +++
 gcc/cp/decl.c                                  |  3 +++
 gcc/doc/invoke.texi                            |  8 +++++++-
 gcc/testsuite/g++.dg/fno-instrument-function.C | 18 ++++++++++++++++++
 gcc/testsuite/gcc.dg/fno-instrument-function.c | 24 ++++++++++++++++++++++++
 6 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/fno-instrument-function.C
 create mode 100644 gcc/testsuite/gcc.dg/fno-instrument-function.c

Comments

Richard Biener Sept. 2, 2014, 9:27 a.m. UTC | #1 
On Mon, Sep 1, 2014 at 10:25 PM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> [This was an old patch of mine that has been posted before,
> but never made it in]
>
> This adds a new C/C++ option to force
> __attribute__((no_instrument_function)) on every function compiled.
>
> This is useful together with LTO. You may want to have the whole
> program compiled with -pg and have to specify that in the LTO
> link, but want to disable it for some specific files. As the
> option works on the frontend level it is already passed through
> properly by LTO.
>
> Without LTO it is equivalent to not specifing -pg or -mfentry.
>
> This fixes some missing functionality in the Linux kernel LTO port,
> in particular it allows using the function tracer with LTO kernels.
>
> Longer term it would be nicer if all suitable options were handled
> like this for LTO by turning them into attributes, but that would
> be a much larger project.
>
> Passed bootstrap and test suite on x86_64-linux. Ok?

Hmm, why not make -no-pg (does that exist?) and/or -mno-fentry
do this?  That is, I don't see the need for a new option.

Or do it the other way around - change the default to
DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT and make
-pg/-mfentry unset that (or have DECL_INSTRUMENT_FUNCTION_ENTRY_EXIT).

Richard.

> gcc/:
>
> 2014-09-01  Andi Kleen <ak@linux.intel.com>
>
>         * c.opt (fno-instrument-function): Document.
>
> gcc/c:
>
> 2014-09-01  Andi Kleen <ak@linux.intel.com>
>
>         * c-decl.c (start_function): Handle force_no_instrument_function
>
> gcc/cp:
>
> 2014-09-01  Andi Kleen <ak@linux.intel.com>
>
>         * decl.c (start_preparsed_function): Handle
>         force_no_instrument_function
>
> gcc/testsuite:
>
> 2014-09-01  Andi Kleen <ak@linux.intel.com>
>
>         * g++.dg/fno-instrument-function.C: Add.
>         * gcc.dg/fno-instrument-function.c: Add.
> ---
>  gcc/c-family/c.opt                             |  4 ++++
>  gcc/c/c-decl.c                                 |  3 +++
>  gcc/cp/decl.c                                  |  3 +++
>  gcc/doc/invoke.texi                            |  8 +++++++-
>  gcc/testsuite/g++.dg/fno-instrument-function.C | 18 ++++++++++++++++++
>  gcc/testsuite/gcc.dg/fno-instrument-function.c | 24 ++++++++++++++++++++++++
>  6 files changed, 59 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/fno-instrument-function.C
>  create mode 100644 gcc/testsuite/gcc.dg/fno-instrument-function.c
>
> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> index 210a099..2aabd23 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -1118,6 +1118,10 @@ Enum(ivar_visibility) String(public) Value(IVAR_VISIBILITY_PUBLIC)
>  EnumValue
>  Enum(ivar_visibility) String(package) Value(IVAR_VISIBILITY_PACKAGE)
>
> +fno-instrument-function
> +C C++ ObjC ObjC++ RejectNegative Report Var(force_no_instrument_function)
> +Force __attribute__((no_instrument_function)) for all functions in translation unit.
> +
>  fnonansi-builtins
>  C++ ObjC++ Var(flag_no_nonansi_builtin, 0)
>
> diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
> index b4995a6..493240f 100644
> --- a/gcc/c/c-decl.c
> +++ b/gcc/c/c-decl.c
> @@ -8044,6 +8044,9 @@ start_function (struct c_declspecs *declspecs, struct c_declarator *declarator,
>    if (current_scope == file_scope)
>      maybe_apply_pragma_weak (decl1);
>
> +  if (force_no_instrument_function)
> +    DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (decl1) = 1;
> +
>    /* Warn for unlikely, improbable, or stupid declarations of `main'.  */
>    if (warn_main && MAIN_NAME_P (DECL_NAME (decl1)))
>      {
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index d03f8a4..505ad50 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -13251,6 +13251,9 @@ start_preparsed_function (tree decl1, tree attrs, int flags)
>        && lookup_attribute ("noinline", attrs))
>      warning (0, "inline function %q+D given attribute noinline", decl1);
>
> +  if (force_no_instrument_function)
> +    DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (decl1) = 1;
> +
>    /* Handle gnu_inline attribute.  */
>    if (GNU_INLINE_P (decl1))
>      {
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index d15d4a9..51b8d20 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -169,7 +169,7 @@ in the following sections.
>  -aux-info @var{filename} -fallow-parameterless-variadic-functions @gol
>  -fno-asm  -fno-builtin  -fno-builtin-@var{function} @gol
>  -fhosted  -ffreestanding -fopenmp -fopenmp-simd -fms-extensions @gol
> --fplan9-extensions -trigraphs  -traditional  -traditional-cpp @gol
> +-fplan9-extensions -trigraphs  -traditional  -traditional-cpp -fno-instrument-function @gol
>  -fallow-single-precision  -fcond-mismatch -flax-vector-conversions @gol
>  -fsigned-bitfields  -fsigned-char @gol
>  -funsigned-bitfields  -funsigned-char}
> @@ -1971,6 +1971,12 @@ Allow implicit conversions between vectors with differing numbers of
>  elements and/or incompatible element types.  This option should not be
>  used for new code.
>
> +@item -fno-instrument-function
> +@opindex fno-instrument-function
> +Override @option{-pg} for this translation unit. This is useful with
> +Link Time Optimization (LTO) to override the effects of -pg for a
> +specific source file.
> +
>  @item -funsigned-char
>  @opindex funsigned-char
>  Let the type @code{char} be unsigned, like @code{unsigned char}.
> diff --git a/gcc/testsuite/g++.dg/fno-instrument-function.C b/gcc/testsuite/g++.dg/fno-instrument-function.C
> new file mode 100644
> index 0000000..e2f6518
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/fno-instrument-function.C
> @@ -0,0 +1,18 @@
> +/* Test -fno-instrument-function */
> +/* { dg-do compile } */
> +/* { dg-options "-pg -fno-instrument-function" } */
> +/* { dg-final { scan-assembler-not "mcount" } } */
> +/* Origin: Andi Kleen */
> +extern void foobar(const char *);
> +
> +void func(void)
> +{
> +  foobar ("Hello world\n");
> +}
> +
> +void func2(void)
> +{
> +  int i;
> +  for (i = 0; i < 10; i++)
> +    foobar ("Hello world");
> +}
> diff --git a/gcc/testsuite/gcc.dg/fno-instrument-function.c b/gcc/testsuite/gcc.dg/fno-instrument-function.c
> new file mode 100644
> index 0000000..9c68fa8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fno-instrument-function.c
> @@ -0,0 +1,24 @@
> +/* Test -fno-instrument-function */
> +/* { dg-do compile } */
> +/* { dg-options "-pg -fno-instrument-function" } */
> +/* { dg-final { scan-assembler-not "mcount" } } */
> +/* Origin: Andi Kleen */
> +extern void foobar(char *);
> +
> +void func(void)
> +{
> +  foobar ("Hello world\n");
> +}
> +
> +void func2(void)
> +{
> +  int i;
> +  for (i = 0; i < 10; i++)
> +    foobar ("Hello world");
> +}
> +
> +void func3(a)
> +char *a;
> +{
> +  foobar("Hello world");
> +}
> --
> 2.1.0
>
Andi Kleen Sept. 2, 2014, 3 p.m. UTC | #2 
> Hmm, why not make -no-pg (does that exist?) and/or -mno-fentry

I'm not sure.

> do this?  That is, I don't see the need for a new option.

That would be really odd behavior. An yes/no option whose default
is controlled by other object files' command line.
And -pg would be for all files in LTO, and no-pg only for that file,
so not be symmetric.

I think an explicit different option has far cleaner semantics for
now (at least until the LTO option mess can be properly cleaned up)

-Andi
Richard Biener Sept. 3, 2014, 11:03 a.m. UTC | #3 
On Tue, Sep 2, 2014 at 5:00 PM, Andi Kleen <ak@linux.intel.com> wrote:
>> Hmm, why not make -no-pg (does that exist?) and/or -mno-fentry
>
> I'm not sure.
>
>> do this?  That is, I don't see the need for a new option.
>
> That would be really odd behavior. An yes/no option whose default
> is controlled by other object files' command line.
> And -pg would be for all files in LTO, and no-pg only for that file,
> so not be symmetric.
>
> I think an explicit different option has far cleaner semantics for
> now (at least until the LTO option mess can be properly cleaned up)

No, not a new "fake" option either but just initialize
DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT properly when
not doing -pg or -mfentry (that is, set it to 1).

Richard.

> -Andi
Andi Kleen Sept. 4, 2014, 6:04 a.m. UTC | #4 
> No, not a new "fake" option either but just initialize
> DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT properly when
> not doing -pg or -mfentry (that is, set it to 1).

The only way would be to handle it in build_decl itself.
There seem to be a bazillion callers all over the tree that all do their
custom thing.

It would also need a new target hook to check for fentry.

Far more complicated patch.

-Andi
Richard Biener Sept. 4, 2014, 1:11 p.m. UTC | #5 
On Thu, Sep 4, 2014 at 8:04 AM, Andi Kleen <andi@firstfloor.org> wrote:
>> No, not a new "fake" option either but just initialize
>> DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT properly when
>> not doing -pg or -mfentry (that is, set it to 1).
>
> The only way would be to handle it in build_decl itself.
> There seem to be a bazillion callers all over the tree that all do their
> custom thing.
>
> It would also need a new target hook to check for fentry.
>
> Far more complicated patch.

Well, then make it the opposite as I suggested - mark functions
to be instrumented - DECL_INSTRUMENT_FUNCTION_ENTRY_EXIT
and do that in the instrumentation code or the target hook that
gets called when a struct function is initialized.

Or as all this is only a hack for LTO, "set"
DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT in
tree-streamer-out.c:pack_ts_function_decl_value_fields.

Yes, you need a target hook for -mfentry, but that's way better
than a user-visible option for such kind of "hack".

Richard.

> -Andi
>
>
> --
> ak@linux.intel.com -- Speaking for myself only.
diff mbox

Patch

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 210a099..2aabd23 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1118,6 +1118,10 @@  Enum(ivar_visibility) String(public) Value(IVAR_VISIBILITY_PUBLIC)
 EnumValue
 Enum(ivar_visibility) String(package) Value(IVAR_VISIBILITY_PACKAGE)
 
+fno-instrument-function
+C C++ ObjC ObjC++ RejectNegative Report Var(force_no_instrument_function)
+Force __attribute__((no_instrument_function)) for all functions in translation unit.
+
 fnonansi-builtins
 C++ ObjC++ Var(flag_no_nonansi_builtin, 0)
 
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index b4995a6..493240f 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -8044,6 +8044,9 @@  start_function (struct c_declspecs *declspecs, struct c_declarator *declarator,
   if (current_scope == file_scope)
     maybe_apply_pragma_weak (decl1);
 
+  if (force_no_instrument_function)
+    DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (decl1) = 1;
+
   /* Warn for unlikely, improbable, or stupid declarations of `main'.  */
   if (warn_main && MAIN_NAME_P (DECL_NAME (decl1)))
     {
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index d03f8a4..505ad50 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -13251,6 +13251,9 @@  start_preparsed_function (tree decl1, tree attrs, int flags)
       && lookup_attribute ("noinline", attrs))
     warning (0, "inline function %q+D given attribute noinline", decl1);
 
+  if (force_no_instrument_function)
+    DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (decl1) = 1;
+
   /* Handle gnu_inline attribute.  */
   if (GNU_INLINE_P (decl1))
     {
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index d15d4a9..51b8d20 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -169,7 +169,7 @@  in the following sections.
 -aux-info @var{filename} -fallow-parameterless-variadic-functions @gol
 -fno-asm  -fno-builtin  -fno-builtin-@var{function} @gol
 -fhosted  -ffreestanding -fopenmp -fopenmp-simd -fms-extensions @gol
--fplan9-extensions -trigraphs  -traditional  -traditional-cpp @gol
+-fplan9-extensions -trigraphs  -traditional  -traditional-cpp -fno-instrument-function @gol
 -fallow-single-precision  -fcond-mismatch -flax-vector-conversions @gol
 -fsigned-bitfields  -fsigned-char @gol
 -funsigned-bitfields  -funsigned-char}
@@ -1971,6 +1971,12 @@  Allow implicit conversions between vectors with differing numbers of
 elements and/or incompatible element types.  This option should not be
 used for new code.
 
+@item -fno-instrument-function
+@opindex fno-instrument-function
+Override @option{-pg} for this translation unit. This is useful with
+Link Time Optimization (LTO) to override the effects of -pg for a
+specific source file.
+
 @item -funsigned-char
 @opindex funsigned-char
 Let the type @code{char} be unsigned, like @code{unsigned char}.
diff --git a/gcc/testsuite/g++.dg/fno-instrument-function.C b/gcc/testsuite/g++.dg/fno-instrument-function.C
new file mode 100644
index 0000000..e2f6518
--- /dev/null
+++ b/gcc/testsuite/g++.dg/fno-instrument-function.C
@@ -0,0 +1,18 @@ 
+/* Test -fno-instrument-function */
+/* { dg-do compile } */
+/* { dg-options "-pg -fno-instrument-function" } */
+/* { dg-final { scan-assembler-not "mcount" } } */
+/* Origin: Andi Kleen */
+extern void foobar(const char *);
+
+void func(void)
+{
+  foobar ("Hello world\n");
+}
+
+void func2(void)
+{
+  int i;
+  for (i = 0; i < 10; i++)
+    foobar ("Hello world");
+}
diff --git a/gcc/testsuite/gcc.dg/fno-instrument-function.c b/gcc/testsuite/gcc.dg/fno-instrument-function.c
new file mode 100644
index 0000000..9c68fa8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fno-instrument-function.c
@@ -0,0 +1,24 @@ 
+/* Test -fno-instrument-function */
+/* { dg-do compile } */
+/* { dg-options "-pg -fno-instrument-function" } */
+/* { dg-final { scan-assembler-not "mcount" } } */
+/* Origin: Andi Kleen */
+extern void foobar(char *);
+
+void func(void)
+{
+  foobar ("Hello world\n");
+}
+
+void func2(void)
+{
+  int i;
+  for (i = 0; i < 10; i++)
+    foobar ("Hello world");
+}
+
+void func3(a)
+char *a;
+{
+  foobar("Hello world");
+}