Re: Possible bug in Aarch64 single-stepping [PATCH]

[Peter Maydell]

On Sun, 8 May 2022 at 20:27, Chris Howard <cvz185@web.de> wrote:
> Thanks for the explanation. What with this being “pretty easy” I’m attempting 
> my first ever patch!  :-)

Thanks for having a go at this. You've got the right basic
idea, but the process details of how to submit a patch aren't
quite right. We document all that stuff here:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html
It's a pretty long document, but we've tried to put the important
stuff at the top. The really important bit is that patches must
have a "Signed-off-by:" line, which is how you say "I wrote
this code and it's OK for it to go into QEMU"; I can fix up
most of the other minor stuff at my end but without that we
can't take the patch at all.

It's also best to send patches as new threads, not as replies
to existing ones: the tooling (and some humans) assumes that.

> This is a context diff with respect to the cloned git repository (Version 
> 7.0.50)

Unified diffs are much easier to read than context ones.
(If you create a proper git commit then "git show" and
"git format-patch" and so on ought to default to unified.)

> *** qemu/target/arm/helper.c    2022-05-08 20:41:48.000000000 +0200
> --- qemu-patch/target/arm/helper.c      2022-05-08 20:55:25.000000000 +0200
> ***************
> *** 6551,6559 ****
>           define_one_arm_cp_reg(cpu, &dbgdidr);
>       }
>
> !     /* Note that all these register fields hold "number of Xs minus 1". */
> !     brps = arm_num_brps(cpu);
> !     wrps = arm_num_wrps(cpu);
>       ctx_cmps = arm_num_ctx_cmps(cpu);
>
>       assert(ctx_cmps <= brps);
> --- 6551,6559 ----
>           define_one_arm_cp_reg(cpu, &dbgdidr);
>       }
>
> !     /* Note that all these reg fields (in ID_AA64DFR0_EL1) hold "number of 
> Xs minus 1". */
> !     brps = arm_num_brps(cpu);                 /* returns actual number of 
> breakpoints */
> !     wrps = arm_num_wrps(cpu);                 /* returns actual number of 
> watchpoints */
>       ctx_cmps = arm_num_ctx_cmps(cpu);
>
>       assert(ctx_cmps <= brps);

I agree that the comment here is wrong, but this is an unrelated
change. We prefer to put separate changes in separate patches,
even if they're in close areas of the code.
I would just delete the existing comment line (as a separate patch)
-- it's clear enough that arm_num_brps() returns the number of
breakpoints, so we don't need to add extra comments saying so.
(The old comment is an accidental leftover from before we defined
those functions, when the code used to in-line extract values from
the ID register fields. The definitions of arm_num_brps() etc in
internals.h, where the ID-register reading now happens, already
have comments remarking about the fields being "num bps - 1".)

> ***************
> *** 6565,6578 ****

The rest of the change looks OK, assuming I'm reading the
context-diff correctly.

thanks
-- PMM