Minor improvements to recordmount.pl

Minor improvements to recordmount.pl

[Wolfram Sang]

Hi,

while playing with ftrace and checking how it works, I also stumbled across
recordmount.pl. The following two patches contain improvements which simplify
the code and make it better readable IMHO. (They should also make it faster,
although I don't think one could measure it.)

I think there is more potential for significant speedups during build-time, but
these would involve structural changes and I'd need to do more research.

So, for a start, pick the low-hanging fruits and remove some "a bit clumsy"
parts :) I did some "unit testing" and built a complete kernel with these
patches applied. No problems, ftrace still works like a charm.

Regards,

   Wolfram

[PATCH 1/2] tracing/recordmount.pl: optimize offsets-handling

[Wolfram Sang]

- move check for open file in front of the writing loop
- use perl-constructs to access the array

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 scripts/recordmcount.pl |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 92f09fe..5de12c7 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -432,14 +432,14 @@ sub update_funcs
 
     # 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;
+    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));
+    }
+    foreach my $cur_offset (@offsets) {
+	printf FILE "\t%s %s + %d\n", $type, $ref_func, $cur_offset - $offset;
     }
 }
 
@@ -514,7 +514,7 @@ while (<IN>) {
     }
     # is this a call site to mcount? If so, record it to print later
     if ($text_found && /$mcount_regex/) {
-	$offsets[$#offsets + 1] = hex $1;
+	push(@offsets, hex $1);
     }
 }
 
-- 
1.6.3.3

[PATCH 2/2] tracing/recordmount.pl: use apropriate perl-constructs

[Wolfram Sang]

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 scripts/recordmcount.pl |   33 ++++++++++++---------------------
 1 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 5de12c7..59b73b9 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -136,13 +136,13 @@ my %text_sections = (
      ".text.unlikely" => 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);
+$objdump ||= 'objdump';
+$objcopy ||= 'objcopy';
+$cc ||= 'gcc';
+$ld ||= 'ld';
+$nm ||= 'nm';
+$rm ||= 'rm';
+$mv ||= 'mv';
 
 #print STDERR "running: $P '$arch' '$objdump' '$objcopy' '$cc' '$ld' " .
 #    "'$nm' '$rm' '$mv' '$inputfile'\n";
@@ -163,9 +163,8 @@ my $section_type;	# Section header plus possible alignment command
 my $can_use_local = 0; 	# If we can use local function references
 
 # Shut up recordmcount if user has older objcopy
-my $quiet_recordmcount = ".tmp_quiet_recordmcount";
-my $print_warning = 1;
-$print_warning = 0 if ( -f $quiet_recordmcount);
+my $quiet_recordmcount = '.tmp_quiet_recordmcount';
+my $print_warning = (! -f $quiet_recordmcount);
 
 ##
 # check_objcopy - whether objcopy supports --globalize-symbols
@@ -194,12 +193,8 @@ sub check_objcopy
     }
 }
 
-if ($arch eq "x86") {
-    if ($bits == 64) {
-	$arch = "x86_64";
-    } else {
-	$arch = "i386";
-    }
+if ($arch eq 'x86') {
+    $arch = ($bits == 64) ? 'x86_64' : 'i386';
 }
 
 #
@@ -476,11 +471,7 @@ while (<IN>) {
 	$read_headers = 0;
 
 	# Only record text sections that we know are safe
-	if (defined($text_sections{$1})) {
-	    $read_function = 1;
-	} else {
-	    $read_function = 0;
-	}
+	$read_function = defined($text_sections{$1});
 	# print out any recorded offsets
 	update_funcs();
 
-- 
1.6.3.3

Re: [PATCH 2/2] tracing/recordmount.pl: use apropriate perl-constructs

[Steven Rostedt]

On Tue, 2010-01-05 at 19:27 +0100, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  scripts/recordmcount.pl |   33 ++++++++++++---------------------
>  1 files changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> index 5de12c7..59b73b9 100755
> --- a/scripts/recordmcount.pl
> +++ b/scripts/recordmcount.pl
> @@ -136,13 +136,13 @@ my %text_sections = (
>       ".text.unlikely" => 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);
> +$objdump ||= 'objdump';
> +$objcopy ||= 'objcopy';
> +$cc ||= 'gcc';
> +$ld ||= 'ld';
> +$nm ||= 'nm';
> +$rm ||= 'rm';
> +$mv ||= 'mv';

I purposely did not do it this way (I need to add a comment about this),
because most kernel developers are not perl programmers, and I wanted
this to be as easy as possible for a non-perl programmer to understand.

Even as a perl programmer it still looks funny to me with the:


  $x ||= 'x';


So I will not accept this part.

>  
>  #print STDERR "running: $P '$arch' '$objdump' '$objcopy' '$cc' '$ld' " .
>  #    "'$nm' '$rm' '$mv' '$inputfile'\n";
> @@ -163,9 +163,8 @@ my $section_type;	# Section header plus possible alignment command
>  my $can_use_local = 0; 	# If we can use local function references
>  
>  # Shut up recordmcount if user has older objcopy
> -my $quiet_recordmcount = ".tmp_quiet_recordmcount";
> -my $print_warning = 1;
> -$print_warning = 0 if ( -f $quiet_recordmcount);
> +my $quiet_recordmcount = '.tmp_quiet_recordmcount';
> +my $print_warning = (! -f $quiet_recordmcount);

Again, this is just using perl obfuscation for most C programmers to
understand.

>  
>  ##
>  # check_objcopy - whether objcopy supports --globalize-symbols
> @@ -194,12 +193,8 @@ sub check_objcopy
>      }
>  }
>  
> -if ($arch eq "x86") {
> -    if ($bits == 64) {
> -	$arch = "x86_64";
> -    } else {
> -	$arch = "i386";
> -    }
> +if ($arch eq 'x86') {
> +    $arch = ($bits == 64) ? 'x86_64' : 'i386';
>  }

This part I like. It keeps to C constructs.

>  
>  #
> @@ -476,11 +471,7 @@ while (<IN>) {
>  	$read_headers = 0;
>  
>  	# Only record text sections that we know are safe
> -	if (defined($text_sections{$1})) {
> -	    $read_function = 1;
> -	} else {
> -	    $read_function = 0;
> -	}
> +	$read_function = defined($text_sections{$1});

I fine with this change too.

-- Steve

>  	# print out any recorded offsets
>  	update_funcs();
>  

Re: [PATCH 1/2] tracing/recordmount.pl: optimize offsets-handling

[Steven Rostedt]

On Tue, 2010-01-05 at 19:27 +0100, Wolfram Sang wrote:
> - move check for open file in front of the writing loop
> - use perl-constructs to access the array
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  scripts/recordmcount.pl |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> index 92f09fe..5de12c7 100755
> --- a/scripts/recordmcount.pl
> +++ b/scripts/recordmcount.pl
> @@ -432,14 +432,14 @@ sub update_funcs
>  
>      # 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;
> +    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));
> +    }
> +    foreach my $cur_offset (@offsets) {
> +	printf FILE "\t%s %s + %d\n", $type, $ref_func, $cur_offset - $offset;
>      }
>  }
>  
> @@ -514,7 +514,7 @@ while (<IN>) {
>      }
>      # is this a call site to mcount? If so, record it to print later
>      if ($text_found && /$mcount_regex/) {
> -	$offsets[$#offsets + 1] = hex $1;
> +	push(@offsets, hex $1);
>      }
>  }
>  

OK, I like this patch. I'll pull it in.

Thanks!

-- Steve

Re: [PATCH 2/2] tracing/recordmount.pl: use apropriate perl-constructs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

> > -$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);
> > +$objdump ||= 'objdump';
> > +$objcopy ||= 'objcopy';
> > +$cc ||= 'gcc';
> > +$ld ||= 'ld';
> > +$nm ||= 'nm';
> > +$rm ||= 'rm';
> > +$mv ||= 'mv';
> 
> I purposely did not do it this way (I need to add a comment about this),
> because most kernel developers are not perl programmers, and I wanted
> this to be as easy as possible for a non-perl programmer to understand.
> 
> Even as a perl programmer it still looks funny to me with the:
> 
> 
>   $x ||= 'x';
> 

Yes, okay, it's an idiom one needs to know. While your point "be C-compatible"
makes also sense to me (comment would be nice indeed), this should do it, too?

	$cc = "gcc" if ($cc == '');

> >  # Shut up recordmcount if user has older objcopy
> > -my $quiet_recordmcount = ".tmp_quiet_recordmcount";
> > -my $print_warning = 1;
> > -$print_warning = 0 if ( -f $quiet_recordmcount);
> > +my $quiet_recordmcount = '.tmp_quiet_recordmcount';
> > +my $print_warning = (! -f $quiet_recordmcount);
> 
> Again, this is just using perl obfuscation for most C programmers to
> understand.

Okay, mileages... :)

> I fine with this change too.

Shall I resend or will you just pick up the interesting parts?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/2] tracing/recordmount.pl: use apropriate perl-constructs

[Steven Rostedt]

On Tue, 2010-01-05 at 20:13 +0100, Wolfram Sang wrote:
> > > -$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);
> > > +$objdump ||= 'objdump';
> > > +$objcopy ||= 'objcopy';
> > > +$cc ||= 'gcc';
> > > +$ld ||= 'ld';
> > > +$nm ||= 'nm';
> > > +$rm ||= 'rm';
> > > +$mv ||= 'mv';
> > 
> > I purposely did not do it this way (I need to add a comment about this),
> > because most kernel developers are not perl programmers, and I wanted
> > this to be as easy as possible for a non-perl programmer to understand.
> > 
> > Even as a perl programmer it still looks funny to me with the:
> > 
> > 
> >   $x ||= 'x';
> > 
> 
> Yes, okay, it's an idiom one needs to know. While your point "be C-compatible"
> makes also sense to me (comment would be nice indeed), this should do it, too?
> 
> 	$cc = "gcc" if ($cc == '');

Yeah that looks better. You can also add a comment that says that this
code is trying to be C programmer friendly, and avoids the "$x ||= 'x'"
construct. Since this is the second or third time I had to deny that
change ;-)

> 
> > >  # Shut up recordmcount if user has older objcopy
> > > -my $quiet_recordmcount = ".tmp_quiet_recordmcount";
> > > -my $print_warning = 1;
> > > -$print_warning = 0 if ( -f $quiet_recordmcount);
> > > +my $quiet_recordmcount = '.tmp_quiet_recordmcount';
> > > +my $print_warning = (! -f $quiet_recordmcount);
> > 
> > Again, this is just using perl obfuscation for most C programmers to
> > understand.
> 
> Okay, mileages... :)
> 
> > I fine with this change too.
> 
> Shall I resend or will you just pick up the interesting parts?

Yes please resend!

Thanks,


-- Steve

[PATCH V2] tracing/recordmount.pl: use apropriate perl-constructs

[Wolfram Sang]

...and add a comment where they are not wanted.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---

Changes since V1:

* dropped the ||=-idiom and simplified the current code

Had to change the syntax for this block again, as "$cc == ''" produces warnings
with 'perl -w' (same for using 'eq'). I hope this version now is also readable
for C-hackers ;) Sorry for the noise here.

* removed the change for $print_warning

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

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 5de12c7..90f2be2 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -136,13 +136,14 @@ my %text_sections = (
      ".text.unlikely" => 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);
+# Note: we are nice to C-programmers here, thus we skip the '||='-idiom.
+$objdump = 'objdump' if (!$objdump);
+$objcopy = 'objcopy' if (!$objcopy);
+$cc = 'gcc' if (!$cc);
+$ld = 'ld' if (!$ld);
+$nm = 'nm' if (!$nm);
+$rm = 'rm' if (!$rm);
+$mv = 'mv' if (!$mv);
 
 #print STDERR "running: $P '$arch' '$objdump' '$objcopy' '$cc' '$ld' " .
 #    "'$nm' '$rm' '$mv' '$inputfile'\n";
@@ -194,12 +195,8 @@ sub check_objcopy
     }
 }
 
-if ($arch eq "x86") {
-    if ($bits == 64) {
-	$arch = "x86_64";
-    } else {
-	$arch = "i386";
-    }
+if ($arch eq 'x86') {
+    $arch = ($bits == 64) ? 'x86_64' : 'i386';
 }
 
 #
@@ -476,11 +473,7 @@ while (<IN>) {
 	$read_headers = 0;
 
 	# Only record text sections that we know are safe
-	if (defined($text_sections{$1})) {
-	    $read_function = 1;
-	} else {
-	    $read_function = 0;
-	}
+	$read_function = defined($text_sections{$1});
 	# print out any recorded offsets
 	update_funcs();
 
-- 
1.6.3.3

[tip:tracing/core] tracing: optimize recordmcount.pl for offsets-handling

[tip-bot for Wolfram Sang]

Commit-ID:  dc4f8845ee2ca39fe054a2d911729ffd269b4b66
Gitweb:     http://git.kernel.org/tip/dc4f8845ee2ca39fe054a2d911729ffd269b4b66
Author:     Wolfram Sang <w.sang@pengutronix.de>
AuthorDate: Tue, 5 Jan 2010 19:27:51 +0100
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Wed, 6 Jan 2010 13:32:39 -0500

tracing: optimize recordmcount.pl for offsets-handling

- move check for open file in front of the writing loop
- use perl-constructs to access the array

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
LKML-Reference: <1262716072-14414-2-git-send-email-w.sang@pengutronix.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 scripts/recordmcount.pl |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 92f09fe..5de12c7 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -432,14 +432,14 @@ sub update_funcs
 
     # 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;
+    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));
+    }
+    foreach my $cur_offset (@offsets) {
+	printf FILE "\t%s %s + %d\n", $type, $ref_func, $cur_offset - $offset;
     }
 }
 
@@ -514,7 +514,7 @@ while (<IN>) {
     }
     # is this a call site to mcount? If so, record it to print later
     if ($text_found && /$mcount_regex/) {
-	$offsets[$#offsets + 1] = hex $1;
+	push(@offsets, hex $1);
     }
 }
 

[tip:tracing/core] tracing: Use appropriate perl constructs in recordmcount.pl

[tip-bot for Wolfram Sang]

Commit-ID:  dfaa9e2c5707b2c217c0121aac796e0fa3051482
Gitweb:     http://git.kernel.org/tip/dfaa9e2c5707b2c217c0121aac796e0fa3051482
Author:     Wolfram Sang <w.sang@pengutronix.de>
AuthorDate: Tue, 5 Jan 2010 21:41:22 +0100
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Wed, 6 Jan 2010 18:08:58 -0500

tracing: Use appropriate perl constructs in recordmcount.pl

Modified recordmcount.pl to use perl constructs that are still
understandable by C hackers that are not perl programmers.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
LKML-Reference: <1262724082-9517-1-git-send-email-w.sang@pengutronix.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 scripts/recordmcount.pl |   29 +++++++++++------------------
 1 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 5de12c7..545fe71 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -136,13 +136,14 @@ my %text_sections = (
      ".text.unlikely" => 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);
+# Note: we are nice to C-programmers here, thus we skip the '||='-idiom.
+$objdump = 'objdump' if (!$objdump);
+$objcopy = 'objcopy' if (!$objcopy);
+$cc = 'gcc' if (!$cc);
+$ld = 'ld' if (!$ld);
+$nm = 'nm' if (!$nm);
+$rm = 'rm' if (!$rm);
+$mv = 'mv' if (!$mv);
 
 #print STDERR "running: $P '$arch' '$objdump' '$objcopy' '$cc' '$ld' " .
 #    "'$nm' '$rm' '$mv' '$inputfile'\n";
@@ -194,12 +195,8 @@ sub check_objcopy
     }
 }
 
-if ($arch eq "x86") {
-    if ($bits == 64) {
-	$arch = "x86_64";
-    } else {
-	$arch = "i386";
-    }
+if ($arch eq 'x86') {
+    $arch = ($bits == 64) ? 'x86_64' : 'i386';
 }
 
 #
@@ -476,11 +473,7 @@ while (<IN>) {
 	$read_headers = 0;
 
 	# Only record text sections that we know are safe
-	if (defined($text_sections{$1})) {
-	    $read_function = 1;
-	} else {
-	    $read_function = 0;
-	}
+	$read_function = defined($text_sections{$1});
 	# print out any recorded offsets
 	update_funcs();