[PATCH] arc: Write correct "eret" value during register collection

[PATCH] arc: Write correct "eret" value during register collection

[Shahab Vahedi via Gdb-patches]

From: Shahab Vahedi <shahab@synopsys.com>

In collect_register() function of arc-linux-tdep.c, the "eret"
(exception return) register value was not being reported correctly.
This patch fixes that.

Background:
When asked for the "pc" value, we have to update the "eret" register
with GDB's STOP_PC.  The "eret" instructs the kernel code where to
jump back when an instruction has stopped due to a breakpoint.  This
is how collect_register() was doing so:

--------------8<--------------
  if (regnum == gdbarch_pc_regnum (gdbarch))
    regnum = ARC_ERET_REGNUM;
  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
-------------->8--------------

Root cause:
Although this is using the correct offset (ERET register's), it is also
changing the REGNUM itself.  Therefore, raw_collect (regnum, ...) is
not reading from "pc" anymore.

gdb/ChangeLog:

	* arc-linux-tdep.c (collect_register): Use "eret" value while
	still writing to "pc" register cache.
---
 gdb/arc-linux-tdep.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index 9ff5f1214a1..3530c7cbdd8 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -319,9 +319,13 @@ static void
 collect_register (const struct regcache *regcache, struct gdbarch *gdbarch,
 		  int regnum, gdb_byte *buf)
 {
+  int offset;
+
   /* Skip non-existing registers.  */
-  if ((arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER))
+  if (arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER)
     return;
+  else
+    offset = arc_linux_core_reg_offsets[ARC_ERET_REGNUM];
 
   /* The address where the execution has stopped is in pseudo-register
      STOP_PC.  However, when kernel code is returning from the exception,
@@ -332,8 +336,8 @@ collect_register (const struct regcache *regcache, struct gdbarch *gdbarch,
      the program will continue at the address after the current instruction.
      */
   if (regnum == gdbarch_pc_regnum (gdbarch))
-    regnum = ARC_ERET_REGNUM;
-  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
+    offset = arc_linux_core_reg_offsets[ARC_ERET_REGNUM];
+  regcache->raw_collect (regnum, buf + offset);
 }
 
 void
-- 
2.29.2

Re: [PATCH] arc: Write correct "eret" value during register collection

[Tom Tromey]

>>>>> "Shahab" == Shahab Vahedi via Gdb-patches <gdb-patches@sourceware.org> writes:

Shahab> 	* arc-linux-tdep.c (collect_register): Use "eret" value while
Shahab> 	still writing to "pc" register cache.

Shahab>  collect_register (const struct regcache *regcache, struct gdbarch *gdbarch,
Shahab>  		  int regnum, gdb_byte *buf)
Shahab>  {
Shahab> +  int offset;
Shahab> +
Shahab>    /* Skip non-existing registers.  */
Shahab> -  if ((arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER))
Shahab> +  if (arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER)
Shahab>      return;
Shahab> +  else
Shahab> +    offset = arc_linux_core_reg_offsets[ARC_ERET_REGNUM];

I would drop the "else" here.

I didn't understand this patch.  It unconditionally sets 'offset' to the
offset of ARC_ERET_REGNUM.  But surely that can't be correct?
The assignment above does this, and so does...

Shahab>    if (regnum == gdbarch_pc_regnum (gdbarch))
Shahab> -    regnum = ARC_ERET_REGNUM;
Shahab> -  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
Shahab> +    offset = arc_linux_core_reg_offsets[ARC_ERET_REGNUM];

... this one.

Tom

Re: [PATCH] arc: Write correct "eret" value during register collection

[Shahab Vahedi via Gdb-patches]

Hi Tom,

On 11/12/20 3:36 PM, Tom Tromey wrote:
>>>>>> "Shahab" == Shahab Vahedi via Gdb-patches <gdb-patches@sourceware.org> writes:
> Shahab>    /* Skip non-existing registers.  */
> Shahab> -  if ((arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER))
> Shahab> +  if (arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER)
> Shahab>      return;
> Shahab> +  else
> Shahab> +    offset = arc_linux_core_reg_offsets[ARC_ERET_REGNUM];
> 
> I would drop the "else" here.

done.

> 
> I didn't understand this patch.  It unconditionally sets 'offset' to the
> offset of ARC_ERET_REGNUM.  But surely that can't be correct?
> The assignment above does this, and so does...
> 
> Shahab>    if (regnum == gdbarch_pc_regnum (gdbarch))
> Shahab> -    regnum = ARC_ERET_REGNUM;
> Shahab> -  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
> Shahab> +    offset = arc_linux_core_reg_offsets[ARC_ERET_REGNUM];
> 
> ... this one.

You're absolutely correct.  That was a snafu on my side.  A second patch is submitted [1].

[1] [PATCH v2] arc: Write correct "eret" value during register collection
https://sourceware.org/pipermail/gdb-patches/2020-November/173212.html


Cheers,
Shahab

[PATCH v2] arc: Write correct "eret" value during register collection

[Shahab Vahedi via Gdb-patches]

From: Shahab Vahedi <shahab@synopsys.com>

In collect_register() function of arc-linux-tdep.c, the "eret"
(exception return) register value was not being reported correctly.
This patch fixes that.

Background:
When asked for the "pc" value, we have to update the "eret" register
with GDB's STOP_PC.  The "eret" instructs the kernel code where to
jump back when an instruction has stopped due to a breakpoint.  This
is how collect_register() was doing so:

--------------8<--------------
  if (regnum == gdbarch_pc_regnum (gdbarch))
    regnum = ARC_ERET_REGNUM;
  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
-------------->8--------------

Root cause:
Although this is using the correct offset (ERET register's), it is also
changing the REGNUM itself.  Therefore, raw_collect (regnum, ...) is
not reading from "pc" anymore.

v2:
- Fix a copy/paste issue as rightfully addressed by Tom [1].

[1]
https://sourceware.org/pipermail/gdb-patches/2020-November/173208.html

gdb/ChangeLog:

	* arc-linux-tdep.c (collect_register): Use "eret" value while
	still writing to "pc" register cache.
---
 gdb/arc-linux-tdep.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index 9ff5f1214a1..6e74bae8056 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -319,8 +319,10 @@ static void
 collect_register (const struct regcache *regcache, struct gdbarch *gdbarch,
 		  int regnum, gdb_byte *buf)
 {
+  int offset;
+
   /* Skip non-existing registers.  */
-  if ((arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER))
+  if (arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER)
     return;
 
   /* The address where the execution has stopped is in pseudo-register
@@ -332,8 +334,10 @@ collect_register (const struct regcache *regcache, struct gdbarch *gdbarch,
      the program will continue at the address after the current instruction.
      */
   if (regnum == gdbarch_pc_regnum (gdbarch))
-    regnum = ARC_ERET_REGNUM;
-  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
+    offset = arc_linux_core_reg_offsets[ARC_ERET_REGNUM];
+  else
+    offset = arc_linux_core_reg_offsets[regnum];
+  regcache->raw_collect (regnum, buf + offset);
 }
 
 void
-- 
2.29.2

Re: [PATCH v2] arc: Write correct "eret" value during register collection

[Tom Tromey]

Shahab> 	* arc-linux-tdep.c (collect_register): Use "eret" value while
Shahab> 	still writing to "pc" register cache.

Thank you, this is ok.

Tom

[PUSHED] arc: Write correct "eret" value during register collection

[Shahab Vahedi via Gdb-patches]

From: Shahab Vahedi <shahab@synopsys.com>

In collect_register() function of arc-linux-tdep.c, the "eret"
(exception return) register value was not being reported correctly.
This patch fixes that.

Background:
When asked for the "pc" value, we have to update the "eret" register
with GDB's STOP_PC.  The "eret" instructs the kernel code where to
jump back when an instruction has stopped due to a breakpoint.  This
is how collect_register() was doing so:

--------------8<--------------
  if (regnum == gdbarch_pc_regnum (gdbarch))
    regnum = ARC_ERET_REGNUM;
  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
-------------->8--------------

Root cause:
Although this is using the correct offset (ERET register's), it is also
changing the REGNUM itself.  Therefore, raw_collect (regnum, ...) is
not reading from "pc" anymore.

v2:
- Fix a copy/paste issue as rightfully addressed by Tom [1].

[1]
https://sourceware.org/pipermail/gdb-patches/2020-November/173208.html

gdb/ChangeLog:

	* arc-linux-tdep.c (collect_register): Populate "eret" by
	"pc" value from the regcache when asked for "pc" value.
---
 gdb/ChangeLog        |  5 +++++
 gdb/arc-linux-tdep.c | 10 +++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 038d45f4ec4..4d1f5241e8c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2020-11-12  Shahab Vahedi  <shahab@synopsys.com>
+
+	* arc-linux-tdep.c (collect_register): Populate "eret" by
+	"pc" value from the regcache when asked for "pc" value.
+
 2020-11-12  Tom Tromey  <tom@tromey.com>
 
 	PR rust/26799:
diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index 9ff5f1214a1..6e74bae8056 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -319,8 +319,10 @@ static void
 collect_register (const struct regcache *regcache, struct gdbarch *gdbarch,
 		  int regnum, gdb_byte *buf)
 {
+  int offset;
+
   /* Skip non-existing registers.  */
-  if ((arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER))
+  if (arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER)
     return;
 
   /* The address where the execution has stopped is in pseudo-register
@@ -332,8 +334,10 @@ collect_register (const struct regcache *regcache, struct gdbarch *gdbarch,
      the program will continue at the address after the current instruction.
      */
   if (regnum == gdbarch_pc_regnum (gdbarch))
-    regnum = ARC_ERET_REGNUM;
-  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
+    offset = arc_linux_core_reg_offsets[ARC_ERET_REGNUM];
+  else
+    offset = arc_linux_core_reg_offsets[regnum];
+  regcache->raw_collect (regnum, buf + offset);
 }
 
 void
-- 
2.29.2

[PUSHED gdb-10-branch] arc: Write correct "eret" value during register collection

[Shahab Vahedi via Gdb-patches]

From: Shahab Vahedi <shahab@synopsys.com>

In collect_register() function of arc-linux-tdep.c, the "eret"
(exception return) register value was not being reported correctly.
This patch fixes that.

Background:
When asked for the "pc" value, we have to update the "eret" register
with GDB's STOP_PC.  The "eret" instructs the kernel code where to
jump back when an instruction has stopped due to a breakpoint.  This
is how collect_register() was doing so:

--------------8<--------------
  if (regnum == gdbarch_pc_regnum (gdbarch))
    regnum = ARC_ERET_REGNUM;
  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
-------------->8--------------

Root cause:
Although this is using the correct offset (ERET register's), it is also
changing the REGNUM itself.  Therefore, raw_collect (regnum, ...) is
not reading from "pc" anymore.

v2:
- Fix a copy/paste issue as rightfully addressed by Tom [1].

[1]
https://sourceware.org/pipermail/gdb-patches/2020-November/173208.html

gdb/ChangeLog:

	* arc-linux-tdep.c (collect_register): Populate "eret" by
	"pc" value from the regcache when asked for "pc" value.
---
 gdb/ChangeLog        |  5 +++++
 gdb/arc-linux-tdep.c | 10 +++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ad2c3db7d49..568e0cdc742 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2020-12-04  Shahab Vahedi  <shahab@synopsys.com>
+
+	* arc-linux-tdep.c (collect_register): Populate "eret" by
+	"pc" value from the regcache when asked for "pc" value.
+
 2020-11-29  Hannes Domani  <ssbssa@yahoo.de>
 
 	PR tui/26973
diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index 9ff5f1214a1..6e74bae8056 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -319,8 +319,10 @@ static void
 collect_register (const struct regcache *regcache, struct gdbarch *gdbarch,
 		  int regnum, gdb_byte *buf)
 {
+  int offset;
+
   /* Skip non-existing registers.  */
-  if ((arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER))
+  if (arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER)
     return;
 
   /* The address where the execution has stopped is in pseudo-register
@@ -332,8 +334,10 @@ collect_register (const struct regcache *regcache, struct gdbarch *gdbarch,
      the program will continue at the address after the current instruction.
      */
   if (regnum == gdbarch_pc_regnum (gdbarch))
-    regnum = ARC_ERET_REGNUM;
-  regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
+    offset = arc_linux_core_reg_offsets[ARC_ERET_REGNUM];
+  else
+    offset = arc_linux_core_reg_offsets[regnum];
+  regcache->raw_collect (regnum, buf + offset);
 }
 
 void
-- 
2.29.2

Re: [PUSHED gdb-10-branch] arc: Write correct "eret" value during register collection

[Joel Brobecker]

Hello Shahab,

> In collect_register() function of arc-linux-tdep.c, the "eret"
> (exception return) register value was not being reported correctly.
> This patch fixes that.
> 
> Background:
> When asked for the "pc" value, we have to update the "eret" register
> with GDB's STOP_PC.  The "eret" instructs the kernel code where to
> jump back when an instruction has stopped due to a breakpoint.  This
> is how collect_register() was doing so:
> 
> --------------8<--------------
>   if (regnum == gdbarch_pc_regnum (gdbarch))
>     regnum = ARC_ERET_REGNUM;
>   regcache->raw_collect (regnum, buf + arc_linux_core_reg_offsets[regnum]);
> -------------->8--------------
> 
> Root cause:
> Although this is using the correct offset (ERET register's), it is also
> changing the REGNUM itself.  Therefore, raw_collect (regnum, ...) is
> not reading from "pc" anymore.
> 
> v2:
> - Fix a copy/paste issue as rightfully addressed by Tom [1].
> 
> [1]
> https://sourceware.org/pipermail/gdb-patches/2020-November/173208.html
> 
> gdb/ChangeLog:
> 
> 	* arc-linux-tdep.c (collect_register): Populate "eret" by
> 	"pc" value from the regcache when asked for "pc" value.

Once the first release off a branch is made (GDB 10.1 in this case),
we ask that all changes pushed to the branch for the corrective
release have a PR attached to the change, with details of the problem
being solved.

This is because we use the PRs to create a list of the fixes that
were made in the corrective release, and the PRs are a link to
more information for those who might be interested in them.

Would you mind doing the following please?

  - Create a PR for it? For the description of the problem, I think
    the commit message above is fine. But if you have additional
    information you can share, please do so ;-).

  - Set the PR's target milestone to GDB 10.2.

  - Once you have the PR created, please add a note to the PR
    indicating the commits that were pushed to both master and
    the gdb-10-branch, just keep have an easy track record of
    which commits solved that PR.

    Typically, I would post something like this:

        This PR was solved on master with this commit:

        | commit 10c19fadfd45da5262d2f8b9624be71c274ff15d
        | Author: Shahab Vahedi <shahab@synopsys.com>
        | Date:   Thu Nov 12 12:50:33 2020 +0100
        | Subject: arc: Write correct "eret" value during register collection

        The patch above was also pushed to gdb-10-branch as:

        | commit abaf3df98b69d66c779ff0896d66fea9cbb67481
        | Author: Shahab Vahedi <shahab@synopsys.com>
        | Date:   Thu Nov 12 12:50:33 2020 +0100
        | Subject: arc: Write correct "eret" value during register collection

  - Update the ChangeLog entry on both master and gdb-10-branch
    to include the PR number. E.g.

        | 2020-11-29  Hannes Domani  <ssbssa@yahoo.de>
        |
 !!! -> |     PR tui/26973
        |     * tui/tui-layout.c (tui_apply_current_layout): Don't delete the
        |     static locator win info.

  - Once the above is done, I believe you can close the PR as
    RESOVED/FIXED.

Thank you!

-- 
Joel

Re: [PUSHED gdb-10-branch] arc: Write correct "eret" value during register collection

[Shahab Vahedi via Gdb-patches]

Hi Joel,

Thank you for your kind nudge and sorry for messing it up. I have one question though:

On 12/4/20 3:10 PM, Joel Brobecker wrote:
> 
>   - Update the ChangeLog entry on both master and gdb-10-branch
>     to include the PR number. E.g.
> 
>         | 2020-11-29  Hannes Domani  <ssbssa@yahoo.de>
>         |
>  !!! -> |     PR tui/26973
>         |     * tui/tui-layout.c (tui_apply_current_layout): Don't delete the
>         |     static locator win info.

The patch in "master" branch was submitted on 12-Nov-2020. Hence, its ChangeLog entry
is buried under other ones after 3 weeks. Is it OK if I just edit ChangeLog in "Master"
to update that entry back? Or is it only allowed to add things to top of ChangeLogs?

I think it should be OK, but I wanted to be sure.

-- 
Shahab

Re: [PUSHED gdb-10-branch] arc: Write correct "eret" value during register collection

[Joel Brobecker]

> Thank you for your kind nudge and sorry for messing it up. I have one
> question though:
> 
> On 12/4/20 3:10 PM, Joel Brobecker wrote:
> > 
> >   - Update the ChangeLog entry on both master and gdb-10-branch
> >     to include the PR number. E.g.
> > 
> >         | 2020-11-29  Hannes Domani  <ssbssa@yahoo.de>
> >         |
> >  !!! -> |     PR tui/26973
> >         |     * tui/tui-layout.c (tui_apply_current_layout): Don't delete the
> >         |     static locator win info.
> 
> The patch in "master" branch was submitted on 12-Nov-2020. Hence, its
> ChangeLog entry is buried under other ones after 3 weeks. Is it OK if
> I just edit ChangeLog in "Master" to update that entry back? Or is it
> only allowed to add things to top of ChangeLogs?

Yes, you can just go back and edit the ChangeLog file on its own.

We fix the ChangeLog up on occasions, when we detect mistakes or
omissions.

-- 
Joel

Re: [PUSHED gdb-10-branch] arc: Write correct "eret" value during register collection

[Shahab Vahedi via Gdb-patches]

Done! [1][2][3]

[1] Bug 27015 - ARC: "eret" value is collected from the wrong data in register cache
https://sourceware.org/bugzilla/show_bug.cgi?id=27015

[2] master -  Update gdb/ChangeLog to reflect the PR for a bug fix
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=10806efd8d

[3] gdb-10-branch -  Update gdb/ChangeLog to reflect the PR for a bug fix
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=4cf1920e40

-- 
Shahab

Re: [PUSHED gdb-10-branch] arc: Write correct "eret" value during register collection

[Joel Brobecker]

> Done! [1][2][3]
> 
> [1] Bug 27015 - ARC: "eret" value is collected from the wrong data in register cache
> https://sourceware.org/bugzilla/show_bug.cgi?id=27015
> 
> [2] master -  Update gdb/ChangeLog to reflect the PR for a bug fix
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=10806efd8d
> 
> [3] gdb-10-branch -  Update gdb/ChangeLog to reflect the PR for a bug fix
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=4cf1920e40

In a word: Perfect!

Thank you :).
-- 
Joel