* [PATCH -tip] ftrace: Correctly calculate the first function in the .text section @ 2009-07-23 9:51 Matt Fleming 2009-07-23 14:50 ` Steven Rostedt 0 siblings, 1 reply; 12+ messages in thread From: Matt Fleming @ 2009-07-23 9:51 UTC (permalink / raw) To: Ingo Molnar, Steven Rostedt; +Cc: linux-kernel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section 2009-07-23 9:51 [PATCH -tip] ftrace: Correctly calculate the first function in the .text section Matt Fleming @ 2009-07-23 14:50 ` Steven Rostedt 2009-07-23 14:52 ` Steven Rostedt 2009-07-23 16:16 ` [PATCH 1/2] ftrace: Fix the conditional that updates $ref_func Matt Fleming 0 siblings, 2 replies; 12+ messages in thread From: Steven Rostedt @ 2009-07-23 14:50 UTC (permalink / raw) To: Matt Fleming; +Cc: Ingo Molnar, linux-kernel 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section 2009-07-23 14:50 ` Steven Rostedt @ 2009-07-23 14:52 ` Steven Rostedt 2009-07-27 22:29 ` Paul Mundt 2009-07-23 16:16 ` [PATCH 1/2] ftrace: Fix the conditional that updates $ref_func Matt Fleming 1 sibling, 1 reply; 12+ messages in thread From: Steven Rostedt @ 2009-07-23 14:52 UTC (permalink / raw) To: Matt Fleming; +Cc: Ingo Molnar, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section 2009-07-23 14:52 ` Steven Rostedt @ 2009-07-27 22:29 ` Paul Mundt 2009-07-27 23:45 ` Steven Rostedt 0 siblings, 1 reply; 12+ messages in thread From: Paul Mundt @ 2009-07-27 22:29 UTC (permalink / raw) To: Steven Rostedt; +Cc: Matt Fleming, Ingo Molnar, linux-kernel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section 2009-07-27 22:29 ` Paul Mundt @ 2009-07-27 23:45 ` Steven Rostedt 2009-08-02 8:16 ` Paul Mundt 0 siblings, 1 reply; 12+ messages in thread From: Steven Rostedt @ 2009-07-27 23:45 UTC (permalink / raw) To: Paul Mundt; +Cc: Matt Fleming, Ingo Molnar, LKML, Thomas Gleixner 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section 2009-07-27 23:45 ` Steven Rostedt @ 2009-08-02 8:16 ` Paul Mundt 2009-08-04 8:07 ` Ingo Molnar 0 siblings, 1 reply; 12+ messages in thread From: Paul Mundt @ 2009-08-02 8:16 UTC (permalink / raw) To: Steven Rostedt; +Cc: Matt Fleming, Ingo Molnar, LKML, Thomas Gleixner 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section 2009-08-02 8:16 ` Paul Mundt @ 2009-08-04 8:07 ` Ingo Molnar 2009-08-04 8:08 ` Paul Mundt 0 siblings, 1 reply; 12+ messages in thread From: Ingo Molnar @ 2009-08-04 8:07 UTC (permalink / raw) To: Paul Mundt, Steven Rostedt, Matt Fleming, LKML, Thomas Gleixner * 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section 2009-08-04 8:07 ` Ingo Molnar @ 2009-08-04 8:08 ` Paul Mundt 0 siblings, 0 replies; 12+ messages in thread From: Paul Mundt @ 2009-08-04 8:08 UTC (permalink / raw) To: Ingo Molnar; +Cc: Steven Rostedt, Matt Fleming, LKML, Thomas Gleixner 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! ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] ftrace: Fix the conditional that updates $ref_func 2009-07-23 14:50 ` Steven Rostedt 2009-07-23 14:52 ` Steven Rostedt @ 2009-07-23 16:16 ` Matt Fleming 2009-07-23 16:16 ` [PATCH 2/2] ftrace: Only update $offset when we update $ref_func Matt Fleming 2009-08-04 8:10 ` [tip:tracing/urgent] ftrace: Fix the conditional that updates $ref_func tip-bot for Matt Fleming 1 sibling, 2 replies; 12+ messages in thread From: Matt Fleming @ 2009-07-23 16:16 UTC (permalink / raw) To: Steven Rostedt; +Cc: linux-kernel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] ftrace: Only update $offset when we update $ref_func 2009-07-23 16:16 ` [PATCH 1/2] ftrace: Fix the conditional that updates $ref_func Matt Fleming @ 2009-07-23 16:16 ` Matt Fleming 2009-08-04 8:10 ` [tip:tracing/urgent] " tip-bot for Matt Fleming 2009-08-04 8:10 ` [tip:tracing/urgent] ftrace: Fix the conditional that updates $ref_func tip-bot for Matt Fleming 1 sibling, 1 reply; 12+ messages in thread From: Matt Fleming @ 2009-07-23 16:16 UTC (permalink / raw) To: Steven Rostedt; +Cc: linux-kernel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [tip:tracing/urgent] ftrace: Only update $offset when we update $ref_func 2009-07-23 16:16 ` [PATCH 2/2] ftrace: Only update $offset when we update $ref_func Matt Fleming @ 2009-08-04 8:10 ` tip-bot for Matt Fleming 0 siblings, 0 replies; 12+ messages in thread From: tip-bot for Matt Fleming @ 2009-08-04 8:10 UTC (permalink / raw) To: linux-tip-commits; +Cc: linux-kernel, hpa, mingo, rostedt, matt, tglx 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/) { ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [tip:tracing/urgent] ftrace: Fix the conditional that updates $ref_func 2009-07-23 16:16 ` [PATCH 1/2] ftrace: Fix the conditional that updates $ref_func Matt Fleming 2009-07-23 16:16 ` [PATCH 2/2] ftrace: Only update $offset when we update $ref_func Matt Fleming @ 2009-08-04 8:10 ` tip-bot for Matt Fleming 1 sibling, 0 replies; 12+ messages in thread From: tip-bot for Matt Fleming @ 2009-08-04 8:10 UTC (permalink / raw) To: linux-tip-commits; +Cc: linux-kernel, hpa, mingo, rostedt, matt, tglx 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; } } ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2009-08-04 8:11 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-07-23 9:51 [PATCH -tip] ftrace: Correctly calculate the first function in the .text section Matt Fleming 2009-07-23 14:50 ` Steven Rostedt 2009-07-23 14:52 ` Steven Rostedt 2009-07-27 22:29 ` Paul Mundt 2009-07-27 23:45 ` Steven Rostedt 2009-08-02 8:16 ` Paul Mundt 2009-08-04 8:07 ` Ingo Molnar 2009-08-04 8:08 ` Paul Mundt 2009-07-23 16:16 ` [PATCH 1/2] ftrace: Fix the conditional that updates $ref_func Matt Fleming 2009-07-23 16:16 ` [PATCH 2/2] ftrace: Only update $offset when we update $ref_func Matt Fleming 2009-08-04 8:10 ` [tip:tracing/urgent] " tip-bot for Matt Fleming 2009-08-04 8:10 ` [tip:tracing/urgent] ftrace: Fix the conditional that updates $ref_func tip-bot for Matt Fleming
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.