Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CGME20200430124958epcas5p15ecc8e744ed0f78837a6d58274a5baf0@epcas5p1.samsung.com>
 <1588250972-8507-1-git-send-email-maninder1.s@samsung.com> <1588250972-8507-3-git-send-email-maninder1.s@samsung.com>
In-Reply-To: <1588250972-8507-3-git-send-email-maninder1.s@samsung.com>
From:   Masahiro Yamada <masahiroy@kernel.org>
Date:   Thu, 7 May 2020 19:35:29 +0900
Message-ID: <CAK7LNAQxOtjsrS6b8pF93=0yqnTJT6b7V2hDOWsYGH-njNcZdw@mail.gmail.com>
Subject: Re: [PATCH 3/4] scripts/checkstack.pl: add arm push handling for
 stack usage
To:     Maninder Singh <maninder1.s@samsung.com>
Cc:     "George G. Davis" <george_davis@mentor.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        a.sahrawat@samsung.com, Vaneet Narang <v.narang@samsung.com>,
        Arnd Bergmann <arnd@arndb.de>, Andi Kleen <ak@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

(+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