[PATCH -tip] ftrace: Correctly calculate the first function in the .text section

[PATCH -tip] ftrace: Correctly calculate the first function in the .text section

[Matt Fleming]

This patch fixes a bug whereby recordmcount.pl didn't stop searching
once it had correctly detected the function at the beginning of the
.text section. To stop it searching, I needed to reset $read_function.
The effect of this bug was that some entries in __mcount_loc section
were created with the negative reloc addends. The last text section
found was used as the base and all mcount calls were at a relative
offset to that, so at final link time the addresses were fixed up to
point to somewhere completely bogus.

This all resulted in ftrace dynamically modifying addresses that weren't
actually mcount callsites.

I also noticed another bug and fixed up the condition from,

	if (!defined($ref_func) || !defined($weak{$text})) {
to
	if (!defined($ref_func) && !defined($weak{$text})) {

This now matches the comment above the conditional.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 scripts/recordmcount.pl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 7109e2b..356ff6a 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -414,8 +414,9 @@ while (<IN>) {
 	    $read_function = 0;
 	} else {
 	    # if we already have a function, and this is weak, skip it
-	    if (!defined($ref_func) || !defined($weak{$text})) {
+	    if (!defined($ref_func) && !defined($weak{$text})) {
 		$ref_func = $text;
+		$read_function = 0;
 	    }
 	}
     } elsif ($read_headers && /$mcount_section/) {
-- 
1.6.4.rc0

Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section

[Steven Rostedt]

On Thu, 23 Jul 2009, Matt Fleming wrote:

> This patch fixes a bug whereby recordmcount.pl didn't stop searching
> once it had correctly detected the function at the beginning of the
> .text section. To stop it searching, I needed to reset $read_function.
> The effect of this bug was that some entries in __mcount_loc section
> were created with the negative reloc addends. The last text section
> found was used as the base and all mcount calls were at a relative
> offset to that, so at final link time the addresses were fixed up to
> point to somewhere completely bogus.
> 
> This all resulted in ftrace dynamically modifying addresses that weren't
> actually mcount callsites.
> 
> I also noticed another bug and fixed up the condition from,

Famous quote: "When the comment does not match the code, they
are most likely both wrong".


> 
> 	if (!defined($ref_func) || !defined($weak{$text})) {
> to
> 	if (!defined($ref_func) && !defined($weak{$text})) {
> 
> This now matches the comment above the conditional.

This is the true bug. But the previous is not.

> 
> Signed-off-by: Matt Fleming <matt@console-pimps.org>
> ---
>  scripts/recordmcount.pl |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> index 7109e2b..356ff6a 100755
> --- a/scripts/recordmcount.pl
> +++ b/scripts/recordmcount.pl
> @@ -414,8 +414,9 @@ while (<IN>) {
>  	    $read_function = 0;
>  	} else {
>  	    # if we already have a function, and this is weak, skip it
> -	    if (!defined($ref_func) || !defined($weak{$text})) {
> +	    if (!defined($ref_func) && !defined($weak{$text})) {

Ack on the above change.

>  		$ref_func = $text;
> +		$read_function = 0;

Nope, this is wrong. The idea is that we are looking for a function that 
is not local. Here's the full condition (with the applied fix):

        if (!defined($locals{$text}) && !defined($weak{$text})) {
            $ref_func = $text;
            $read_function = 0;
        } else {
            # if we already have a function, and this is weak, skip it
            if (!defined($ref_func) && !defined($weak{$text})) {
                $ref_func = $text;
            }
        }

Ideally, we want a non local function to use. If the first function in the 
section is local (defined as static) we record it, but we continue to look 
for more functions (don't set $read_function to 0). The first function in 
the section can easily be a static function, and there my be a global one 
later. If we find a global one later, we rather use that one. By setting 
$read_function to zero here, we stop our search, and we will be using a 
local static function instead.

When ever we use a static function as are reference point, we need to go 
through the pain of converting it to a global function to link in our 
mcount section, and then converting it back to local after it is linked. 

Offsets should be fine when negative, what issues do you see? I will admit 
though, that the s/||/&&/ was a real bug.

Thanks,

-- Steve

>  	    }
>  	}
>      } elsif ($read_headers && /$mcount_section/) {
> -- 
> 1.6.4.rc0
> 
> 

Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section

[Steven Rostedt]

On Thu, 23 Jul 2009, Steven Rostedt wrote:
> 
> > 
> > 	if (!defined($ref_func) || !defined($weak{$text})) {
> > to
> > 	if (!defined($ref_func) && !defined($weak{$text})) {
> > 
> > This now matches the comment above the conditional.
> 
> This is the true bug. But the previous is not.

Could you resend the patch with this fix only. Then I can send it out as 
an ugent fix.

Thanks!

-- Steve

[PATCH 1/2] ftrace: Fix the conditional that updates $ref_func

[Matt Fleming]

Fix the conditional that checks if we already have a $ref_func and that
the new function is weak. The code as previously checking whether either
condition was false, and we really need to only update $ref_func is both
cconditions are false.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 scripts/recordmcount.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 7109e2b..16c5563 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -414,7 +414,7 @@ while (<IN>) {
 	    $read_function = 0;
 	} else {
 	    # if we already have a function, and this is weak, skip it
-	    if (!defined($ref_func) || !defined($weak{$text})) {
+	    if (!defined($ref_func) && !defined($weak{$text})) {
 		$ref_func = $text;
 	    }
 	}
-- 
1.6.4.rc0

[PATCH 2/2] ftrace: Only update $offset when we update $ref_func

[Matt Fleming]

The value of $offset should be the offset of $ref_func from the
beginning of the object file. Therefore, we should set both variables
together.

This fixes a bug I was hitting on sh where $offset (which is used to
calcualte the addends for the __mcount_loc entries) was being set
multiple times and didn't correspond to $ref_func's offset in the object
file. The addends in __mcount_loc were calculated incorrectly, resulting
in ftrace dynamically modifying addresses that weren't mcount call
sites.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 scripts/recordmcount.pl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 16c5563..d29baa2 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -403,7 +403,6 @@ while (<IN>) {
     # section found, now is this a start of a function?
     } elsif ($read_function && /$function_regex/) {
 	$text_found = 1;
-	$offset = hex $1;
 	$text = $2;
 
 	# if this is either a local function or a weak function
@@ -412,10 +411,12 @@ while (<IN>) {
 	if (!defined($locals{$text}) && !defined($weak{$text})) {
 	    $ref_func = $text;
 	    $read_function = 0;
+	    $offset = hex $1;
 	} else {
 	    # if we already have a function, and this is weak, skip it
 	    if (!defined($ref_func) && !defined($weak{$text})) {
 		$ref_func = $text;
+		$offset = hex $1;
 	    }
 	}
     } elsif ($read_headers && /$mcount_section/) {
-- 
1.6.4.rc0

Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section

[Paul Mundt]

On Thu, Jul 23, 2009 at 10:52:47AM -0400, Steven Rostedt wrote:
> 
> On Thu, 23 Jul 2009, Steven Rostedt wrote:
> > 
> > > 
> > > 	if (!defined($ref_func) || !defined($weak{$text})) {
> > > to
> > > 	if (!defined($ref_func) && !defined($weak{$text})) {
> > > 
> > > This now matches the comment above the conditional.
> > 
> > This is the true bug. But the previous is not.
> 
> Could you resend the patch with this fix only. Then I can send it out as 
> an ugent fix.
> 
Is anything happening with these patches? Without them, a lot of SH
configurations are completely broken.

Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section

[Steven Rostedt]

On Tue, 28 Jul 2009, Paul Mundt wrote:

> On Thu, Jul 23, 2009 at 10:52:47AM -0400, Steven Rostedt wrote:
> > 
> > On Thu, 23 Jul 2009, Steven Rostedt wrote:
> > > 
> > > > 
> > > > 	if (!defined($ref_func) || !defined($weak{$text})) {
> > > > to
> > > > 	if (!defined($ref_func) && !defined($weak{$text})) {
> > > > 
> > > > This now matches the comment above the conditional.
> > > 
> > > This is the true bug. But the previous is not.
> > 
> > Could you resend the patch with this fix only. Then I can send it out as 
> > an ugent fix.
> > 
> Is anything happening with these patches? Without them, a lot of SH
> configurations are completely broken.

Thomas,

Could you push these off to Linus? My git pull request had:

Message-ID: <20090723210047.294710482@goodmis.org>

Thanks,

-- Steve

Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section

[Paul Mundt]

On Mon, Jul 27, 2009 at 07:45:46PM -0400, Steven Rostedt wrote:
> 
> On Tue, 28 Jul 2009, Paul Mundt wrote:
> 
> > On Thu, Jul 23, 2009 at 10:52:47AM -0400, Steven Rostedt wrote:
> > > 
> > > On Thu, 23 Jul 2009, Steven Rostedt wrote:
> > > > 
> > > > > 
> > > > > 	if (!defined($ref_func) || !defined($weak{$text})) {
> > > > > to
> > > > > 	if (!defined($ref_func) && !defined($weak{$text})) {
> > > > > 
> > > > > This now matches the comment above the conditional.
> > > > 
> > > > This is the true bug. But the previous is not.
> > > 
> > > Could you resend the patch with this fix only. Then I can send it out as 
> > > an ugent fix.
> > > 
> > Is anything happening with these patches? Without them, a lot of SH
> > configurations are completely broken.
> 
> Thomas,
> 
> Could you push these off to Linus? My git pull request had:
> 
> Message-ID: <20090723210047.294710482@goodmis.org>
> 
This is still outstanding. Should I just merge it in to my tree and send
it to Linus instead?

Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section

[Ingo Molnar]

* Paul Mundt <lethal@linux-sh.org> wrote:

> > Could you push these off to Linus? My git pull request had:
> > 
> > Message-ID: <20090723210047.294710482@goodmis.org>
> 
> This is still outstanding. Should I just merge it in to my tree 
> and send it to Linus instead?

No need, i've picked them up for -rc6.

Thanks,

	Ingo

Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section

[Paul Mundt]

On Tue, Aug 04, 2009 at 10:07:23AM +0200, Ingo Molnar wrote:
> 
> * Paul Mundt <lethal@linux-sh.org> wrote:
> 
> > > Could you push these off to Linus? My git pull request had:
> > > 
> > > Message-ID: <20090723210047.294710482@goodmis.org>
> > 
> > This is still outstanding. Should I just merge it in to my tree 
> > and send it to Linus instead?
> 
> No need, i've picked them up for -rc6.
> 
Great, thanks Ingo!

[tip:tracing/urgent] ftrace: Fix the conditional that updates $ref_func

[tip-bot for Matt Fleming]

Commit-ID:  fc4c73554c9d93b3e495f2f7acae1323b0d5db84
Gitweb:     http://git.kernel.org/tip/fc4c73554c9d93b3e495f2f7acae1323b0d5db84
Author:     Matt Fleming <matt@console-pimps.org>
AuthorDate: Thu, 23 Jul 2009 17:16:14 +0100
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Thu, 23 Jul 2009 12:20:08 -0400

ftrace: Fix the conditional that updates $ref_func

Fix the conditional that checks if we already have a $ref_func and that
the new function is weak. The code as previously checking whether either
condition was false, and we really need to only update $ref_func is both
cconditions are false.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
LKML-Reference: <1248365775-25196-1-git-send-email-matt@console-pimps.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


---
 scripts/recordmcount.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 7109e2b..16c5563 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -414,7 +414,7 @@ while (<IN>) {
 	    $read_function = 0;
 	} else {
 	    # if we already have a function, and this is weak, skip it
-	    if (!defined($ref_func) || !defined($weak{$text})) {
+	    if (!defined($ref_func) && !defined($weak{$text})) {
 		$ref_func = $text;
 	    }
 	}

[tip:tracing/urgent] ftrace: Only update $offset when we update $ref_func

[tip-bot for Matt Fleming]

Commit-ID:  bd171d5ffc5cb2ba471e8205c679ee9d12b90116
Gitweb:     http://git.kernel.org/tip/bd171d5ffc5cb2ba471e8205c679ee9d12b90116
Author:     Matt Fleming <matt@console-pimps.org>
AuthorDate: Thu, 23 Jul 2009 17:16:15 +0100
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Thu, 23 Jul 2009 12:20:30 -0400

ftrace: Only update $offset when we update $ref_func

The value of $offset should be the offset of $ref_func from the
beginning of the object file. Therefore, we should set both variables
together.

This fixes a bug I was hitting on sh where $offset (which is used to
calcualte the addends for the __mcount_loc entries) was being set
multiple times and didn't correspond to $ref_func's offset in the object
file. The addends in __mcount_loc were calculated incorrectly, resulting
in ftrace dynamically modifying addresses that weren't mcount call
sites.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
LKML-Reference: <1248365775-25196-2-git-send-email-matt@console-pimps.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


---
 scripts/recordmcount.pl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 16c5563..d29baa2 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -403,7 +403,6 @@ while (<IN>) {
     # section found, now is this a start of a function?
     } elsif ($read_function && /$function_regex/) {
 	$text_found = 1;
-	$offset = hex $1;
 	$text = $2;
 
 	# if this is either a local function or a weak function
@@ -412,10 +411,12 @@ while (<IN>) {
 	if (!defined($locals{$text}) && !defined($weak{$text})) {
 	    $ref_func = $text;
 	    $read_function = 0;
+	    $offset = hex $1;
 	} else {
 	    # if we already have a function, and this is weak, skip it
 	    if (!defined($ref_func) && !defined($weak{$text})) {
 		$ref_func = $text;
+		$offset = hex $1;
 	    }
 	}
     } elsif ($read_headers && /$mcount_section/) {