This change has been discarded.

This change has been integrated into RR3207 (which does it in a lot better way).

Information
Submitter:	Boris Shingarov
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11209:83e890c83564
\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-
arm: Remote GDB: register xfer for AArch32

This patch stops remote gdb from panicking when connecting to a 32-bit
target.

We send the response to the g-packet from the buffer and the size in
GdbRegCache.size.  Both the buffer is allocated, and the size is set,
at RemoteGDB object instantiation time, which is before we know if we
will be running an AArch32 or AArch64 workload; therefore the longest
possible size is assumed.  The problem is that at actual packet
processing time, if we are in AArch32 mode with its smaller register
set, we still answer the whole buffer and gdb panics from seeing the
oversized packet.
To avoid this, I set the 'size' member to the correct value when
processing each g-packet, according to whether we are in 32- or 64-bit
mode.

Another problem is the order in which register values are transferred.
Ideally, we should serve gdb with a feature descriptor XML document.
For now, I simply put in the default offsets assumed by gdb 7.10 in
the absence of feature descriptors.
Also in the absence of feature descriptors, gdb expects that the CPU
has extended-precision FP registers (96 bits wide).  This situation
is beyond the simple design of the reg cache as a union of uint8/...
arrays.  In my opinion, it is worth improving the general union-based
design (and possibly implementing feature descriptors) before fixing
FPs in this case; therefore this patch simply transfers zeros to gdb,
and ignores FP values coming from gdb.

Testing Done

Tested with gdb-7.10 configured with "--target=arm-linux-gnueabihf" connecting to gem5 running an arm-linux-gnueabihf binary in SE mode.
Tested also with gdb-7.10 configured with "--target=aarch64-linux-gnu" connecting to gem5 running an aarch64-linux-gnu binary in SE mode to ensure against AArch64 regressions.

Note that even with this patch, remote debugging of 32-bit ARM is still not useful because of problems with inserting breakpoints which I address in a separate patch.

Issue Summary

Description	From	Last Updated	Status
These new offsets seem correct, but I don't like the fact that you renamed them and replaced the enum. Could ...	Andreas Sandberg	Nov. 12, 2015, 5:25 a.m.	Resolved
Use a GDB32_NUMREGS constant similar to how aarch64 does it.	Andreas Sandberg	Nov. 12, 2015, 5:29 a.m.	Resolved
Why did you move this one into the aarch64 block?	Andreas Sandberg	Nov. 12, 2015, 5:34 a.m.	Dropped
Keep this in the common code path.	Andreas Sandberg	Nov. 12, 2015, 5:34 a.m.	Dropped
Are FP register transfers currently broken?	Andreas Sandberg	Nov. 12, 2015, 5:40 a.m.	Dropped

Hi Boris! Thanks for taking time to improve gem5's ARM backend.

I have had a look at the patch and it seems like the the offset changes are correct. The issues I have highlighted above (with the exception of the change to FP register transfer) are mostly style related to make everyone's lives easier. I would apprecaite it if you could fix those issues.

Also, before the change can be committed, the changeset needs to comply with gem5's commit log style. See http://www.m5sim.org/Submitting_Contributions. In particular, I'd like a more detailed description of what you change and why.

src/arch/arm/remote_gdb.hh (Diff revision 1)

These new offsets seem correct, but I don't like the fact that you renamed them and replaced the enum.

Could you keep the old names and enum structure? That makes the code symmetrical with respect to the aarch64 and it would reduce the size of the changeset (and that makes reviewing much easier).

Show all issues

Boris Shingarov Nov. 12, 2015, 5:46 a.m. (Nov. 12, 2015, 5:46 a.m.)

The intent of these offsets is different.  The intent of the enum is to count registers, and in that sense those values are ordinal numbers.  That design is fine as long as registers are uniformly sized, but in this place, they are not, causing problems across more than one architecture where this is the case.  The fundamental issue is that we can't model a sequence of varying-size registers, some sizes not even powers of 2, as a reg32 array or reg64 array.  In this case we are lucky, but what if there were a 32-bit register followed by a 64-bit register?  How many registers are there before the 64-bit register?

I do agree, however, with your "symmetry" argument.  I have reverted the names and the enum to how they were, so that improvements to how the offsets are calculated can be made in a separate patch across the board (so that the code continues to be consistent).

src/arch/arm/remote_gdb.hh (Diff revision 1)

Use a GDB32_NUMREGS constant similar to how aarch64 does it.

Show all issues

Boris Shingarov Nov. 12, 2015, 5:46 a.m. (Nov. 12, 2015, 5:46 a.m.)

See above.  When some registers are strange sizes like 96 bits (and C compilers generally don't even give us something to put into that union of uint8/16/32/64), and others are 32 bits, to count registers and then multiply by the size, does not work.  However, in this case we are lucky that we can (although the counting is somewhat perverse, but at least we don't end up with a fractional count -- which could easily happen), so I preserved the original structure.

src/arch/arm/remote_gdb.cc (Diff revision 1)

Why did you move this one into the aarch64 block?

Show all issues

Boris Shingarov Nov. 12, 2015, 5:46 a.m. (Nov. 12, 2015, 5:46 a.m.)

I didn't simply move it into the aarch64 block but to after the calculation of the size, so that bytes() will return the correct value.  Granted, "size" is preset to a safe (large enough) value as well as the buffer is safely allocated for the largest case, but that is a spaghetti argument.

src/arch/arm/remote_gdb.cc (Diff revision 1)

Keep this in the common code path.

Show all issues

Boris Shingarov Nov. 12, 2015, 5:46 a.m. (Nov. 12, 2015, 5:46 a.m.)
```
See above.
```

src/arch/arm/remote_gdb.cc (Diff revision 1)

Are FP register transfers currently broken?

Show all issues

Boris Shingarov Nov. 12, 2015, 5:46 a.m. (Nov. 12, 2015, 5:46 a.m.)

They are broken.  By default, gdb on "arm-linux-gnueabihf" assumes extended precision, transferring 96 bits on the wire.  Such a value does not fit in regs32[...] as written.
Fixing this will be non-trivial.
Even if the compiler were to provide 96-bit scalars to add to the union, counting registers breaks: how many 96-bit regs are sixteen 32-bit GPRs?

Diff:

Revision 2 (+45 -48)

Show changes

	src/arch/arm/remote_gdb.hh
	src/arch/arm/remote_gdb.cc
	src/sim/system.cc

Hi Boris. I think you might have uploaded the wrong diff (or potentially a diff that needs to be applied after your old diff). Also, the diff viewer in ReviewBoard can't show it, which suggests that it wasn't uploaded correctly.

Description:

~		Changeset 11209:83e890c83564
	~	Changeset 11209:83e890c83564
	+	---------------------------
	+	arm: Remote GDB: register xfer for AArch32

~		arm: Remote GDB: register xfer for AArch32
	~	This patch stops remote gdb from panicking when connecting to a 32-bit
	+	target.
	+
	+	We send the response to the g-packet from the buffer and the size in
	+	GdbRegCache.size. Both the buffer is allocated, and the size is set,
	+	at RemoteGDB object instantiation time, which is before we know if we
	+	will be running an AArch32 or AArch64 workload; therefore the longest
	+	possible size is assumed. The problem is that at actual packet
	+	processing time, if we are in AArch32 mode with its smaller register
	+	set, we still answer the whole buffer and gdb panics from seeing the
	+	oversized packet.
	+	To avoid this, I set the 'size' member to the correct value when
	+	processing each g-packet, according to whether we are in 32- or 64-bit
	+	mode.
	+
	+	Another problem is the order in which register values are transferred.
	+	Ideally, we should serve gdb with a feature descriptor XML document.
	+	For now, I simply put in the default offsets assumed by gdb 7.10 in
	+	the absence of feature descriptors.
	+	Also in the absence of feature descriptors, gdb expects that the CPU
	+	has extended-precision FP registers (96 bits wide). This situation
	+	is beyond the simple design of the reg cache as a union of uint8/...
	+	arrays. In my opinion, it is worth improving the general union-based
	+	design (and possibly implementing feature descriptors) before fixing
	+	FPs in this case; therefore this patch simply transfers zeros to gdb,
	+	and ignores FP values coming from gdb.

Diff:

Revision 3 (+1 -1)

Show changes

src/sim/system.cc

Diff:

Revision 4 (+1 -1)

Show changes

src/sim/system.cc

Change Summary:

Ok, I uploaded the patch file manually and now it seems to show correctly on rb.  I guess I can't trust the "hg postreview" extension.

Diff:

Revision 5 (+30 -25)

Show changes

	src/arch/arm/remote_gdb.hh
	src/arch/arm/remote_gdb.cc

Status: Discarded

Change Summary:

This change has been integrated into RR3207 (which does it in a lot better way).

You have a pending review.

Review Board 2.0.15

This change has been discarded.

arm: Remote GDB: register xfer for AArch32

Screenshots

Files

Issue Summary

Diff:

Description:

Changeset 11209:83e890c83564

Diff:

Diff:

Change Summary:

Diff:

Status: Discarded

Change Summary: