[PATCH 1/4] scripts/checkstack.pl: don't display $dre as different entity

[PATCH 1/4] scripts/checkstack.pl: don't display $dre as different entity

[Maninder Singh]

currnetly script prints stack usage for functions
in two ways:($re and $dre)

dre breaks sorting mechanism.
0xffffa00011f26f88 sunxi_mux_clk_setup.isra.0 [vmlinux]:Dynamic (0x140)
..
0xffffa00011f27210 sunxi_divs_clk_setup [vmlinux]:      Dynamic (0x1d0)

so we can print it in decimal only.

Also address before function name is changed to function
start address rather than stack consumption address.
Because in next patch, arm has two ways to use stack
which can be clubbed and printed in one function only.

All symbols whose stack by adding(re and dre) is greater than
100, will be printed.

0xffffa00011f2720c0 sunxi_divs_clk_setup [vmlinux]:     464
...
0xffffa00011f26f840 sunxi_mux_clk_setup.isra.0 [vmlinux]:320

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 scripts/checkstack.pl | 52 +++++++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/scripts/checkstack.pl b/scripts/checkstack.pl
index 371bd17..412c459 100755
--- a/scripts/checkstack.pl
+++ b/scripts/checkstack.pl
@@ -109,11 +109,28 @@ my (@stack, $re, $dre, $x, $xs, $funcre);
 #
 # main()
 #
-my ($func, $file, $lastslash);
+my ($func, $file, $lastslash, $total_size, $addr, $intro);
 
 while (my $line = <STDIN>) {
 	if ($line =~ m/$funcre/) {
+		if ($total_size > 100) {
+			push @stack, "$intro$total_size\n";
+		}
+
 		$func = $1;
+		next if $line !~ m/^($xs*)/;
+		$addr = $1;
+		$addr =~ s/ /0/g;
+		$addr = "0x$addr";
+
+		$intro = "$addr $func [$file]:";
+		my $padlen = 56 - length($intro);
+		while ($padlen > 0) {
+			$intro .= '	';
+			$padlen -= 8;
+		}
+
+		$total_size = 0;
 	}
 	elsif ($line =~ m/(.*):\s*file format/) {
 		$file = $1;
@@ -134,37 +151,18 @@ while (my $line = <STDIN>) {
 		}
 		next if ($size > 0x10000000);
 
-		next if $line !~ m/^($xs*)/;
-		my $addr = $1;
-		$addr =~ s/ /0/g;
-		$addr = "0x$addr";
-
-		my $intro = "$addr $func [$file]:";
-		my $padlen = 56 - length($intro);
-		while ($padlen > 0) {
-			$intro .= '	';
-			$padlen -= 8;
-		}
-		next if ($size < 100);
-		push @stack, "$intro$size\n";
+		$total_size = $total_size + $size
 	}
 	elsif (defined $dre && $line =~ m/$dre/) {
-		my $size = "Dynamic ($1)";
-
-		next if $line !~ m/^($xs*)/;
-		my $addr = $1;
-		$addr =~ s/ /0/g;
-		$addr = "0x$addr";
+		my $size = $1;
 
-		my $intro = "$addr $func [$file]:";
-		my $padlen = 56 - length($intro);
-		while ($padlen > 0) {
-			$intro .= '	';
-			$padlen -= 8;
-		}
-		push @stack, "$intro$size\n";
+		$size = hex($size) if ($size =~ /^0x/);
+		$total_size = $total_size + $size
 	}
 }
+if ($total_size > 100) {
+	push @stack, "$intro$total_size\n";
+}
 
 # Sort output by size (last field)
 print sort { ($b =~ /:\t*(\d+)$/)[0] <=> ($a =~ /:\t*(\d+)$/)[0] } @stack;
-- 
1.9.1

[PATCH 2/4] scripts/checkstack.pl: Add argument to print stacks greather than value.

[Maninder Singh]

Add arguments support to print stacks which are greater than
argument value only.

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 scripts/checkstack.pl | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/scripts/checkstack.pl b/scripts/checkstack.pl
index 412c459..8e5ef98 100755
--- a/scripts/checkstack.pl
+++ b/scripts/checkstack.pl
@@ -35,7 +35,7 @@ use strict;
 # $1 (first bracket) matches the dynamic amount of the stack growth
 #
 # use anything else and feel the pain ;)
-my (@stack, $re, $dre, $x, $xs, $funcre);
+my (@stack, $re, $dre, $x, $xs, $funcre, $min_stack);
 {
 	my $arch = shift;
 	if ($arch eq "") {
@@ -43,6 +43,11 @@ my (@stack, $re, $dre, $x, $xs, $funcre);
 		chomp($arch);
 	}
 
+	$min_stack = shift;
+	if ($min_stack eq "" || $min_stack !~ /^\d+$/) {
+		$min_stack = 100;
+	}
+
 	$x	= "[0-9a-f]";	# hex character
 	$xs	= "[0-9a-f ]";	# hex character or space
 	$funcre = qr/^$x* <(.*)>:$/;
@@ -113,7 +118,7 @@ my ($func, $file, $lastslash, $total_size, $addr, $intro);
 
 while (my $line = <STDIN>) {
 	if ($line =~ m/$funcre/) {
-		if ($total_size > 100) {
+		if ($total_size > $min_stack) {
 			push @stack, "$intro$total_size\n";
 		}
 
@@ -150,7 +155,6 @@ while (my $line = <STDIN>) {
 			$size += 0x80000000;
 		}
 		next if ($size > 0x10000000);
-
 		$total_size = $total_size + $size
 	}
 	elsif (defined $dre && $line =~ m/$dre/) {
@@ -160,7 +164,7 @@ while (my $line = <STDIN>) {
 		$total_size = $total_size + $size
 	}
 }
-if ($total_size > 100) {
+if ($total_size > $min_stack) {
 	push @stack, "$intro$total_size\n";
 }
 
-- 
1.9.1

Re: [PATCH 2/4] scripts/checkstack.pl: Add argument to print stacks greather than value.

[Masahiro Yamada]

On Thu, Apr 30, 2020 at 9:50 PM Maninder Singh <maninder1.s@samsung.com> wrote:
>
> Add arguments support to print stacks which are greater than
> argument value only.
>
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---
>  scripts/checkstack.pl | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/checkstack.pl b/scripts/checkstack.pl
> index 412c459..8e5ef98 100755
> --- a/scripts/checkstack.pl
> +++ b/scripts/checkstack.pl
> @@ -35,7 +35,7 @@ use strict;
>  # $1 (first bracket) matches the dynamic amount of the stack growth
>  #
>  # use anything else and feel the pain ;)
> -my (@stack, $re, $dre, $x, $xs, $funcre);
> +my (@stack, $re, $dre, $x, $xs, $funcre, $min_stack);
>  {
>         my $arch = shift;
>         if ($arch eq "") {
> @@ -43,6 +43,11 @@ my (@stack, $re, $dre, $x, $xs, $funcre);
>                 chomp($arch);
>         }
>
> +       $min_stack = shift;
> +       if ($min_stack eq "" || $min_stack !~ /^\d+$/) {
> +               $min_stack = 100;
> +       }
> +
>         $x      = "[0-9a-f]";   # hex character
>         $xs     = "[0-9a-f ]";  # hex character or space
>         $funcre = qr/^$x* <(.*)>:$/;
> @@ -113,7 +118,7 @@ my ($func, $file, $lastslash, $total_size, $addr, $intro);
>
>  while (my $line = <STDIN>) {
>         if ($line =~ m/$funcre/) {
> -               if ($total_size > 100) {
> +               if ($total_size > $min_stack) {
>                         push @stack, "$intro$total_size\n";
>                 }
>
> @@ -150,7 +155,6 @@ while (my $line = <STDIN>) {
>                         $size += 0x80000000;
>                 }
>                 next if ($size > 0x10000000);
> -


This is a noise change.

You can do this in 1/4 if you want to.



>                 $total_size = $total_size + $size
>         }
>         elsif (defined $dre && $line =~ m/$dre/) {
> @@ -160,7 +164,7 @@ while (my $line = <STDIN>) {
>                 $total_size = $total_size + $size
>         }
>  }
> -if ($total_size > 100) {
> +if ($total_size > $min_stack) {
>         push @stack, "$intro$total_size\n";
>  }
>
> --
> 1.9.1
>


--
Best Regards

Masahiro Yamada

[PATCH 3/4] scripts/checkstack.pl: add arm push handling for stack usage

[Maninder Singh]

To count stack usage of push {*, fp, ip, lr, pc} instruction in ARM,
if FRAME POINTER is enabled.
e.g. c01f0d48: e92ddff0 push {r4, r5, r6, r7, r8, r9, sl, fp, ip, lr, pc}

c01f0d50 <Y>:
c01f0d44:       e1a0c00d        mov     ip, sp
c01f0d48:       e92ddff0        push    {r4, r5, r6, r7, r8, r9, sl, fp, ip, lr, pc}
c01f0d4c:       e24cb004        sub     fp, ip, #4
c01f0d50:       e24dd094        sub     sp, sp, #448    ; 0x1C0 

$ cat dump | scripts/checkstack.pl arm
0xc01f0d50 Y []:                                        448

added subroutine frame work for this.
After change:
0xc01f0d500 Y []:                                       492

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 scripts/checkstack.pl | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/scripts/checkstack.pl b/scripts/checkstack.pl
index 8e5ef98..b292ef4 100755
--- a/scripts/checkstack.pl
+++ b/scripts/checkstack.pl
@@ -34,8 +34,10 @@ use strict;
 # $& (whole re) matches the complete objdump line with the stack growth
 # $1 (first bracket) matches the dynamic amount of the stack growth
 #
+# $sub: subroutine for special handling to check stack usage.
+#
 # use anything else and feel the pain ;)
-my (@stack, $re, $dre, $x, $xs, $funcre, $min_stack);
+my (@stack, $re, $dre, $sub, $x, $xs, $funcre, $min_stack);
 {
 	my $arch = shift;
 	if ($arch eq "") {
@@ -59,6 +61,7 @@ my (@stack, $re, $dre, $x, $xs, $funcre, $min_stack);
 	} elsif ($arch eq 'arm') {
 		#c0008ffc:	e24dd064	sub	sp, sp, #100	; 0x64
 		$re = qr/.*sub.*sp, sp, #(([0-9]{2}|[3-9])[0-9]{2})/o;
+		$sub = \&arm_push_handling;
 	} elsif ($arch =~ /^x86(_64)?$/ || $arch =~ /^i[3456]86$/) {
 		#c0105234:       81 ec ac 05 00 00       sub    $0x5ac,%esp
 		# or
@@ -112,6 +115,24 @@ my (@stack, $re, $dre, $x, $xs, $funcre, $min_stack);
 }
 
 #
+# To count stack usage of push {*, fp, ip, lr, pc} instruction in ARM,
+# if FRAME POINTER is enabled.
+# e.g. c01f0d48: e92ddff0 push {r4, r5, r6, r7, r8, r9, sl, fp, ip, lr, pc}
+#
+sub arm_push_handling {
+	my $regex = qr/.*push.*fp, ip, lr, pc}/o;
+	my $size = 0;
+	my $line_arg = shift;
+
+	if ($line_arg =~ m/$regex/) {
+		$size = $line_arg =~ tr/,//;
+		$size = ($size + 1) * 4;
+	}
+
+	return $size;
+}
+
+#
 # main()
 #
 my ($func, $file, $lastslash, $total_size, $addr, $intro);
@@ -163,6 +184,10 @@ while (my $line = <STDIN>) {
 		$size = hex($size) if ($size =~ /^0x/);
 		$total_size = $total_size + $size
 	}
+	elsif (defined $sub) {
+		my $size = &$sub($line);
+		$total_size = $total_size + $size;
+	}
 }
 if ($total_size > $min_stack) {
 	push @stack, "$intro$total_size\n";
-- 
1.9.1

Re: [PATCH 3/4] scripts/checkstack.pl: add arm push handling for stack usage

[Masahiro Yamada]

(+CC Andi Kleen, and more mentor)


On Thu, Apr 30, 2020 at 9:50 PM Maninder Singh <maninder1.s@samsung.com> wrote:
>
> To count stack usage of push {*, fp, ip, lr, pc} instruction in ARM,
> if FRAME POINTER is enabled.
> e.g. c01f0d48: e92ddff0 push {r4, r5, r6, r7, r8, r9, sl, fp, ip, lr, pc}
>
> c01f0d50 <Y>:
> c01f0d44:       e1a0c00d        mov     ip, sp
> c01f0d48:       e92ddff0        push    {r4, r5, r6, r7, r8, r9, sl, fp, ip, lr, pc}
> c01f0d4c:       e24cb004        sub     fp, ip, #4
> c01f0d50:       e24dd094        sub     sp, sp, #448    ; 0x1C0
>
> $ cat dump | scripts/checkstack.pl arm
> 0xc01f0d50 Y []:                                        448
>
> added subroutine frame work for this.
> After change:
> 0xc01f0d500 Y []:                                       492
>
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>



I do not mean to block this patch, but
some people still invest efforts to improve this script.
So, please let me ask this question.

Do you know CONFIG_FRAME_WARN?

Commit 35bb5b1e0e84cfa1a8906f7e6a77f391ff315791
mentioned -Wframe-larger-than should obsolete
make checkstack.

Now, -Wframe-larger-than is supported by
both minimal version of GCC and Clang.

I know checkstack.pl dumps the stack size
of functions, which is different from what
-Wframe-larger-than does, but the goal is
quite similar, I think.

I just wondered if we need both.




> ---
>  scripts/checkstack.pl | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/checkstack.pl b/scripts/checkstack.pl
> index 8e5ef98..b292ef4 100755
> --- a/scripts/checkstack.pl
> +++ b/scripts/checkstack.pl
> @@ -34,8 +34,10 @@ use strict;
>  # $& (whole re) matches the complete objdump line with the stack growth
>  # $1 (first bracket) matches the dynamic amount of the stack growth
>  #
> +# $sub: subroutine for special handling to check stack usage.
> +#
>  # use anything else and feel the pain ;)
> -my (@stack, $re, $dre, $x, $xs, $funcre, $min_stack);
> +my (@stack, $re, $dre, $sub, $x, $xs, $funcre, $min_stack);
>  {
>         my $arch = shift;
>         if ($arch eq "") {
> @@ -59,6 +61,7 @@ my (@stack, $re, $dre, $x, $xs, $funcre, $min_stack);
>         } elsif ($arch eq 'arm') {
>                 #c0008ffc:      e24dd064        sub     sp, sp, #100    ; 0x64
>                 $re = qr/.*sub.*sp, sp, #(([0-9]{2}|[3-9])[0-9]{2})/o;
> +               $sub = \&arm_push_handling;
>         } elsif ($arch =~ /^x86(_64)?$/ || $arch =~ /^i[3456]86$/) {
>                 #c0105234:       81 ec ac 05 00 00       sub    $0x5ac,%esp
>                 # or
> @@ -112,6 +115,24 @@ my (@stack, $re, $dre, $x, $xs, $funcre, $min_stack);
>  }
>
>  #
> +# To count stack usage of push {*, fp, ip, lr, pc} instruction in ARM,
> +# if FRAME POINTER is enabled.
> +# e.g. c01f0d48: e92ddff0 push {r4, r5, r6, r7, r8, r9, sl, fp, ip, lr, pc}
> +#
> +sub arm_push_handling {
> +       my $regex = qr/.*push.*fp, ip, lr, pc}/o;
> +       my $size = 0;
> +       my $line_arg = shift;
> +
> +       if ($line_arg =~ m/$regex/) {
> +               $size = $line_arg =~ tr/,//;
> +               $size = ($size + 1) * 4;
> +       }
> +
> +       return $size;
> +}
> +
> +#
>  # main()
>  #
>  my ($func, $file, $lastslash, $total_size, $addr, $intro);
> @@ -163,6 +184,10 @@ while (my $line = <STDIN>) {
>                 $size = hex($size) if ($size =~ /^0x/);
>                 $total_size = $total_size + $size
>         }
> +       elsif (defined $sub) {
> +               my $size = &$sub($line);
> +               $total_size = $total_size + $size;


                  $total_size += $size;

           Or

                  $total_size += &$sub($line);

                   then, delete $size.



> +       }
>  }
>  if ($total_size > $min_stack) {
>         push @stack, "$intro$total_size\n";
> --
> 1.9.1
>


-- 
Best Regards
Masahiro Yamada

RE:(2) [PATCH 3/4] scripts/checkstack.pl: add arm push handling for stack usage

[Vaneet Narang]

Hi Masahiro, 

>> To count stack usage of push {*, fp, ip, lr, pc} instruction in ARM,
>> if FRAME POINTER is enabled.
>> e.g. c01f0d48: e92ddff0 push {r4, r5, r6, r7, r8, r9, sl, fp, ip, lr, pc}
>>
>> c01f0d50 <Y>:
>> c01f0d44:       e1a0c00d        mov     ip, sp
>> c01f0d48:       e92ddff0        push    {r4, r5, r6, r7, r8, r9, sl, fp, ip, lr, pc}
>> c01f0d4c:       e24cb004        sub     fp, ip, #4
>> c01f0d50:       e24dd094        sub     sp, sp, #448    ; 0x1C0
>>
>> $ cat dump | scripts/checkstack.pl arm
>> 0xc01f0d50 Y []:                                        448
>>
>> added subroutine frame work for this.
>> After change:
>> 0xc01f0d500 Y []:                                       492
  
 
> Do you know CONFIG_FRAME_WARN?
 Yes we know this and we use it to get compilation error if some function is using more stack.
This config will report issue at compilation.
 
>I know checkstack.pl dumps the stack size
>of functions, which is different from what
>-Wframe-larger-than does, but the goal is
>quite similar, I think.
> 
>I just wondered if we need both.
 
We feel purpose of this patch is different from CONFIG_FRAME_WARN.
This patch is specific to ARM and fixes bug in stack usage calculation.

We were comparing stack usage of ARM with ARM64 and found big gap.
We realised ARM is not calculating stack usage properly.
It only considers stack used by local variables but it doesn't consider 
stack used to store register context at the start of functions. 
This is not the case with ARM64. It seems ARM64 considers both.

We found even stack variables are of same size on both target but 
arm64 stack usage is high.

Considering below assembly, Actual stack usage is 492 but current script reports 448.
push instruction uses 44 bytes of stack to take backup of registers as per ARM calling
convention.

c01f0d44:       e1a0c00d        mov     ip, sp
c01f0d48:       e92ddff0        push    {r4, r5, r6, r7, r8, r9, sl, fp, ip, lr, pc}
c01f0d4c:       e24cb004        sub     fp, ip, #4
c01f0d50:       e24dd094        sub     sp, sp, #448    ; 0x1C0

Thanks & Regards,
Vaneet Narang
 
  

[PATCH 4/4] scripts/checkstack.pl: fix arm sp regex

[Maninder Singh]

if objdump has below entries;
c01ed608 <X>:
c01ed614:       e24ddff7        sub     sp, sp, #120    ; 0x78

c01f0d50 <Y>:
c01f0d50:       e24dd094        sub     sp, sp, #140    ; 0x8c

scripts fails to read stack usage.
so making regex $re for ARM similar to aarch64

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 scripts/checkstack.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkstack.pl b/scripts/checkstack.pl
index b292ef4..e80de70 100755
--- a/scripts/checkstack.pl
+++ b/scripts/checkstack.pl
@@ -60,7 +60,7 @@ my (@stack, $re, $dre, $sub, $x, $xs, $funcre, $min_stack);
 		$dre = qr/^.*sub.*sp, sp, #(0x$x{1,8})/o;
 	} elsif ($arch eq 'arm') {
 		#c0008ffc:	e24dd064	sub	sp, sp, #100	; 0x64
-		$re = qr/.*sub.*sp, sp, #(([0-9]{2}|[3-9])[0-9]{2})/o;
+		$re = qr/.*sub.*sp, sp, #([0-9]{1,4})/o;
 		$sub = \&arm_push_handling;
 	} elsif ($arch =~ /^x86(_64)?$/ || $arch =~ /^i[3456]86$/) {
 		#c0105234:       81 ec ac 05 00 00       sub    $0x5ac,%esp
-- 
1.9.1

Re: [PATCH 4/4] scripts/checkstack.pl: fix arm sp regex

[Masahiro Yamada]

On Thu, Apr 30, 2020 at 9:50 PM Maninder Singh <maninder1.s@samsung.com> wrote:
>
> if objdump has below entries;
> c01ed608 <X>:
> c01ed614:       e24ddff7        sub     sp, sp, #120    ; 0x78
>
> c01f0d50 <Y>:
> c01f0d50:       e24dd094        sub     sp, sp, #140    ; 0x8c
>
> scripts fails to read stack usage.
> so making regex $re for ARM similar to aarch64
>
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>


This looks good to me, and it is a bug fix.

Just a question about the SOB.


Maninder Singh is the author and also the submitter, right?

What does "Signed-off-by: Vaneet Narang <v.narang@samsung.com>" mean?

Co-developed-by or something else?






> ---
>  scripts/checkstack.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkstack.pl b/scripts/checkstack.pl
> index b292ef4..e80de70 100755
> --- a/scripts/checkstack.pl
> +++ b/scripts/checkstack.pl
> @@ -60,7 +60,7 @@ my (@stack, $re, $dre, $sub, $x, $xs, $funcre, $min_stack);
>                 $dre = qr/^.*sub.*sp, sp, #(0x$x{1,8})/o;
>         } elsif ($arch eq 'arm') {
>                 #c0008ffc:      e24dd064        sub     sp, sp, #100    ; 0x64
> -               $re = qr/.*sub.*sp, sp, #(([0-9]{2}|[3-9])[0-9]{2})/o;
> +               $re = qr/.*sub.*sp, sp, #([0-9]{1,4})/o;
>                 $sub = \&arm_push_handling;
>         } elsif ($arch =~ /^x86(_64)?$/ || $arch =~ /^i[3456]86$/) {
>                 #c0105234:       81 ec ac 05 00 00       sub    $0x5ac,%esp
> --
> 1.9.1
>


-- 
Best Regards
Masahiro Yamada

RE:[PATCH 4/4] scripts/checkstack.pl: fix arm sp regex

[Maninder Singh]

Hi Masahiro,

Thanks for review.
We will integrate your review comements and send v2.

>>

>> so making regex $re for ARM similar to aarch64
>>
>> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
>> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> 
> 
>This looks good to me, and it is a bug fix.
> 
>Just a question about the SOB.
> 
> 
>Maninder Singh is the author and also the submitter, right?
> 
>What does "Signed-off-by: Vaneet Narang <v.narang@samsung.com>" mean?
> 
>Co-developed-by or something else?
 
Yes All 4 patches are co-developed by Vaneet and me.
we were checking stack usage for arm and AARCH64 and found
some changes in scrips which can help others also.

Thanks,
Maninder Singh

Re: [PATCH 4/4] scripts/checkstack.pl: fix arm sp regex

[Masahiro Yamada]

On Thu, May 7, 2020 at 8:43 PM Maninder Singh <maninder1.s@samsung.com> wrote:
>
> Hi Masahiro,
>
> Thanks for review.
> We will integrate your review comements and send v2.
>
> >>
>
> >> so making regex $re for ARM similar to aarch64
> >>
> >> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> >> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> >
> >
> >This looks good to me, and it is a bug fix.
> >
> >Just a question about the SOB.
> >
> >
> >Maninder Singh is the author and also the submitter, right?
> >
> >What does "Signed-off-by: Vaneet Narang <v.narang@samsung.com>" mean?
> >
> >Co-developed-by or something else?
>
> Yes All 4 patches are co-developed by Vaneet and me.
> we were checking stack usage for arm and AARCH64 and found
> some changes in scrips which can help others also.
>

OK. Then,

Co-developed-by: Vaneet Narang <v.narang@samsung.com>

is the correct tag.


Signed-off-by is added when the patch is forwarded by
somebody else. This is not the case.







-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/4] scripts/checkstack.pl: don't display $dre as different entity

[Masahiro Yamada]

On Thu, Apr 30, 2020 at 9:50 PM Maninder Singh <maninder1.s@samsung.com> wrote:
>
> currnetly script prints stack usage for functions
> in two ways:($re and $dre)
>
> dre breaks sorting mechanism.
> 0xffffa00011f26f88 sunxi_mux_clk_setup.isra.0 [vmlinux]:Dynamic (0x140)
> ..
> 0xffffa00011f27210 sunxi_divs_clk_setup [vmlinux]:      Dynamic (0x1d0)
>
> so we can print it in decimal only.
>
> Also address before function name is changed to function
> start address rather than stack consumption address.
> Because in next patch, arm has two ways to use stack
> which can be clubbed and printed in one function only.
>
> All symbols whose stack by adding(re and dre) is greater than
> 100, will be printed.
>
> 0xffffa00011f2720c0 sunxi_divs_clk_setup [vmlinux]:     464
> ...
> 0xffffa00011f26f840 sunxi_mux_clk_setup.isra.0 [vmlinux]:320
>
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---
>  scripts/checkstack.pl | 52 +++++++++++++++++++++++++--------------------------
>  1 file changed, 25 insertions(+), 27 deletions(-)
>
> diff --git a/scripts/checkstack.pl b/scripts/checkstack.pl
> index 371bd17..412c459 100755
> --- a/scripts/checkstack.pl
> +++ b/scripts/checkstack.pl
> @@ -109,11 +109,28 @@ my (@stack, $re, $dre, $x, $xs, $funcre);
>  #
>  # main()
>  #
> -my ($func, $file, $lastslash);
> +my ($func, $file, $lastslash, $total_size, $addr, $intro);


$total_size is undefined for the first function.
I think 0 is implied, but is it clearer to initialize it here?

$total_size = 0;



>  while (my $line = <STDIN>) {
>         if ($line =~ m/$funcre/) {
> +               if ($total_size > 100) {
> +                       push @stack, "$intro$total_size\n";
> +               }
> +
>                 $func = $1;
> +               next if $line !~ m/^($xs*)/;

Hmm, I think this 'next' is unlikely to happen.
But, it happened, the same line would be pushed twice.

Maybe, is it better to move 'next it' above
the 'if ($total_size > 100)' check?




> +               $addr = $1;
> +               $addr =~ s/ /0/g;
> +               $addr = "0x$addr";
> +
> +               $intro = "$addr $func [$file]:";
> +               my $padlen = 56 - length($intro);
> +               while ($padlen > 0) {
> +                       $intro .= '     ';
> +                       $padlen -= 8;
> +               }
> +
> +               $total_size = 0;
>         }
>         elsif ($line =~ m/(.*):\s*file format/) {
>                 $file = $1;
> @@ -134,37 +151,18 @@ while (my $line = <STDIN>) {
>                 }
>                 next if ($size > 0x10000000);
>
> -               next if $line !~ m/^($xs*)/;
> -               my $addr = $1;
> -               $addr =~ s/ /0/g;
> -               $addr = "0x$addr";
> -
> -               my $intro = "$addr $func [$file]:";
> -               my $padlen = 56 - length($intro);
> -               while ($padlen > 0) {
> -                       $intro .= '     ';
> -                       $padlen -= 8;
> -               }
> -               next if ($size < 100);
> -               push @stack, "$intro$size\n";
> +               $total_size = $total_size + $size


For consistency, I personally prefer adding ';'
to every statement even for the last one in the block...


Is this simpler ?

                  $total_size += $size;






>         }
>         elsif (defined $dre && $line =~ m/$dre/) {
> -               my $size = "Dynamic ($1)";
> -
> -               next if $line !~ m/^($xs*)/;
> -               my $addr = $1;
> -               $addr =~ s/ /0/g;
> -               $addr = "0x$addr";
> +               my $size = $1;
>
> -               my $intro = "$addr $func [$file]:";
> -               my $padlen = 56 - length($intro);
> -               while ($padlen > 0) {
> -                       $intro .= '     ';
> -                       $padlen -= 8;
> -               }
> -               push @stack, "$intro$size\n";
> +               $size = hex($size) if ($size =~ /^0x/);
> +               $total_size = $total_size + $size



Ditto. How about this?

                  $total_size += $size;


>         }
>  }
> +if ($total_size > 100) {
> +       push @stack, "$intro$total_size\n";
> +}
>
>  # Sort output by size (last field)
>  print sort { ($b =~ /:\t*(\d+)$/)[0] <=> ($a =~ /:\t*(\d+)$/)[0] } @stack;
> --
> 1.9.1
>


--
Best Regards
Masahiro Yamada