Now the implementation of recordmcount.pl have several problems:
* This tool is not implemented as the documentation (the block comment at
the head of the file) says. It will not always choose the first function
as offset reference. e.g.
.section ".sched.text", "ax"
[...]
func1:
[...]
call mcount (offset: 0x10)
[...]
ret
.globl fun2
func2: (offset: 0x20)
[...]
[...]
ret
func3:
[...]
call mcount (offset: 0x30)
[...]
First we will select func1, but we forget to set $read_function = 0 here
(Line 410 - 423). When we meet func2, we will replace $ref_func to func2
and finally set $read_function = 0 at Line 412.
Luckily, this 'wrongly' implementation accidently makes the result same to
the documentation. Because we use $ref_func + (call offset - ref_func offset)
in tmp.s, here any ref func can give a right address finally.
* The algorithm and implementation is a little complex. As above shows, we
can use any function in this section as a reference, not just the first
one. This will simplify the code much. A reasonable method is: choose the
first global function we meet as a reference. If there is none, choose the
first local one.
* mcount section check is wrong. If the mcount section is not the first
section, we will never find it.
* Find objcopy version is wrong. If objcopy has a version but is less than
2.17, we will still disable local function reference. $found_version can't
be used to determine the show of the warning message.
* Argment number check is wrong. Should be 10.
* .L check path is partial, not on all run path.
* If there is no global and local symbols, return early. If there are only
locals and can't use local reference, return early.
* input file can be absolute path?
* Some documenation and in-line comment is wrong.
* Code deals with weak functions will not be called. Remove it.
To resolve all these problems, I rewrite some part of the code and make the
algorithm and code simpler and cleaner.
Two pending issues:
* I ignore all the weak functions now. Is that a big impact?
* I just make some simple tests. Steven, are there some ways to do a full
and strong verification? Also I will very appreciate if some guys can help
me do some tests on other arches except x86.
As the change is large, except the patch, I also put the new version file in
the attchment.
Signed-off-by: Li Hong <[email protected]>
---
scripts/recordmcount.pl | 487 ++++++++++++++++++++++-------------------------
1 files changed, 224 insertions(+), 263 deletions(-)
diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 090d300..1eeff67 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -1,82 +1,99 @@
#!/usr/bin/perl -w
# (c) 2008, Steven Rostedt <[email protected]>
+# (C) 2009, Li Hong <[email protected]>
+#
# Licensed under the terms of the GNU GPL License version 2
#
# recordmcount.pl - makes a section called __mcount_loc that holds
# all the offsets to the calls to mcount.
#
#
-# What we want to end up with is a section in vmlinux called
-# __mcount_loc that contains a list of pointers to all the
-# call sites in the kernel that call mcount. Later on boot up, the kernel
-# will read this list, save the locations and turn them into nops.
-# When tracing or profiling is later enabled, these locations will then
-# be converted back to pointers to some function.
+# What we want to end up with this is a data area in .init.data in vmlinux
+# that contains a list of pointers to all the call sites in the kernel that
+# call mcount. Later on boot up, the kernel will read this list, save the
+# locations and turn them into nops. When tracing or profiling is later
+# enabled, these locations will then be converted back to pointers to
+# some function.
#
# This is no easy feat. This script is called just after the original
# object is compiled and before it is linked.
#
-# The references to the call sites are offsets from the section of text
-# that the call site is in. Hence, all functions in a section that
-# has a call site to mcount, will have the offset from the beginning of
-# the section and not the beginning of the function.
+# When parse this object file using 'objdump', the references to the call
+# sites are offsets from the section that the call site is in. Hence, all
+# functions in a section that has a call site to mcount, will have the
+# offset from the beginning of the section and not the beginning of the
+# function.
+#
+# But where this section will reside finally in vmlinx is undetermined at
+# this point. So we can't use this kind of offsets to record the final
+# address of this call site.
#
-# The trick is to find a way to record the beginning of the section.
-# The way we do this is to look at the first function in the section
-# which will also be the location of that section after final link.
+# The trick is to change the call offset refering the start of a section to
+# refering a function symbol in this section. During the link step, 'ld' will
+# compute the final address according to the information we record.
+#
# e.g.
#
# .section ".sched.text", "ax"
-# .globl my_func
-# my_func:
# [...]
-# call mcount (offset: 0x5)
+# func1:
+# [...]
+# call mcount (offset: 0x10)
# [...]
# ret
-# other_func:
+# .globl fun2
+# func2: (offset: 0x20)
# [...]
-# call mcount (offset: 0x1b)
+# [...]
+# ret
+# func3:
+# [...]
+# call mcount (offset: 0x30)
# [...]
#
# Both relocation offsets for the mcounts in the above example will be
-# offset from .sched.text. If we make another file called tmp.s with:
+# offset from .sched.text. If we choose global symbol func2 as a reference and
+# make another file called tmp.s with the new offsets:
#
# .section __mcount_loc
-# .quad my_func + 0x5
-# .quad my_func + 0x1b
+# .quad func2 - 0x10
+# .quad func2 + 0x10
#
-# We can then compile this tmp.s into tmp.o, and link it to the original
+# We can then compile this tmp.s into tmp.o, and link it back to the original
# object.
#
-# But this gets hard if my_func is not globl (a static function).
-# In such a case we have:
+# In our algorithm, we will choose the first global function we meet in this
+# section as the reference. But this gets hard if there is no global functions
+# in this section. In such a case we have to select a local one. E.g. func1:
#
# .section ".sched.text", "ax"
-# my_func:
+# func1:
# [...]
-# call mcount (offset: 0x5)
+# call mcount (offset: 0x10)
# [...]
# ret
-# other_func:
+# func2:
# [...]
-# call mcount (offset: 0x1b)
+# call mcount (offset: 0x20)
# [...]
+# .section "other.section"
#
# If we make the tmp.s the same as above, when we link together with
-# the original object, we will end up with two symbols for my_func:
+# the original object, we will end up with two symbols for func1:
# one local, one global. After final compile, we will end up with
-# an undefined reference to my_func.
+# an undefined reference to func1 or a wrong reference to another global
+# func1 in other files.
#
# Since local objects can reference local variables, we need to find
# a way to make tmp.o reference the local objects of the original object
-# file after it is linked together. To do this, we convert the my_func
+# file after it is linked together. To do this, we convert func1
# into a global symbol before linking tmp.o. Then after we link tmp.o
-# we will only have a single symbol for my_func that is global.
-# We can convert my_func back into a local symbol and we are done.
+# we will only have a single symbol for func1 that is global.
+# We can convert func1 back into a local symbol and we are done.
#
# Here are the steps we take:
#
-# 1) Record all the local symbols by using 'nm'
+# 1) Record all the global and local symbols by using 'nm'
# 2) Use objdump to find all the call site offsets and sections for
# mcount.
# 3) Compile the list into its own object.
@@ -86,7 +103,6 @@
# 6) Link together this new object with the list object.
# 7) Convert the local functions back to local symbols and rename
# the result as the original object.
-# End.
# 8) Link the object with the list object.
# 9) Move the result back to the original object.
# End.
@@ -94,56 +110,139 @@
use strict;
-my $P = $0;
-$P =~ s@.*/@@g;
-
+my $P = $0; $P =~ s@.*/@@g;
my $V = '0.1';
-
-if ($#ARGV < 7) {
- print "usage: $P arch bits objdump objcopy cc ld nm rm mv is_module inputfile\n";
- print "version: $V\n";
- exit(1);
-}
-
+my $opened = 0;
my ($arch, $bits, $objdump, $objcopy, $cc,
$ld, $nm, $rm, $mv, $is_module, $inputfile) = @ARGV;
+my ($dirname, $filename, $prefix, $ext, $mcount_s, $mcount_o);
-# This file refers to mcount and shouldn't be ftraced, so lets' ignore it
-if ($inputfile eq "kernel/trace/ftrace.o") {
- exit(0);
-}
-
-# Acceptable sections to record.
-my %text_sections = (
- ".text" => 1,
- ".sched.text" => 1,
- ".spinlock.text" => 1,
- ".irqentry.text" => 1,
-);
-
-$objdump = "objdump" if ((length $objdump) == 0);
-$objcopy = "objcopy" if ((length $objcopy) == 0);
-$cc = "gcc" if ((length $cc) == 0);
-$ld = "ld" if ((length $ld) == 0);
-$nm = "nm" if ((length $nm) == 0);
-$rm = "rm" if ((length $rm) == 0);
-$mv = "mv" if ((length $mv) == 0);
-
-#print STDERR "running: $P '$arch' '$objdump' '$objcopy' '$cc' '$ld' " .
-# "'$nm' '$rm' '$mv' '$inputfile'\n";
-
+my %globals; # List of global functions
my %locals; # List of local (static) functions
-my %weak; # List of weak functions
my %convert; # List of local functions used that needs conversion
-my $type;
-my $nm_regex; # Find the local functions (return function)
+# The following three variables are only valid in one section and will
+# re-initialized in a new section.
+my @offsets; # Array of offsets of mcount callers
+my $ref_func; # Reference function used for offsets
+my $offset; # Offset of ref_func to section beginning
+
+my $global_regex; # Match a global function (return function)
+my $local_regex; # Match a local function (return function)
my $section_regex; # Find the start of a section
-my $function_regex; # Find the name of a function
- # (return offset and func name)
+my $function_regex; # Find the name of a function (return offset and function)
my $mcount_regex; # Find the call site to mcount (return offset)
+
+my $can_use_local = 0; # If we can use local function references
my $alignment; # The .align value to use for $mcount_section
+my $type; # The type to use for $mcount_section
my $section_type; # Section header plus possible alignment command
+my $mcount_section; # Mcount section name
+my %text_sections = ( # Acceptable sections to record.
+ ".text" => 1,
+ ".sched.text" => 1,
+ ".spinlock.text" => 1,
+ ".irqentry.text" => 1,
+);
+
+$global_regex = "^[0-9a-fA-F]+\\s+T\\s+(\\S+)";
+$local_regex = "^[0-9a-fA-F]+\\s+t\\s+(\\S+)";
+$section_regex = "Disassembly of section\\s+(\\S+):";
+$function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
+$mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount\$";
+$mcount_section = "__mcount_loc";
+$section_type = '@progbits';
+$type = ".long";
+
+##
+# check_objcopy - whether objcopy supports --globalize-symbols
+#
+# --globalize-symbols came out in 2.17, we must test the version
+# of objcopy, and if it is less than 2.17, then we can not
+# record local functions.
+sub check_objcopy
+{
+ open (IN, "$objcopy --version |") or die "error running $objcopy";
+ while (<IN>) {
+ if (/objcopy.*\s(\d+)\.(\d+)/) {
+ $can_use_local = 1 if ($1 > 2 || ($1 == 2 && $2 >= 17));
+ last;
+ }
+ }
+ close (IN);
+
+ print STDERR "WARNING: could not find objcopy version or version is less than 2.17.\n" .
+ "\tLocal function references is disabled.\n" if !$can_use_local;
+}
+
+##
+# update_funcs - print out the current mcount callers
+#
+# Loop through all the mcount caller offsets and print a reference
+# to the caller based from the ref_func.
+#
+sub update_funcs
+{
+ if ($ref_func && @offsets) {
+ return if $locals{$ref_func} && !$can_use_local;
+ $convert{$ref_func} = 1 if $locals{$ref_func};
+
+ if (!$opened) {
+ open(FILE, ">$mcount_s") or die "can't create $mcount_s\n";
+ $opened = 1;
+ print FILE "\t.section $mcount_section,\"a\",$section_type\n";
+ print FILE "\t.align $alignment\n" if (defined($alignment));
+ }
+
+ foreach (@offsets) {
+ printf FILE "\t%s %s + %d\n", $type, $ref_func, $_ - $offset;
+ }
+ }
+}
+
+
+##
+# Real work starts from here
+#
+
+if ($#ARGV != 10) {
+ print "usage: $P arch bits objdump objcopy cc ld nm rm mv is_module inputfile\n";
+ print "version: $V\n";
+ exit(1);
+}
+
+# Some arguments can be empty string
+$objdump = "objdump" unless $objdump;
+$objcopy = "objcopy" unless $objcopy;
+$cc = "gcc" unless $cc;
+$ld = "ld" unless $ld;
+$nm = "nm" unless $nm;
+$rm = "rm" unless $rm;
+$mv = "mv" unless $mv;
+
+# This file refers to mcount and shouldn't be ftraced, so let's ignore it
+exit(0) if ($inputfile =~ "kernel/trace/ftrace.o");
+
+check_objcopy();
+
+if ($inputfile =~ m,^(.*)/([^/]*)$,) {
+ $dirname = $1;
+ $filename = $2;
+} else {
+ $dirname = ".";
+ $filename = $inputfile;
+}
+
+if ($filename =~ m,^(.*)(\.\S),) {
+ $prefix = $1;
+ $ext = $2;
+} else {
+ $prefix = $filename;
+ $ext = "";
+}
+
+$mcount_s = $dirname . "/.tmp_mc_" . $prefix . ".s";
+$mcount_o = $dirname . "/.tmp_mc_" . $prefix . ".o";
if ($arch eq "x86") {
if ($bits == 64) {
@@ -153,17 +252,6 @@ if ($arch eq "x86") {
}
}
-#
-# We base the defaults off of i386, the other archs may
-# feel free to change them in the below if statements.
-#
-$nm_regex = "^[0-9a-fA-F]+\\s+t\\s+(\\S+)";
-$section_regex = "Disassembly of section\\s+(\\S+):";
-$function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
-$mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount\$";
-$section_type = '@progbits';
-$type = ".long";
-
if ($arch eq "x86_64") {
$mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount([+-]0x[0-9a-zA-Z]+)?\$";
$type = ".quad";
@@ -206,7 +294,8 @@ if ($arch eq "x86_64") {
$cc .= " -m32";
} elsif ($arch eq "powerpc") {
- $nm_regex = "^[0-9a-fA-F]+\\s+t\\s+(\\.?\\S+)";
+ $global_regex = "^[0-9a-fA-F]+\\s+T\\s+(\\.?\\S+)";
+ $local_regex = "^[0-9a-fA-F]+\\s+t\\s+(\\.?\\S+)";
$function_regex = "^([0-9a-fA-F]+)\\s+<(\\.?.*?)>:";
$mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\s\\.?_mcount\$";
@@ -249,201 +338,75 @@ if ($arch eq "x86_64") {
die "Arch $arch is not supported with CONFIG_FTRACE_MCOUNT_RECORD";
}
-my $text_found = 0;
-my $read_function = 0;
-my $opened = 0;
-my $mcount_section = "__mcount_loc";
-
-my $dirname;
-my $filename;
-my $prefix;
-my $ext;
-
-if ($inputfile =~ m,^(.*)/([^/]*)$,) {
- $dirname = $1;
- $filename = $2;
-} else {
- $dirname = ".";
- $filename = $inputfile;
-}
-
-if ($filename =~ m,^(.*)(\.\S),) {
- $prefix = $1;
- $ext = $2;
-} else {
- $prefix = $filename;
- $ext = "";
-}
-
-my $mcount_s = $dirname . "/.tmp_mc_" . $prefix . ".s";
-my $mcount_o = $dirname . "/.tmp_mc_" . $prefix . ".o";
-
#
-# --globalize-symbols came out in 2.17, we must test the version
-# of objcopy, and if it is less than 2.17, then we can not
-# record local functions.
-my $use_locals = 01;
-my $local_warn_once = 0;
-my $found_version = 0;
-
-open (IN, "$objcopy --version |") || die "error running $objcopy";
-while (<IN>) {
- if (/objcopy.*\s(\d+)\.(\d+)/) {
- my $major = $1;
- my $minor = $2;
-
- $found_version = 1;
- if ($major < 2 ||
- ($major == 2 && $minor < 17)) {
- $use_locals = 0;
- }
- last;
- }
-}
-close (IN);
-
-if (!$found_version) {
- print STDERR "WARNING: could not find objcopy version.\n" .
- "\tDisabling local function references.\n";
-}
-
+# Step 1: Find all global and local fuctions. "T" is global and "t" is local
#
-# Step 1: find all the local (static functions) and weak symbols.
-# 't' is local, 'w/W' is weak (we never use a weak function)
-#
-open (IN, "$nm $inputfile|") || die "error running $nm";
+open (IN, "$nm $inputfile|") or die "error running $nm";
while (<IN>) {
- if (/$nm_regex/) {
+ if (/$global_regex/) {
+ $globals{$1} = 1;
+ } elsif (/$local_regex/) {
$locals{$1} = 1;
- } elsif (/^[0-9a-fA-F]+\s+([wW])\s+(\S+)/) {
- $weak{$2} = $1;
}
}
close(IN);
-my @offsets; # Array of offsets of mcount callers
-my $ref_func; # reference function to use for offsets
-my $offset = 0; # offset of ref_func to section beginning
+# Exit early if no work to do
+exit(0) unless (%globals or (%locals and $can_use_local));
-##
-# update_funcs - print out the current mcount callers
#
-# Go through the list of offsets to callers and write them to
-# the output file in a format that can be read by an assembler.
+# Step 2: For each section we concern, record all mcount call information
#
-sub update_funcs
-{
- return if ($#offsets < 0);
-
- defined($ref_func) || die "No function to reference";
-
- # A section only had a weak function, to represent it.
- # Unfortunately, a weak function may be overwritten by another
- # function of the same name, making all these offsets incorrect.
- # To be safe, we simply print a warning and bail.
- if (defined $weak{$ref_func}) {
- print STDERR
- "$inputfile: WARNING: referencing weak function" .
- " $ref_func for mcount\n";
- return;
- }
-
- # is this function static? If so, note this fact.
- if (defined $locals{$ref_func}) {
-
- # only use locals if objcopy supports globalize-symbols
- if (!$use_locals) {
- return;
- }
- $convert{$ref_func} = 1;
- }
-
- # Loop through all the mcount caller offsets and print a reference
- # to the caller based from the ref_func.
- for (my $i=0; $i <= $#offsets; $i++) {
- if (!$opened) {
- open(FILE, ">$mcount_s") || die "can't create $mcount_s\n";
- $opened = 1;
- print FILE "\t.section $mcount_section,\"a\",$section_type\n";
- print FILE "\t.align $alignment\n" if (defined($alignment));
- }
- printf FILE "\t%s %s + %d\n", $type, $ref_func, $offsets[$i] - $offset;
- }
-}
-
-#
-# Step 2: find the sections and mcount call sites
-#
-open(IN, "$objdump -hdr $inputfile|") || die "error running $objdump";
-
-my $text;
-
-my $read_headers = 1;
+open(IN, "$objdump -hdr $inputfile|") or die "error running $objdump";
+my $valid_section = 0;
while (<IN>) {
- # is it a section?
- if (/$section_regex/) {
- $read_headers = 0;
-
- # Only record text sections that we know are safe
- if (defined($text_sections{$1})) {
- $read_function = 1;
- } else {
- $read_function = 0;
- }
- # print out any recorded offsets
- update_funcs() if (defined($ref_func));
-
- # reset all markers and arrays
- $text_found = 0;
- undef($ref_func);
- undef(@offsets);
-
- # section found, now is this a start of a function?
- } elsif ($read_function && /$function_regex/) {
- $text_found = 1;
- $text = $2;
-
- # if this is either a local function or a weak function
- # keep looking for functions that are global that
- # we can use safely.
- 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}) &&
- # PPC64 can have symbols that start with .L and
- # gcc considers these special. Don't use them!
- $text !~ /^\.L/) {
- $ref_func = $text;
- $offset = hex $1;
- }
- }
- } elsif ($read_headers && /$mcount_section/) {
- #
- # Somehow the make process can execute this script on an
- # object twice. If it does, we would duplicate the mcount
- # section and it will cause the function tracer self test
- # to fail. Check if the mcount section exists, and if it does,
- # warn and exit.
- #
+ # Somehow the make process can execute this script on an object twice.
+ # If it does, we would duplicate the mcount section and it will cause the
+ # function tracer self test to fail. Check if the mcount section exists,
+ # and if it does, warn and exit.
+ if (/$mcount_section/) {
print STDERR "ERROR: $mcount_section already in $inputfile\n" .
"\tThis may be an indication that your build is corrupted.\n" .
"\tDelete $inputfile and try again. If the same object file\n" .
"\tstill causes an issue, then disable CONFIG_DYNAMIC_FTRACE.\n";
+ close(FILE) and `$rm $mcount_s` if $opened;
exit(-1);
}
+
+ # the start of a new section
+ if (/$section_regex/) {
+ # write out the offsets in previous section if there are some
+ update_funcs();
- # is this a call site to mcount? If so, record it to print later
- if ($text_found && /$mcount_regex/) {
+ # reset for a new section
+ undef($ref_func);
+ undef(@offsets) ;
+ $valid_section = 0;
+
+ # this is a section we concern?
+ $valid_section = 1 if $text_sections{$1};
+ }
+
+ # record a ref_function and this offset
+ if ($valid_section && /$function_regex/) {
+ # PPC64 can have symbols that start with .L and
+ # gcc considers these special. Don't use them!
+ if ($2 !~ /^\.L/
+ && ((!$ref_func && ($globals{$2} || $locals{$2})) # set a new ref_func
+ || ($ref_func && $locals{$ref_func} && $globals{$2}))) { # replace a local
+ $ref_func = $2;
+ $offset = hex $1;
+ }
+ }
+
+ # record a mcount call site
+ if ($valid_section && /$mcount_regex/) {
$offsets[$#offsets + 1] = hex $1;
}
}
-# dump out anymore offsets that may have been found
-update_funcs() if (defined($ref_func));
+update_funcs();
# If we did not find any mcount callers, we are done (do nothing).
if (!$opened) {
@@ -457,18 +420,16 @@ close(FILE);
#
`$cc -o $mcount_o -c $mcount_s`;
-my @converts = keys %convert;
-
#
# Step 4: Do we have sections that started with local functions?
#
-if ($#converts >= 0) {
+if (keys %convert) {
my $globallist = "";
my $locallist = "";
- foreach my $con (@converts) {
- $globallist .= " --globalize-symbol $con";
- $locallist .= " --localize-symbol $con";
+ foreach (keys %convert) {
+ $globallist .= " --globalize-symbol $_";
+ $locallist .= " --localize-symbol $_";
}
my $globalobj = $dirname . "/.tmp_gl_" . $filename;
--
1.6.0.4
Hi!
BTW, please send to my [email protected] account. My Red Hat mail has
horrible spam filters, and I almost deleted this because it was in
between 40 spam emails. I just noticed "tracing" in the subject, and had
to undelete this email.
On Sun, 2009-10-25 at 20:02 +0800, Li Hong wrote:
> Now the implementation of recordmcount.pl have several problems:
Heh, one problem people have told me is that it is written in perl ;-)
>
> * This tool is not implemented as the documentation (the block comment at
> the head of the file) says. It will not always choose the first function
> as offset reference. e.g.
>
> .section ".sched.text", "ax"
> [...]
> func1:
> [...]
> call mcount (offset: 0x10)
> [...]
> ret
> .globl fun2
> func2: (offset: 0x20)
> [...]
> [...]
> ret
> func3:
> [...]
> call mcount (offset: 0x30)
> [...]
>
> First we will select func1, but we forget to set $read_function = 0 here
> (Line 410 - 423). When we meet func2, we will replace $ref_func to func2
> and finally set $read_function = 0 at Line 412.
read_function means that we want to still check for functions. If we
don't have a global yet, then we want to still check for functions, so
we do not want to stop read_function. But if you look at the code, it
does exactly what I want.
$read_function set to 1 when we have a proper section.
We see func1 which is local, so we record it and continue. If func2 was
still local, we would not record it because we already have a ref
function...
< $read_function is set to 1 when we found a section to parse >
} elsif ($read_function && /$function_regex/) {
$text_found = 1;
$text = $2;
# if this is either a local function or a weak function
# keep looking for functions that are global that
# we can use safely.
if (!defined($locals{$text}) && !defined($weak{$text})) {
< if this is global and not weak, we use it and stop our search>
$ref_func = $text;
$read_function = 0;
$offset = hex $1;
} else {
< the current text is not global, or is weak >
# if we already have a function, and this is weak, skip it
if (!defined($ref_func) && !defined($weak{$text}) &&
< note the !defined($ref_func) this means we only go in the if, if we have
not hit any function yet. >
# PPC64 can have symbols that start with .L and
# gcc considers these special. Don't use them!
$text !~ /^\.L/) {
$ref_func = $text;
$offset = hex $1;
}
}
>
> Luckily, this 'wrongly' implementation accidently makes the result same to
> the documentation. Because we use $ref_func + (call offset - ref_func offset)
> in tmp.s, here any ref func can give a right address finally.
Correct, but as I shown above, there was no bug to begin with.
>
> * The algorithm and implementation is a little complex. As above shows, we
> can use any function in this section as a reference, not just the first
> one. This will simplify the code much. A reasonable method is: choose the
> first global function we meet as a reference. If there is none, choose the
> first local one.
The code already does that.
>
> * mcount section check is wrong. If the mcount section is not the first
> section, we will never find it.
Wrong, we do a header check, not a section check. One of the options I
pass into objdump is "-h" which dumps the headers. I search all headers
for __mcount_loc before we start looking at sections. When we go into
the while loop, I have $read_headers = 1. Which means we are reading the
headers and that's the only place I look. When we hit our first section,
the $read_headers turns to 0, and we are done looking at them.
>
> * Find objcopy version is wrong. If objcopy has a version but is less than
> 2.17, we will still disable local function reference. $found_version can't
> be used to determine the show of the warning message.
Ouch, that is a bug. $find_version should not be used, but $use_locals
should. Hmm, and the initialization of that is also wrong. Ug, that
whole logic needs to be fixed. Thanks!
>
> * Argment number check is wrong. Should be 10.
Ah, we keep adding arguments, and forgetting to update the counter.
Thanks.
>
> * .L check path is partial, not on all run path.
It only matters on local symbols, and it is checked there.
>
> * If there is no global and local symbols, return early. If there are only
> locals and can't use local reference, return early.
Not sure what you mean by the above.
>
> * input file can be absolute path?
Hmm, is this about the ftrace.c check. Not sure if full path is passed
in.
>
> * Some documenation and in-line comment is wrong.
I'm sure it is ;-)
>
> * Code deals with weak functions will not be called. Remove it.
Eek!!! No!!!! This will cause a bug! If we pick a weak function as a
reference and use it, and that weak function is overridden by another
function, we will be pointing to all wrong locations. That weak code
must stay!
>
> To resolve all these problems, I rewrite some part of the code and make the
> algorithm and code simpler and cleaner.
>
> Two pending issues:
>
> * I ignore all the weak functions now. Is that a big impact?
Yep, as mentioned above.
>
> * I just make some simple tests. Steven, are there some ways to do a full
> and strong verification? Also I will very appreciate if some guys can help
> me do some tests on other arches except x86.
I mostly did hand inspections of code. As well as testing the final
results. The ftrace code should test to make sure things work on bootup.
There's lots of self tests that it runs.
Also you can cat out debugfs/tracing/available_filter_functions into a
temp file, before and after your changes and make sure they are
identical. If not, you must understand fully why they are not.
>
> As the change is large, except the patch, I also put the new version file in
> the attchment.
Changes like this should be broken up into several patches. One for
every point. Only a few of the above I would accept. Namely the objdump
version thing and perhaps some comments.
As for the other changes, I'm not sure. As I've said, there was nothing
broken to start with for a lot of those points. If you make it cleaner,
I'm fine with that. But it must maintain functionality. Another test is
to do a objdump of the mcount_loc before and after your changes to make
sure they are still the same.
Also, I'm going to start working on converting this to C. There will be
a check to see if the build machine has libelf installed, and if so, it
will compile a C program to do these changes, and if not, it will fall
back to the perl script.
Thanks,
-- Steve