[PATCH] recordmcount.pl: look for jgnop instruction as well as bcrl on s390

[PATCH] recordmcount.pl: look for jgnop instruction as well as bcrl on s390

[Jerome Marchand]

On s390, recordmcount.pl is looking for "bcrl 0,<xxx>" instructions in
the objdump -d outpout. However since binutils 2.37, objdump -d
display "jgnop <xxx>" for the same instruction. Update the
mcount_regex so that it accepts both.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 scripts/recordmcount.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 7d631aaa0ae1..52a000b057a5 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -219,7 +219,7 @@ if ($arch eq "x86_64") {
 
 } elsif ($arch eq "s390" && $bits == 64) {
     if ($cc =~ /-DCC_USING_HOTPATCH/) {
-	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*brcl\\s*0,[0-9a-f]+ <([^\+]*)>\$";
+	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*(bcrl\\s*0,|jgnop\\s*)[0-9a-f]+ <([^\+]*)>\$";
 	$mcount_adjust = 0;
     }
     $alignment = 8;
-- 
2.31.1

Re: [PATCH] recordmcount.pl: look for jgnop instruction as well as bcrl on s390

[Miroslav Benes]

On Fri, 10 Dec 2021, Jerome Marchand wrote:

> On s390, recordmcount.pl is looking for "bcrl 0,<xxx>" instructions in
> the objdump -d outpout. However since binutils 2.37, objdump -d
> display "jgnop <xxx>" for the same instruction. Update the
> mcount_regex so that it accepts both.
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>

Yes, we ran into exactly this issue too...

> ---
>  scripts/recordmcount.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> index 7d631aaa0ae1..52a000b057a5 100755
> --- a/scripts/recordmcount.pl
> +++ b/scripts/recordmcount.pl
> @@ -219,7 +219,7 @@ if ($arch eq "x86_64") {
>  
>  } elsif ($arch eq "s390" && $bits == 64) {
>      if ($cc =~ /-DCC_USING_HOTPATCH/) {
> -	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*brcl\\s*0,[0-9a-f]+ <([^\+]*)>\$";
> +	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*(bcrl\\s*0,|jgnop\\s*)[0-9a-f]+ <([^\+]*)>\$";
>  	$mcount_adjust = 0;
>      }
>      $alignment = 8;

...and we have exactly the same fix in SLES. I haven't got to submit it 
to upstream yet :(, many thanks for doing it.

So at least

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

M

Re: [PATCH] recordmcount.pl: look for jgnop instruction as well as bcrl on s390

[Heiko Carstens]

On Fri, Dec 10, 2021 at 10:57:00AM +0100, Miroslav Benes wrote:
> On Fri, 10 Dec 2021, Jerome Marchand wrote:
> 
> > On s390, recordmcount.pl is looking for "bcrl 0,<xxx>" instructions in
> > the objdump -d outpout. However since binutils 2.37, objdump -d
> > display "jgnop <xxx>" for the same instruction. Update the
> > mcount_regex so that it accepts both.
> > 
> > Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> 
> Yes, we ran into exactly this issue too...
> 
> > ---
> >  scripts/recordmcount.pl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> > index 7d631aaa0ae1..52a000b057a5 100755
> > --- a/scripts/recordmcount.pl
> > +++ b/scripts/recordmcount.pl
> > @@ -219,7 +219,7 @@ if ($arch eq "x86_64") {
> >  
> >  } elsif ($arch eq "s390" && $bits == 64) {
> >      if ($cc =~ /-DCC_USING_HOTPATCH/) {
> > -	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*brcl\\s*0,[0-9a-f]+ <([^\+]*)>\$";
> > +	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*(bcrl\\s*0,|jgnop\\s*)[0-9a-f]+ <([^\+]*)>\$";
> >  	$mcount_adjust = 0;
> >      }
> >      $alignment = 8;
> 
> ...and we have exactly the same fix in SLES. I haven't got to submit it 
> to upstream yet :(, many thanks for doing it.
> 
> So at least
> 
> Reviewed-by: Miroslav Benes <mbenes@suse.cz>

Just out of curiosity: am I right if I assume that both of you have
kernel sources without upstream commit
d983c89cc96a ("s390/ftrace: Add -mfentry and -mnop-mcount support")
and the commits directly preceding that one?

Otherwise I would be surprised that this would make any difference.

Applied to s390 tree + added a stable tag.

Thank you!

Re: [PATCH] recordmcount.pl: look for jgnop instruction as well as bcrl on s390

[Miroslav Benes]

On Fri, 10 Dec 2021, Heiko Carstens wrote:

> On Fri, Dec 10, 2021 at 10:57:00AM +0100, Miroslav Benes wrote:
> > On Fri, 10 Dec 2021, Jerome Marchand wrote:
> > 
> > > On s390, recordmcount.pl is looking for "bcrl 0,<xxx>" instructions in
> > > the objdump -d outpout. However since binutils 2.37, objdump -d
> > > display "jgnop <xxx>" for the same instruction. Update the
> > > mcount_regex so that it accepts both.
> > > 
> > > Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> > 
> > Yes, we ran into exactly this issue too...
> > 
> > > ---
> > >  scripts/recordmcount.pl | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> > > index 7d631aaa0ae1..52a000b057a5 100755
> > > --- a/scripts/recordmcount.pl
> > > +++ b/scripts/recordmcount.pl
> > > @@ -219,7 +219,7 @@ if ($arch eq "x86_64") {
> > >  
> > >  } elsif ($arch eq "s390" && $bits == 64) {
> > >      if ($cc =~ /-DCC_USING_HOTPATCH/) {
> > > -	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*brcl\\s*0,[0-9a-f]+ <([^\+]*)>\$";
> > > +	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*(bcrl\\s*0,|jgnop\\s*)[0-9a-f]+ <([^\+]*)>\$";
> > >  	$mcount_adjust = 0;
> > >      }
> > >      $alignment = 8;
> > 
> > ...and we have exactly the same fix in SLES. I haven't got to submit it 
> > to upstream yet :(, many thanks for doing it.
> > 
> > So at least
> > 
> > Reviewed-by: Miroslav Benes <mbenes@suse.cz>
> 
> Just out of curiosity: am I right if I assume that both of you have
> kernel sources without upstream commit
> d983c89cc96a ("s390/ftrace: Add -mfentry and -mnop-mcount support")
> and the commits directly preceding that one?

Unfortunately, it was reported also on 5.3-based kernels which have the 
commits.
 
> Otherwise I would be surprised that this would make any difference.

How come? I mean, s390 does not define HAVE_C_RECORDMCOUNT which implies 
that recordmcount.pl is used (see scripts/Makefile.build and 
BUILD_C_RECORDMCOUNT definition in Makefile).

> Applied to s390 tree + added a stable tag.

Thanks.

Miroslav

Re: [PATCH] recordmcount.pl: look for jgnop instruction as well as bcrl on s390

[Miroslav Benes]

> > Otherwise I would be surprised that this would make any difference.
> 
> How come? I mean, s390 does not define HAVE_C_RECORDMCOUNT which implies 
> that recordmcount.pl is used (see scripts/Makefile.build and 
> BUILD_C_RECORDMCOUNT definition in Makefile).

Ah, sorry, that was too quick.

I guess you want to say that recordmcount is not used at all and GCC 
should do it. Because CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT is not set, 
since FTRACE_MCOUNT_USE_CC should be set.

What a maze.

But isn't -mrecord-mcount x86_64-only option?

Miroslav

Re: [PATCH] recordmcount.pl: look for jgnop instruction as well as bcrl on s390

[Heiko Carstens]

On Fri, Dec 10, 2021 at 01:31:23PM +0100, Miroslav Benes wrote:
> > > Otherwise I would be surprised that this would make any difference.
> > 
> > How come? I mean, s390 does not define HAVE_C_RECORDMCOUNT which implies 
> > that recordmcount.pl is used (see scripts/Makefile.build and 
> > BUILD_C_RECORDMCOUNT definition in Makefile).
> 
> Ah, sorry, that was too quick.
> 
> I guess you want to say that recordmcount is not used at all and GCC 
> should do it. Because CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT is not set, 
> since FTRACE_MCOUNT_USE_CC should be set.
> 
> What a maze.
> 
> But isn't -mrecord-mcount x86_64-only option?

No, it is (at least) also supported on s390. IIRC that was added with
gcc9. Anyway, this is not too important since the fix makes sense
anyway.

Re: [PATCH] recordmcount.pl: look for jgnop instruction as well as bcrl on s390

[Steven Rostedt]

On Fri, 10 Dec 2021 16:51:35 +0100
Heiko Carstens <hca@linux.ibm.com> wrote:

> On Fri, Dec 10, 2021 at 01:31:23PM +0100, Miroslav Benes wrote:
> > > > Otherwise I would be surprised that this would make any difference.  
> > > 
> > > How come? I mean, s390 does not define HAVE_C_RECORDMCOUNT which implies 
> > > that recordmcount.pl is used (see scripts/Makefile.build and 
> > > BUILD_C_RECORDMCOUNT definition in Makefile).  
> > 
> > Ah, sorry, that was too quick.
> > 
> > I guess you want to say that recordmcount is not used at all and GCC 
> > should do it. Because CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT is not set, 
> > since FTRACE_MCOUNT_USE_CC should be set.
> > 
> > What a maze.
> > 
> > But isn't -mrecord-mcount x86_64-only option?  
> 
> No, it is (at least) also supported on s390. IIRC that was added with
> gcc9. Anyway, this is not too important since the fix makes sense
> anyway.

Thanks everyone for fixing this.

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

Re: [PATCH] recordmcount.pl: look for jgnop instruction as well as bcrl on s390

[Jerome Marchand]

On 10/12/2021 11:58, Heiko Carstens wrote:
> On Fri, Dec 10, 2021 at 10:57:00AM +0100, Miroslav Benes wrote:
>> On Fri, 10 Dec 2021, Jerome Marchand wrote:
>>
>>> On s390, recordmcount.pl is looking for "bcrl 0,<xxx>" instructions in
>>> the objdump -d outpout. However since binutils 2.37, objdump -d
>>> display "jgnop <xxx>" for the same instruction. Update the
>>> mcount_regex so that it accepts both.
>>>
>>> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
>>
>> Yes, we ran into exactly this issue too...
>>
>>> ---
>>>   scripts/recordmcount.pl | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
>>> index 7d631aaa0ae1..52a000b057a5 100755
>>> --- a/scripts/recordmcount.pl
>>> +++ b/scripts/recordmcount.pl
>>> @@ -219,7 +219,7 @@ if ($arch eq "x86_64") {
>>>   
>>>   } elsif ($arch eq "s390" && $bits == 64) {
>>>       if ($cc =~ /-DCC_USING_HOTPATCH/) {
>>> -	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*brcl\\s*0,[0-9a-f]+ <([^\+]*)>\$";
>>> +	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*(bcrl\\s*0,|jgnop\\s*)[0-9a-f]+ <([^\+]*)>\$";
>>>   	$mcount_adjust = 0;
>>>       }
>>>       $alignment = 8;
>>
>> ...and we have exactly the same fix in SLES. I haven't got to submit it
>> to upstream yet :(, many thanks for doing it.
>>
>> So at least
>>
>> Reviewed-by: Miroslav Benes <mbenes@suse.cz>
> 
> Just out of curiosity: am I right if I assume that both of you have
> kernel sources without upstream commit
> d983c89cc96a ("s390/ftrace: Add -mfentry and -mnop-mcount support")
> and the commits directly preceding that one?

That is correct. As a matter of fact, the first thing I did to fix the 
issue was to backport it, but that didn't work since it requires gcc 9 
and that particular kernel is build with gcc 8.5.

Jerome

> 
> Otherwise I would be surprised that this would make any difference.
> 
> Applied to s390 tree + added a stable tag.
> 
> Thank you!
> 

Re: [PATCH] recordmcount.pl: look for jgnop instruction as well as bcrl on s390

[Miroslav Benes]

Hi,

On Fri, 10 Dec 2021, Jerome Marchand wrote:

> On s390, recordmcount.pl is looking for "bcrl 0,<xxx>" instructions in
> the objdump -d outpout. However since binutils 2.37, objdump -d
> display "jgnop <xxx>" for the same instruction. Update the
> mcount_regex so that it accepts both.
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  scripts/recordmcount.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> index 7d631aaa0ae1..52a000b057a5 100755
> --- a/scripts/recordmcount.pl
> +++ b/scripts/recordmcount.pl
> @@ -219,7 +219,7 @@ if ($arch eq "x86_64") {
>  
>  } elsif ($arch eq "s390" && $bits == 64) {
>      if ($cc =~ /-DCC_USING_HOTPATCH/) {
> -	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*brcl\\s*0,[0-9a-f]+ <([^\+]*)>\$";
> +	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*(bcrl\\s*0,|jgnop\\s*)[0-9a-f]+ <([^\+]*)>\$";

there is a typo I did not notice before. Sorry about that *sigh*.

s/bcrl/brcl/ on the whole patch.

Miroslav

Re: [PATCH] recordmcount.pl: look for jgnop instruction as well as bcrl on s390

[Heiko Carstens]

> > On s390, recordmcount.pl is looking for "bcrl 0,<xxx>" instructions in
> > the objdump -d outpout. However since binutils 2.37, objdump -d
> > display "jgnop <xxx>" for the same instruction. Update the
> > mcount_regex so that it accepts both.
> > 
> > Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> > ---
> >  scripts/recordmcount.pl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> > index 7d631aaa0ae1..52a000b057a5 100755
> > --- a/scripts/recordmcount.pl
> > +++ b/scripts/recordmcount.pl
> > @@ -219,7 +219,7 @@ if ($arch eq "x86_64") {
> >  
> >  } elsif ($arch eq "s390" && $bits == 64) {
> >      if ($cc =~ /-DCC_USING_HOTPATCH/) {
> > -	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*brcl\\s*0,[0-9a-f]+ <([^\+]*)>\$";
> > +	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*(bcrl\\s*0,|jgnop\\s*)[0-9a-f]+ <([^\+]*)>\$";
> 
> there is a typo I did not notice before. Sorry about that *sigh*.
> 
> s/bcrl/brcl/ on the whole patch.

Hm.. nice. So I will schedule the patch below for next week. Actually
tested with the variants I mentioned in the commit message.

From 30b3302ae3e9fb30c4cef5c17a2a63ba6ba34195 Mon Sep 17 00:00:00 2001
From: Heiko Carstens <hca@linux.ibm.com>
Date: Thu, 23 Dec 2021 17:43:14 +0100
Subject: [PATCH] recordmcount.pl: fix typo in s390 mcount regex

Commit 85bf17b28f97 ("recordmcount.pl: look for jgnop instruction as well
as bcrl on s390") added a new alternative mnemonic for the existing brcl
instruction. This is required for the combination old gcc version (pre 9.0)
and binutils since version 2.37.
However at the same time this commit introduced a typo, replacing brcl with
bcrl. As a result no mcount locations are detected anymore with old gcc
versions (pre 9.0) and binutils before version 2.37.
Fix this by using the correct mnemonic again.

Reported-by: Miroslav Benes <mbenes@suse.cz>
Cc: Jerome Marchand <jmarchan@redhat.com>
Cc: <stable@vger.kernel.org>
Fixes: 85bf17b28f97 ("recordmcount.pl: look for jgnop instruction as well as bcrl on s390")
Link: https://lore.kernel.org/r/alpine.LSU.2.21.2112230949520.19849@pobox.suse.cz
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 scripts/recordmcount.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 52a000b057a5..3ccb2c70add4 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -219,7 +219,7 @@ if ($arch eq "x86_64") {
 
 } elsif ($arch eq "s390" && $bits == 64) {
     if ($cc =~ /-DCC_USING_HOTPATCH/) {
-	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*(bcrl\\s*0,|jgnop\\s*)[0-9a-f]+ <([^\+]*)>\$";
+	$mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*c0 04 00 00 00 00\\s*(brcl\\s*0,|jgnop\\s*)[0-9a-f]+ <([^\+]*)>\$";
 	$mcount_adjust = 0;
     }
     $alignment = 8;
-- 
2.32.0