[PATCH] Aarch64-SIM: BLR opcode does not support XLR register properly.

[PATCH] Aarch64-SIM: BLR opcode does not support XLR register properly.

[Carlo Bramini]

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

After a suggestion received at your bugzilla, I'm posting here a patch. Detailed explanation can be found here:

https://sourceware.org/bugzilla/show_bug.cgi?id=25318

Thank you very much for your time and your support.
Sincerely.

[-- Attachment #2: sim_aarch64.patch --]
[-- Type: application/octet-stream, Size: 1337 bytes --]

diff --git a/sim/aarch64/simulator.c b/sim/aarch64/simulator.c
index 60972976fc..0b8dd8a146 100644
--- a/sim/aarch64/simulator.c
+++ b/sim/aarch64/simulator.c
@@ -1174,7 +1174,7 @@ ldrsw_abs (sim_cpu *cpu, uint32_t offset)
   val = aarch64_get_mem_s32 (cpu, aarch64_get_reg_u64 (cpu, rn, SP_OK)
 			     + SCALE (offset, 32));
   /* The target register may not be SP but the source may be.  */
-  return aarch64_set_reg_s64 (cpu, rt, NO_SP, val);
+  aarch64_set_reg_s64 (cpu, rt, NO_SP, val);
 }
 
 /* 64 bit load sign-extended 32 bit unscaled signed 9 bit
@@ -13437,13 +13437,13 @@ br (sim_cpu *cpu)
 static void
 blr (sim_cpu *cpu)
 {
-  unsigned rn = INSTR (9, 5);
+  uint64_t target = aarch64_get_reg_u64 (cpu, INSTR (9, 5), NO_SP);
 
   TRACE_DECODE (cpu, "emulated at line %d", __LINE__);
   /* The pseudo code in the spec says we update LR before fetching.
      the value from the rn.  */
   aarch64_save_LR (cpu);
-  aarch64_set_next_PC (cpu, aarch64_get_reg_u64 (cpu, rn, NO_SP));
+  aarch64_set_next_PC (cpu, target);
 
   if (TRACE_BRANCH_P (cpu))
     {
@@ -14222,7 +14222,8 @@ dexBr (sim_cpu *cpu)
   switch (group2)
     {
     case BR_IMM_000:
-      return dexBranchImmediate (cpu);
+      dexBranchImmediate (cpu);
+      return;
 
     case BR_IMMCMP_001:
       /* Compare has bit 25 clear while test has it set.  */

Re: [PATCH] Aarch64-SIM: BLR opcode does not support XLR register properly.

[Luis Machado]

Hi Carlo,

Thanks for the patch.

I'd include just the fix itself and not any other cosmetic changes to 
the code, like moving/removing return statements. The statements are 
useless, but it makes the patch cleaner that way.

It would also make it clear it is a reasonably obvious fix, which can be 
pushed without a FSF assignment. Though it would be nice to have one in 
place for further contributions.

Other than that, it is missing a ChangeLog entry, but it is not a big 
deal for this particular change as one can quickly write one.

Luis

On 2/5/20 8:21 AM, Carlo Bramini wrote:
> After a suggestion received at your bugzilla, I'm posting here a patch. Detailed explanation can be found here:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=25318
> 
> Thank you very much for your time and your support.
> Sincerely.
> 

Re: [PATCH] Aarch64-SIM: BLR opcode does not support XLR register properly.

[Carlo Bramini]

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

Hello,
thank you very much for your quick reply.
I made a new patch, which includes just the fix to BLR opcode. I'm also adding an entry for the ChangeLog file, as you requested.

Sincerely.

---

./ChangeLog:

2020-02-05  Carlo Bramini  <carlo_bramini@users.sourceforge.net>

* sim/aarch64/simulator.c: Fix BLR opcode for supporting XLR register as source operand.


---

> Il 5 febbraio 2020 alle 12.39 Luis Machado <luis.machado@linaro.org> ha scritto:
> 
> 
> Hi Carlo,
> 
> Thanks for the patch.
> 
> I'd include just the fix itself and not any other cosmetic changes to 
> the code, like moving/removing return statements. The statements are 
> useless, but it makes the patch cleaner that way.
> 
> It would also make it clear it is a reasonably obvious fix, which can be 
> pushed without a FSF assignment. Though it would be nice to have one in 
> place for further contributions.
> 
> Other than that, it is missing a ChangeLog entry, but it is not a big 
> deal for this particular change as one can quickly write one.
> 
> Luis
> 
> On 2/5/20 8:21 AM, Carlo Bramini wrote:
> > After a suggestion received at your bugzilla, I'm posting here a patch. Detailed explanation can be found here:
> > 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=25318
> > 
> > Thank you very much for your time and your support.
> > Sincerely.
> >

[-- Attachment #2: sim_aarch64.patch --]
[-- Type: application/octet-stream, Size: 669 bytes --]

diff --git a/sim/aarch64/simulator.c b/sim/aarch64/simulator.c
index 84919d6b1f..715a7f4b6b 100644
--- a/sim/aarch64/simulator.c
+++ b/sim/aarch64/simulator.c
@@ -13437,13 +13437,13 @@ br (sim_cpu *cpu)
 static void
 blr (sim_cpu *cpu)
 {
-  unsigned rn = INSTR (9, 5);
+  uint64_t target = aarch64_get_reg_u64 (cpu, INSTR (9, 5), NO_SP);
 
   TRACE_DECODE (cpu, "emulated at line %d", __LINE__);
   /* The pseudo code in the spec says we update LR before fetching.
      the value from the rn.  */
   aarch64_save_LR (cpu);
-  aarch64_set_next_PC (cpu, aarch64_get_reg_u64 (cpu, rn, NO_SP));
+  aarch64_set_next_PC (cpu, target);
 
   if (TRACE_BRANCH_P (cpu))
     {

Re: [PATCH] Aarch64-SIM: BLR opcode does not support XLR register properly.

[Carlo Bramini]

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

Hello,
please excuse me, I just discovered that there are multiple ChangeLog files and probably I have to provide an entry for the right one. Sorry for my mistake in previous message.

Sincerely.

---

./sim/aarch64/ChangeLog:

2020-02-05 Carlo Bramini <carlo_bramini@users.sourceforge.net>

* simulator.c: Fix BLR opcode for supporting XLR register as source operand.

---


> ---------- Messaggio originale ----------Da: Carlo Bramini <carlo.bramix@libero.it>A: gdb-patches@sourceware.org
> Data: 5 febbraio 2020 alle 18.14Oggetto: Re: [PATCH] Aarch64-SIM: BLR opcode does not support XLR register properly.
> Hello,thank you very much for your quick reply.I made a new patch, which includes just the fix to BLR opcode. I'm also adding an entry for the ChangeLog file, as you requested.
> Sincerely.
> ---
> ./ChangeLog:
> 2020-02-05 Carlo Bramini <carlo_bramini@users.sourceforge.net>
> * sim/aarch64/simulator.c: Fix BLR opcode for supporting XLR register as source operand.
> 
> ---
> > Il 5 febbraio 2020 alle 12.39 Luis Machado <luis.machado@linaro.org> ha scritto:
> > 
> > Hi Carlo,
> > Thanks for the patch.
> > I'd include just the fix itself and not any other cosmetic changes tothe code, like moving/removing return statements. The statements areuseless, but it makes the patch cleaner that way.
> > It would also make it clear it is a reasonably obvious fix, which can bepushed without a FSF assignment. Though it would be nice to have one inplace for further contributions.
> > Other than that, it is missing a ChangeLog entry, but it is not a bigdeal for this particular change as one can quickly write one.
> > Luis
> > On 2/5/20 8:21 AM, Carlo Bramini wrote:> After a suggestion received at your bugzilla, I'm posting here a patch. Detailed explanation can be found here:
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=25318
> > > 
> > > Thank you very much for your time and your support.Sincerely.

[-- Attachment #2: sim_aarch64.patch --]
[-- Type: application/octet-stream, Size: 669 bytes --]

diff --git a/sim/aarch64/simulator.c b/sim/aarch64/simulator.c
index 84919d6b1f..715a7f4b6b 100644
--- a/sim/aarch64/simulator.c
+++ b/sim/aarch64/simulator.c
@@ -13437,13 +13437,13 @@ br (sim_cpu *cpu)
 static void
 blr (sim_cpu *cpu)
 {
-  unsigned rn = INSTR (9, 5);
+  uint64_t target = aarch64_get_reg_u64 (cpu, INSTR (9, 5), NO_SP);
 
   TRACE_DECODE (cpu, "emulated at line %d", __LINE__);
   /* The pseudo code in the spec says we update LR before fetching.
      the value from the rn.  */
   aarch64_save_LR (cpu);
-  aarch64_set_next_PC (cpu, aarch64_get_reg_u64 (cpu, rn, NO_SP));
+  aarch64_set_next_PC (cpu, target);
 
   if (TRACE_BRANCH_P (cpu))
     {

Re: [PATCH] Aarch64-SIM: BLR opcode does not support XLR register properly.

[Andrew Burgess]

* Carlo Bramini <carlo.bramix@libero.it> [2020-02-05 18:25:36 +0100]:

> Hello, please excuse me, I just discovered that there are multiple
> ChangeLog files and probably I have to provide an entry for the
> right one. Sorry for my mistake in previous message.

Thank you for reporting this change, and putting a patch together.

I pushed this as it is such a small change, but if you wish to
contribute further changes you will probably need to completing a
copyright assignment[1].

A copy of what I pushed is included below.

Thanks,
Andrew

[1] https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment

---

commit 69b1ffdb01106ed84a41a80f6ad2d9c26c4f45a9
Author: Carlo Bramini <carlo_bramini@users.sourceforge.net>
Date:   Thu Feb 6 22:50:26 2020 +0000

    sim/aarch64: Fix register ordering bug in blr (PR sim/25318)
    
    A comment in the implementation of blr says:
    
      /* The pseudo code in the spec says we update LR before fetching.
         the value from the rn.  */
    
    With 'rn' being the register holding the destination address.
    
    This may have been true at one point, but the ISA manual now clearly
    shows the destination register being read before the link register is
    written.
    
    This commit updates the implementation of blr to match.
    
    sim/aarch64/ChangeLog:
    
            PR sim/25318
            * simulator.c (blr): Read destination register before calling
            aarch64_save_LR.
    
    Change-Id: Icb1c556064e3d9c807ac28440475caa205ab1064

diff --git a/sim/aarch64/simulator.c b/sim/aarch64/simulator.c
index 84919d6b1fc..5f16a69478c 100644
--- a/sim/aarch64/simulator.c
+++ b/sim/aarch64/simulator.c
@@ -13437,13 +13437,12 @@ br (sim_cpu *cpu)
 static void
 blr (sim_cpu *cpu)
 {
-  unsigned rn = INSTR (9, 5);
+  /* Ensure we read the destination before we write LR.  */
+  uint64_t target = aarch64_get_reg_u64 (cpu, INSTR (9, 5), NO_SP);
 
   TRACE_DECODE (cpu, "emulated at line %d", __LINE__);
-  /* The pseudo code in the spec says we update LR before fetching.
-     the value from the rn.  */
   aarch64_save_LR (cpu);
-  aarch64_set_next_PC (cpu, aarch64_get_reg_u64 (cpu, rn, NO_SP));
+  aarch64_set_next_PC (cpu, target);
 
   if (TRACE_BRANCH_P (cpu))
     {