Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(473)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 5877043: [google] Minor cleanup and test fixes for -mpatch-functions-for-instrumentation.

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 1 month ago by harshit
Modified:
9 years, 7 months ago
Reviewers:
davidxl
CC:
gcc-patches_gcc.gnu.org, Sharad Singhai
Visibility:
Public.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+95 lines, -22 lines) Patch
M gcc/config/i386/i386.c View 2 chunks +2 lines, -1 line 0 comments Download
M gcc/config/i386/i386.md View 2 chunks +2 lines, -2 lines 0 comments Download
M gcc/testsuite/gcc.target/i386/patch-functions-1.c View 2 chunks +6 lines, -3 lines 0 comments Download
M gcc/testsuite/gcc.target/i386/patch-functions-2.c View 2 chunks +6 lines, -3 lines 0 comments Download
M gcc/testsuite/gcc.target/i386/patch-functions-3.c View 2 chunks +6 lines, -3 lines 0 comments Download
M gcc/testsuite/gcc.target/i386/patch-functions-4.c View 2 chunks +6 lines, -3 lines 0 comments Download
M gcc/testsuite/gcc.target/i386/patch-functions-5.c View 2 chunks +6 lines, -3 lines 0 comments Download
M gcc/testsuite/gcc.target/i386/patch-functions-6.c View 2 chunks +3 lines, -2 lines 0 comments Download
M gcc/testsuite/gcc.target/i386/patch-functions-7.c View 2 chunks +3 lines, -2 lines 0 comments Download
A gcc/testsuite/gcc.target/i386/patch-functions-8.c View 1 chunk +29 lines, -0 lines 0 comments Download
A gcc/testsuite/gcc.target/i386/patch-functions-sibling-call.c View 1 chunk +26 lines, -0 lines 0 comments Download

Messages

Total messages: 2
harshit
2012-03-21 Harshit Chopra <harshit@google.com> Minor changes: i386.c: made check_should_patch_current_function C90 compatible. i386.md: Added '\t' to ...
12 years, 1 month ago (2012-03-21 21:45:59 UTC) #1
davidxl
12 years, 1 month ago (2012-03-26 16:50:00 UTC) #2 
Ok for google branches (main and 4_7).

thanks,

David

On Wed, Mar 21, 2012 at 2:45 PM, Harshit Chopra <harshit@google.com> wrote:
> 2012-03-21   Harshit Chopra  <harshit@google.com>
>
>  Minor changes:
>    i386.c: made check_should_patch_current_function C90 compatible.
>    i386.md: Added '\t' to bytes generated by
>             ix86_output_function_nops_prologue_epilogue for proper formatting
>             of assembly.
>    patch-functions-*.c: Fixed verification in tests. Added a test to verify
>                         nop-bytes generated for sibling calls and another test
>                         to verify a binary with nop-bytes runs properly.
>
>        * gcc/config/i386/i386.c (check_should_patch_current_function):
>        * gcc/config/i386/i386.md:
>        * gcc/testsuite/gcc.target/i386/patch-functions-1.c (void foo):
>        (int main):
>        * gcc/testsuite/gcc.target/i386/patch-functions-2.c:
>        * gcc/testsuite/gcc.target/i386/patch-functions-3.c:
>        * gcc/testsuite/gcc.target/i386/patch-functions-4.c:
>        * gcc/testsuite/gcc.target/i386/patch-functions-5.c:
>        * gcc/testsuite/gcc.target/i386/patch-functions-6.c:
>        * gcc/testsuite/gcc.target/i386/patch-functions-7.c:
>        * gcc/testsuite/gcc.target/i386/patch-functions-8.c (int foo):
>        (int bar):
>        * gcc/testsuite/gcc.target/i386/patch-functions-sibling-call.c:
>
> Testing method:
>  make check-gcc RUNTESTFLAGS="i386.exp=patch-functions*
--target_board=\"unix{-m32,}\""
>
> Patch to be applied to google/main.
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 08bd5f0..be1f7a4 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -10981,6 +10981,7 @@ check_should_patch_current_function (void)
>   const char* func_name = NULL;
>   struct loops loops;
>   int num_loops = 0;
> +  int min_functions_instructions;
>
>   /* Patch the function if it has at least a loop.  */
>   if (!patch_functions_ignore_loops)
> @@ -11007,7 +11008,7 @@ check_should_patch_current_function (void)
>       strcmp("main", func_name) == 0)
>     return true;
>
> -  int min_functions_instructions =
> +  min_functions_instructions =
>       PARAM_VALUE (PARAM_FUNCTION_PATCH_MIN_INSTRUCTIONS);
>   if (min_functions_instructions > 0)
>     {
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index 08353ff..38a04ae 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -11688,7 +11688,7 @@
>       /* Emit 10 nop bytes after ret.  */
>       if (ix86_output_function_nops_prologue_epilogue (asm_out_file,
>                                                      
FUNCTION_PATCH_EPILOGUE_SECTION,
> -                                                      "ret",
> +                                                      "\tret",
>                                                       10))
>        return "";
>     }
> @@ -11712,7 +11712,7 @@
>       /* Emit 9 nop bytes after rep;ret.  */
>       if (ix86_output_function_nops_prologue_epilogue (asm_out_file,
>                                                      
FUNCTION_PATCH_EPILOGUE_SECTION,
> -                                                      "rep\;ret",
> +                                                      "\trep\;ret",
>                                                       9))
>        return "";
>     }
> diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-1.c
b/gcc/testsuite/gcc.target/i386/patch-functions-1.c
> index 308e8c3..aa1f424 100644
> --- a/gcc/testsuite/gcc.target/i386/patch-functions-1.c
> +++ b/gcc/testsuite/gcc.target/i386/patch-functions-1.c
> @@ -1,5 +1,5 @@
>  /* Verify -mpatch-functions-for-instrumentation works.  */
> -/* { dg-do run } */
> +/* { dg-do compile } */
>  /* { dg-require-effective-target lp64 } */
>  /* { dg-options "-mpatch-functions-for-instrumentation" } */
>
> @@ -8,13 +8,16 @@
>  /* Check nop-bytes at end.  */
>  /* { dg-final { scan-assembler "ret(.*).byte\t0x90(.*).byte\t0x90" } } */
>
> -void foo() {
> +__attribute__ ((noinline))
> +void foo()
> +{
>   /* Dummy loop.  */
>   int x = 0;
>   while (++x);
>  }
>
> -int main() {
> +int main()
> +{
>   foo();
>   return 0;
>  }
> diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-2.c
b/gcc/testsuite/gcc.target/i386/patch-functions-2.c
> index 6baad32..78de867 100644
> --- a/gcc/testsuite/gcc.target/i386/patch-functions-2.c
> +++ b/gcc/testsuite/gcc.target/i386/patch-functions-2.c
> @@ -1,4 +1,4 @@
> -/* { dg-do run } */
> +/* { dg-do compile } */
>  /* { dg-require-effective-target lp64 } */
>  /* { dg-options "-mpatch-functions-for-instrumentation
-mno-patch-functions-main-always" } */
>
> @@ -8,11 +8,14 @@
>  /* { dg-final { scan-assembler-not ".byte\t0xeb,0x09(.*).byte\t0x90" } } */
>  /* { dg-final { scan-assembler-not "ret(.*).byte\t0x90(.*).byte\t0x90" } } */
>
> -void foo() {
> +__attribute__ ((noinline))
> +void foo()
> +{
>   int x = 0;
>  }
>
> -int main() {
> +int main()
> +{
>   foo();
>   return 0;
>  }
> diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-3.c
b/gcc/testsuite/gcc.target/i386/patch-functions-3.c
> index 49b57a8..9e8eb52 100644
> --- a/gcc/testsuite/gcc.target/i386/patch-functions-3.c
> +++ b/gcc/testsuite/gcc.target/i386/patch-functions-3.c
> @@ -1,4 +1,4 @@
> -/* { dg-do run } */
> +/* { dg-do compile } */
>  /* { dg-require-effective-target lp64 } */
>  /* { dg-options "-mpatch-functions-for-instrumentation --param
function-patch-min-instructions=0" } */
>
> @@ -8,11 +8,14 @@
>  /* { dg-final { scan-assembler ".byte\t0xeb,0x09(.*).byte\t0x90" } } */
>  /* { dg-final { scan-assembler "ret(.*).byte\t0x90(.*).byte\t0x90" } } */
>
> -void foo() {
> +__attribute__ ((noinline))
> +void foo()
> +{
>   int x = 0;
>  }
>
> -int main() {
> +int main()
> +{
>   foo();
>   return 0;
>  }
> diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-4.c
b/gcc/testsuite/gcc.target/i386/patch-functions-4.c
> index 5fcbd0f..7a031d7 100644
> --- a/gcc/testsuite/gcc.target/i386/patch-functions-4.c
> +++ b/gcc/testsuite/gcc.target/i386/patch-functions-4.c
> @@ -1,4 +1,4 @@
> -/* { dg-do run } */
> +/* { dg-do compile } */
>  /* { dg-require-effective-target lp64 } */
>  /* { dg-options "-mpatch-functions-for-instrumentation
-mpatch-functions-ignore-loops -mno-patch-functions-main-always" } */
>
> @@ -8,12 +8,15 @@
>  /* { dg-final { scan-assembler-not ".byte\t0xeb,0x09(.*).byte\t0x90" } } */
>  /* { dg-final { scan-assembler-not "ret(.*).byte\t0x90(.*).byte\t0x90" } } */
>
> -void foo() {
> +__attribute__ ((noinline))
> +void foo()
> +{
>   int x = 0;
>   while (++x);
>  }
>
> -int main() {
> +int main()
> +{
>   foo();
>   return 0;
>  }
> diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-5.c
b/gcc/testsuite/gcc.target/i386/patch-functions-5.c
> index 1f9cccf..cd6a014 100644
> --- a/gcc/testsuite/gcc.target/i386/patch-functions-5.c
> +++ b/gcc/testsuite/gcc.target/i386/patch-functions-5.c
> @@ -1,4 +1,4 @@
> -/* { dg-do run } */
> +/* { dg-do compile } */
>  /* { dg-require-effective-target lp64 } */
>  /* { dg-options "-mpatch-functions-for-instrumentation
-mpatch-functions-ignore-loops --param function-patch-min-instructions=0" } */
>
> @@ -8,12 +8,15 @@
>  /* { dg-final { scan-assembler ".byte\t0xeb,0x09(.*).byte\t0x90" } } */
>  /* { dg-final { scan-assembler "ret(.*).byte\t0x90(.*).byte\t0x90" } } */
>
> -void foo() {
> +__attribute__ ((noinline))
> +void foo()
> +{
>   int x = 0;
>   while (++x);
>  }
>
> -int main() {
> +int main()
> +{
>   foo();
>   return 0;
>  }
> diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-6.c
b/gcc/testsuite/gcc.target/i386/patch-functions-6.c
> index 5bf844f..c1d6446 100644
> --- a/gcc/testsuite/gcc.target/i386/patch-functions-6.c
> +++ b/gcc/testsuite/gcc.target/i386/patch-functions-6.c
> @@ -1,4 +1,4 @@
> -/* { dg-do run } */
> +/* { dg-do compile } */
>  /* { dg-require-effective-target lp64 } */
>  /* { dg-options "-mpatch-functions-for-instrumentation" } */
>
> @@ -8,7 +8,8 @@
>  /* { dg-final { scan-assembler ".byte\t0xeb,0x09(.*).byte\t0x90" } } */
>  /* { dg-final { scan-assembler "ret(.*).byte\t0x90(.*).byte\t0x90" } } */
>
> -int main(int argc, char **argv) {
> +int main()
> +{
>   int x = 0;
>   return 0;
>  }
> diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-7.c
b/gcc/testsuite/gcc.target/i386/patch-functions-7.c
> index e2c50a0..f625298 100644
> --- a/gcc/testsuite/gcc.target/i386/patch-functions-7.c
> +++ b/gcc/testsuite/gcc.target/i386/patch-functions-7.c
> @@ -1,4 +1,4 @@
> -/* { dg-do run } */
> +/* { dg-do compile } */
>  /* { dg-require-effective-target lp64 } */
>  /* { dg-options "-mpatch-functions-for-instrumentation
-mno-patch-functions-main-always" } */
>
> @@ -8,7 +8,8 @@
>  /* { dg-final { scan-assembler-not ".byte\t0xeb,0x09(.*).byte\t0x90" } } */
>  /* { dg-final { scan-assembler-not "ret(.*).byte\t0x90(.*).byte\t0x90" } } */
>
> -int main(int argc, char **argv) {
> +int main()
> +{
>   int x = 0;
>   return 0;
>  }
> diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-8.c
b/gcc/testsuite/gcc.target/i386/patch-functions-8.c
> new file mode 100644
> index 0000000..436379c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/patch-functions-8.c
> @@ -0,0 +1,29 @@
> +/* Verify -mpatch-functions-for-instrumentation works.  */
> +/* { dg-do run } */
> +/* { dg-require-effective-target lp64 } */
> +
> +/* -O2 forces a sibling call for foo from bar.  */
> +/* { dg-options "-O2 -mpatch-functions-for-instrumentation --param
function-patch-min-instructions=0" } */
> +
> +__attribute__ ((noinline))
> +int foo()
> +{
> +  /* Dummy loop.  */
> +  int x = 10;
> +  int y = 100;
> +  while (--x)
> +    ++y;
> +  return y;
> +}
> +
> +__attribute__ ((noinline))
> +int bar()
> +{
> +  return foo();
> +}
> +
> +int main()
> +{
> +  bar();
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-sibling-call.c
b/gcc/testsuite/gcc.target/i386/patch-functions-sibling-call.c
> new file mode 100644
> index 0000000..847a95c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/patch-functions-sibling-call.c
> @@ -0,0 +1,26 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target lp64 } */
> +/* -O2 forces a sibling call.  */
> +/* { dg-options "-O2 -mpatch-functions-for-instrumentation" } */
> +
> +/* { dg-final { scan-assembler ".byte\t0xeb,0x09(.*).byte\t0x90" } } */
> +
> +/* Checks correct nop-bytes are generated just before a sibling call.  */
> +/* { dg-final { scan-assembler ".byte\t0xeb,0x09(.*).byte\t0x90(.*)jmp" } }
*/
> +
> +/* Not instrumented as function has no loop and is small.  */
> +__attribute__ ((noinline))
> +int foo(int n)
> +{
> +  int x = 0;
> +  return n + 10;
> +}
> +
> +__attribute__ ((noinline))
> +int bar(int n)
> +{
> +  /* Dummy loop.  */
> +  while (--n)
> +    n = n * 2;
> +  return foo(n);
> +}
>
> --
> This patch is available for review at http://codereview.appspot.com/5877043
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b