| Message ID | 20240523035124.2639220-1-thiago.bauermann@linaro.org |
|---|---|
| Headers | show |
| Series | Add support for AArch64 MOPS instructions | expand |
On 5/23/24 04:51, Thiago Jung Bauermann wrote: > Hello, > > Almost all of the changes in this version are in patch 2. Its changelog > has the details. > > One of them is a simplification of the code in > aarch64_record_memcopy_memset because Luis noticed that both code paths in > it can share code. Also, the gdb.reverse/aarch64-mops.exp was moved from > patch 5 (which doesn't exist anymore) to patch 2, and code cleanups > suggested by Guinevere were implemented. In addition, it was adapted to > work with Clang's line number information, which considers some register > preparation instructions as part of the line with the asm statement. > > Also, the testcase gdb.arch/aarch64-mops-single-step.exp was moved to > patch 1 so patch 4 doesn't exist anymore either. > > Here is the original cover letter for convenience: > > This patch series implements GDB support for the new instructions in > AArch64's MOPS feature. Patch 1 has a small overview. > > What is needed from GDB is recognizing the MOPS sequences of instructions > as atomic so that they can be stepped over during instruction single > stepping, and also to avoid doing displaced stepping with them. This is > done in patch 1. > > Patch 2 adds support for the new instructions to the record an replay > target. > > The other patches add testcases to test each of the aspects above, plus > one testcase to verify the interaction of the MOPS instructions with > watchpoints. > > Tested on Ubuntu 23.10 aarch64-linux-gnu with no regressions, using the > Arm FVP emulator as well as QEMU v8.2. > > > Thiago Jung Bauermann (3): > gdb/aarch64: Disable displaced single-step for MOPS instructions > gdb/aarch64: Add record support for MOPS instructions. > gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp > > gdb/aarch64-tdep.c | 77 +++++++- > .../gdb.arch/aarch64-mops-single-step.c | 73 +++++++ > .../gdb.arch/aarch64-mops-single-step.exp | 98 +++++++++ > .../gdb.arch/aarch64-mops-watchpoint.c | 66 +++++++ > .../gdb.arch/aarch64-mops-watchpoint.exp | 79 ++++++++ > gdb/testsuite/gdb.reverse/aarch64-mops.c | 78 ++++++++ > gdb/testsuite/gdb.reverse/aarch64-mops.exp | 186 ++++++++++++++++++ > gdb/testsuite/lib/gdb.exp | 99 ++++++++++ > 8 files changed, 753 insertions(+), 3 deletions(-) > create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-single-step.c > create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-single-step.exp > create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c > create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp > create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.c > create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.exp > > > base-commit: 002ccda0ef390fc2f02c0a27f01993bd5009f03d I went through the tests and validated they work fine on an emulator. Unless there are other specific objections (from Guinevere on the record/replay side or Pedro), this looks good to me. Approved-By: Luis Machado <luis.machado@arm.com> Tested-By: Luis Machado <luis.machado@arm.com>
Hello Luis, Luis Machado <luis.machado@arm.com> writes: > On 5/23/24 04:51, Thiago Jung Bauermann wrote: >> Hello, >> >> Almost all of the changes in this version are in patch 2. Its changelog >> has the details. >> >> One of them is a simplification of the code in >> aarch64_record_memcopy_memset because Luis noticed that both code paths in >> it can share code. Also, the gdb.reverse/aarch64-mops.exp was moved from >> patch 5 (which doesn't exist anymore) to patch 2, and code cleanups >> suggested by Guinevere were implemented. In addition, it was adapted to >> work with Clang's line number information, which considers some register >> preparation instructions as part of the line with the asm statement. >> >> Also, the testcase gdb.arch/aarch64-mops-single-step.exp was moved to >> patch 1 so patch 4 doesn't exist anymore either. >> >> Here is the original cover letter for convenience: >> >> This patch series implements GDB support for the new instructions in >> AArch64's MOPS feature. Patch 1 has a small overview. >> >> What is needed from GDB is recognizing the MOPS sequences of instructions >> as atomic so that they can be stepped over during instruction single >> stepping, and also to avoid doing displaced stepping with them. This is >> done in patch 1. >> >> Patch 2 adds support for the new instructions to the record an replay >> target. >> >> The other patches add testcases to test each of the aspects above, plus >> one testcase to verify the interaction of the MOPS instructions with >> watchpoints. >> >> Tested on Ubuntu 23.10 aarch64-linux-gnu with no regressions, using the >> Arm FVP emulator as well as QEMU v8.2. >> >> >> Thiago Jung Bauermann (3): >> gdb/aarch64: Disable displaced single-step for MOPS instructions >> gdb/aarch64: Add record support for MOPS instructions. >> gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp >> >> gdb/aarch64-tdep.c | 77 +++++++- >> .../gdb.arch/aarch64-mops-single-step.c | 73 +++++++ >> .../gdb.arch/aarch64-mops-single-step.exp | 98 +++++++++ >> .../gdb.arch/aarch64-mops-watchpoint.c | 66 +++++++ >> .../gdb.arch/aarch64-mops-watchpoint.exp | 79 ++++++++ >> gdb/testsuite/gdb.reverse/aarch64-mops.c | 78 ++++++++ >> gdb/testsuite/gdb.reverse/aarch64-mops.exp | 186 ++++++++++++++++++ >> gdb/testsuite/lib/gdb.exp | 99 ++++++++++ >> 8 files changed, 753 insertions(+), 3 deletions(-) >> create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-single-step.c >> create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-single-step.exp >> create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c >> create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp >> create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.c >> create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.exp >> >> >> base-commit: 002ccda0ef390fc2f02c0a27f01993bd5009f03d > > I went through the tests and validated they work fine on an emulator. > > Unless there are other specific objections (from Guinevere on the record/replay side or > Pedro), > this looks good to me. > > Approved-By: Luis Machado <luis.machado@arm.com> > Tested-By: Luis Machado <luis.machado@arm.com> Thank you! I'll wait for additional feedback. I would also like to know whether it's fine to backport this series to the release branch.
On 6/3/24 18:07, Thiago Jung Bauermann wrote: > Hello Luis, > > Luis Machado <luis.machado@arm.com> writes: > >> On 5/23/24 04:51, Thiago Jung Bauermann wrote: >>> Hello, >>> >>> Almost all of the changes in this version are in patch 2. Its changelog >>> has the details. >>> >>> One of them is a simplification of the code in >>> aarch64_record_memcopy_memset because Luis noticed that both code paths in >>> it can share code. Also, the gdb.reverse/aarch64-mops.exp was moved from >>> patch 5 (which doesn't exist anymore) to patch 2, and code cleanups >>> suggested by Guinevere were implemented. In addition, it was adapted to >>> work with Clang's line number information, which considers some register >>> preparation instructions as part of the line with the asm statement. >>> >>> Also, the testcase gdb.arch/aarch64-mops-single-step.exp was moved to >>> patch 1 so patch 4 doesn't exist anymore either. >>> >>> Here is the original cover letter for convenience: >>> >>> This patch series implements GDB support for the new instructions in >>> AArch64's MOPS feature. Patch 1 has a small overview. >>> >>> What is needed from GDB is recognizing the MOPS sequences of instructions >>> as atomic so that they can be stepped over during instruction single >>> stepping, and also to avoid doing displaced stepping with them. This is >>> done in patch 1. >>> >>> Patch 2 adds support for the new instructions to the record an replay >>> target. >>> >>> The other patches add testcases to test each of the aspects above, plus >>> one testcase to verify the interaction of the MOPS instructions with >>> watchpoints. >>> >>> Tested on Ubuntu 23.10 aarch64-linux-gnu with no regressions, using the >>> Arm FVP emulator as well as QEMU v8.2. >>> >>> >>> Thiago Jung Bauermann (3): >>> gdb/aarch64: Disable displaced single-step for MOPS instructions >>> gdb/aarch64: Add record support for MOPS instructions. >>> gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp >>> >>> gdb/aarch64-tdep.c | 77 +++++++- >>> .../gdb.arch/aarch64-mops-single-step.c | 73 +++++++ >>> .../gdb.arch/aarch64-mops-single-step.exp | 98 +++++++++ >>> .../gdb.arch/aarch64-mops-watchpoint.c | 66 +++++++ >>> .../gdb.arch/aarch64-mops-watchpoint.exp | 79 ++++++++ >>> gdb/testsuite/gdb.reverse/aarch64-mops.c | 78 ++++++++ >>> gdb/testsuite/gdb.reverse/aarch64-mops.exp | 186 ++++++++++++++++++ >>> gdb/testsuite/lib/gdb.exp | 99 ++++++++++ >>> 8 files changed, 753 insertions(+), 3 deletions(-) >>> create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-single-step.c >>> create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-single-step.exp >>> create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c >>> create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp >>> create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.c >>> create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.exp >>> >>> >>> base-commit: 002ccda0ef390fc2f02c0a27f01993bd5009f03d >> >> I went through the tests and validated they work fine on an emulator. >> >> Unless there are other specific objections (from Guinevere on the record/replay side or >> Pedro), >> this looks good to me. >> >> Approved-By: Luis Machado <luis.machado@arm.com> >> Tested-By: Luis Machado <luis.machado@arm.com> > > Thank you! I'll wait for additional feedback. > > I would also like to know whether it's fine to backport this series to > the release branch. > Given it is a smaller scope of change, I don't see a problem with that.
Hello, Luis Machado <luis.machado@arm.com> writes: > On 6/3/24 18:07, Thiago Jung Bauermann wrote: >> Hello Luis, >> >> Luis Machado <luis.machado@arm.com> writes: >> >>> I went through the tests and validated they work fine on an emulator. >>> >>> Unless there are other specific objections (from Guinevere on the record/replay side or >>> Pedro), >>> this looks good to me. >>> >>> Approved-By: Luis Machado <luis.machado@arm.com> >>> Tested-By: Luis Machado <luis.machado@arm.com> >> >> Thank you! I'll wait for additional feedback. >> >> I would also like to know whether it's fine to backport this series to >> the release branch. > > Given it is a smaller scope of change, I don't see a problem with that. Thanks! I just pushed to the main branch ending at commit 55e3fcf5e523 and to gdb-15-branch ending at commit 3504ea29464f.