Re: [hatari-devel] Re: Breakpoint Dn width

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

On 01/24/2018 02:22 AM, Miro Kropáček wrote:
> On 24 January 2018 at 10:14, Miro Kropáček <miro.kropacek@xxxxxxxxx> wrote:
>
> > > b a5 = $c4ba && d2. b = $ee
> > ERROR in tokenized string:
> > 'a5 = $c4ba && d2 . b = $ee'
> >                    ^-space/width modifier makes sense only for an address
> > (register)

There are few reasons for allowing this only for indirect accesses:

1) I think widths are most used/useful for indirect accesses,
   so error about forgetting the parenthesis is good.
   For comparisons with just some of the bits in the register,
   user is supposed to use mask, like you noticed

2) For some other types, width really doesn't make sense, and
   allowing use of width for everything could sometimes result
   in very confusing error messages (about some things set internally,
   not something user has set on the command line directly)

3) While width does make sense for registers, internally few
   of the registers (PC & SR) are handled as functions, not as
   directly accessible CPU emulation variables, and I don't want
   to enable width modifier for them due to 2).  It would be
   illogical for user if width modifier would work just for
   a subset of registers

With complex stuff like that, it's better to keep usage logical,
and error messages as helpful as possible.


> [..]
>
> > How can I compare just for the lower 8 bits of d2 itself?
>
> OK, found it:
>
> a5 = $c4ba && d2 & $ff = $ee
>
> Still, supporting An.<width> / Dn.<width> seems like a nice feature.

Attached is a patch to do that.  I'm not going to commit it
though, due to 1) & 3).

Instead, I commited hopefully a more useful error message:
https://hg.tuxfamily.org/mercurialroot/hatari/hatari/rev/d9e385efc54d


	- Eero

diff -r a880a0b9d1f9 src/debug/breakcond.c
--- a/src/debug/breakcond.c	Wed Jan 24 21:16:26 2018 +0200
+++ b/src/debug/breakcond.c	Wed Jan 24 23:11:44 2018 +0200
@@ -45,6 +45,12 @@
 
 #define BC_DEFAULT_DSP_SPACE 'P'
 
+/* value types from vars.h */
+static inline bool is_type_register(value_t x)
+{
+	return (x == VALUE_TYPE_REG16 || x == VALUE_TYPE_REG32);
+}
+
 typedef struct {
 	bool is_indirect;
 	char dsp_space;	/* DSP has P, X, Y address spaces, zero if not DSP */
@@ -604,13 +610,13 @@
 		EXITFUNC(("arg:%d -> true (missing)\n", pstate->arg));
 		return true;
 	}
-	if (!bc_value->is_indirect) {
-		pstate->error = "space/width modifier makes sense only for an address (register)";
-		EXITFUNC(("arg:%d -> false\n", pstate->arg));
-		return false;
-	}
-	pstate->arg++;
 	if (bc_value->dsp_space) {
+		if (!bc_value->is_indirect) {
+			pstate->error = "address space modifier makes sense only for an address (register)";
+			EXITFUNC(("arg:%d -> false\n", pstate->arg));
+			return false;
+		}
+		pstate->arg++;
 		switch (pstate->argv[pstate->arg][0]) {
 		case 'p':
 		case 'x':
@@ -623,6 +629,14 @@
 			return false;
 		}
 	} else {
+#if 1
+		if (!(bc_value->is_indirect || is_type_register(bc_value->valuetype))) {
+			pstate->error = "width modifier can be used only with addresses and (A0-7/D0-7) registers";
+			EXITFUNC(("arg:%d -> false\n", pstate->arg));
+			return false;
+		}
+#endif
+		pstate->arg++;
 		switch (pstate->argv[pstate->arg][0]) {
 		case 'l':
 			mode = 32;
@@ -648,6 +662,11 @@
 		bc_value->dsp_space = mode;
 		EXITFUNC(("arg:%d -> space:%c, true\n", pstate->arg, mode));
 	} else {
+		if (is_type_register(bc_value->valuetype) && (unsigned)mode > bc_value->bits) {
+			pstate->error = "register isn't wide enough for given width modifier";
+			EXITFUNC(("arg:%d -> false\n", pstate->arg));
+			return false;
+		}
 		bc_value->bits = mode;
 		EXITFUNC(("arg:%d -> width:%d, true\n", pstate->arg, mode));
 	}