Date: Tue, 18 Jun 2024 07:43:42 -0700
From: Doug Anderson <dianders@...omium.org>
To: Daniel Thompson <daniel.thompson@...aro.org>
Cc: kgdb-bugreport@...ts.sourceforge.net, 
	Christophe JAILLET <christophe.jaillet@...adoo.fr>, Jason Wessel <jason.wessel@...driver.com>, 
	Thorsten Blum <thorsten.blum@...lux.com>, Yuran Pereira <yuran.pereira@...mail.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/13] kdb: Tweak "repeat" handling code for "mdW" and "mdWcN"

Hi,

On Tue, Jun 18, 2024 at 5:57 AM Daniel Thompson
<daniel.thompson@...aro.org> wrote:
>
> On Mon, Jun 17, 2024 at 05:34:41PM -0700, Douglas Anderson wrote:
> > In general, "md"-style commands are meant to be "repeated". This is a
> > feature of kdb and "md"-style commands get it enabled because they
> > have the flag KDB_REPEAT_NO_ARGS. What this means is that if you type
> > "md4c2 0xffffff808ef05400" and then keep hitting return on the "kdb>"
> > prompt that you'll read more and more memory. For instance:
> >   [5]kdb> md4c2 0xffffff808ef05400
> >   0xffffff808ef05400 00000204 00000000                     ........
> >   [5]kdb>
> >   0xffffff808ef05408 8235e000 00000000                     ..5.....
> >   [5]kdb>
> >   0xffffff808ef05410 00000003 00000001                     ........
> >
> > As a side effect of the way kdb works is implemented, you can get the
> > same behavior as the above by typing the command again with no
> > arguments. Though it seems unlikely anyone would do this it shouldn't
> > really hurt:
> >   [5]kdb> md4c2 0xffffff808ef05400
> >   0xffffff808ef05400 00000204 00000000                     ........
> >   [5]kdb> md4c2
> >   0xffffff808ef05408 8235e000 00000000                     ..5.....
> >   [5]kdb> md4c2
> >   0xffffff808ef05410 00000003 00000001                     ........
> >
> > In general supporting "repeat" should be easy. If argc is 0 then we
> > just copy the results of the arg parsing from the last time, making
> > sure that the address has been updated. This is all handled nicely in
> > the "if (argc == 0)" clause in kdb_md().
> >
> > Oddly, the "mdW" and "mdWcN" code seems to update "last_bytesperword"
> > and "last_repeat", which doesn't seem like it should be necessary. It
> > appears that this code is needed to make this use case work, though
> > it's a bit unclear if this is truly an important feature to support:
> >   [1]kdb> md2c3 0xffffff80c8e2b280
> >   0xffffff80c8e2b280 0200 0000 0000                            ...
> >   [1]kdb> md2c4
> >   0xffffff80c8e2b286 0000 e000 8235 0000                       ...
> >
> > In order to abstract the code better, remove the code updating
> > "last_bytesperword" and "last_repeat" from the "mdW" and "mdWcN"
> > handling. This breaks the above case where the user tweaked "argv[0]"
> > and then tried to somehow leverage the "repeat" code to do something
> > smart, but that feels like it was a misfeature anyway.
>
> I'm not too keen on "successfully" doing the wrong thing.
>
> In that light I'd view this as a feature that is arguably simpler to
> implement than it is to error check *not* implementing it. In other
> words by the time you add error checking to the argc == 0 path to
> spot mismatches then you are better off honouring the user request
> rather then telling them they got it wrong.

Hmmm. How about we store the last "argv0" and we just immediately fail
if "argc == 0" and "argv[0]" doesn't match the previous one?

The override rules for lines / word count is already complicated
enough and my head was spinning trying to get this right and reason
about it. I tried several times and each time I thought I had it
working cleanly I ended up with some other weird corner case that was
broken. For instance, with the current code this case is broken (IMO):

[3]kdb> md2c4 0xffff0000d8948040
0xffff0000d8948040 0204 0000 0000 0000                       ........
[3]kdb> md2
0xffff0000d8948048 3000 824f 0000 0000 0003 0000 0001 0000   .0O.............
0xffff0000d8948058 0003 0000 0000 0000 0000 0000 0000 0000   ................
0xffff0000d8948068 8000 884a 8000 ffff 0001 0000 0100 0040   ..J...........@.
0xffff0000d8948078 0000 0000 0001 0000 0000 0000 0000 0000   ................
0xffff0000d8948088 0030 0000 0000 0000 000a 0000 0000 0000   0...............
0xffff0000d8948098 f0e6 fffb 0000 0000 9340 809b 0000 ffff   ........@.......
0xffff0000d89480a8 0005 0000 0003 0000 0001 0000 0078 0000   ............x...
0xffff0000d89480b8 0078 0000 0078 0000 0000 0000 0000 0000   x...x...........

Specifically I would have expected the "last" wordcount to persist but
instead it fell back to the default mdcount. Maybe the above is right
but it's distinctly non-obvious.


> Daniel.
>
>
> PS I have never done so but I also wondered if it is reasonable to use
>    this feature to manually decompose structures. For example:
>
>      md1c1 structure_pointer; md1c7; md8c1; md8c1; md2c2

Sure, you can come up with cases where it could be useful, but it
feels like there are other ways to accomplish the same thing w/ less
complexity.


-Doug

Powered by blists - more mailing lists