Re: [PATCH] gdb: mips: Add MIPSR6 support

["Maciej W. Rozycki" <macro@orcam.me.uk>]

On Mon, 3 Oct 2022, Andrew Burgess wrote:

> > +      /* Step through the forbidden slot to avoid repeated exceptions we do
> > +	 not currently have access to the BD bit when hitting a breakpoint
> > +	 and therefore cannot tell if the breakpoint hit on the branch or the
> > +	 forbidden slot.  */
> > +      /* delay_slot_size = 0; */
> 
> It is probably just because I don't understand the MIPS architecture
> well enough, but I don't see the connection between the text of the
> comment, and the commented out source line.
> 
> Assuming the comment is explaining why the source line is commented out,
> I'd like, at a minimum, to see the source line included within the same
> comment block.
> 
> In general though, I'm not a fan of commented out code.  The question I
> would ask is, when will it be un-commented?  If the answer is never,
> then lets just remove the commented code.
> 
> The next question I have is, if we can't include this code, then is
> something going to go wrong?  What would be the expected consequence of
> this thing going wrong?
> 
> My ideal comment would include a description of what might go wrong, why
> we can't solve the problem right now, and what we would need to do to
> fix it, in this case it seems like 'check the BD bit' would be part of
> the solution.  This way, when someone hits the problem and tries to
> debug it, then they will see the comment and thing, "hey, that's the
> exact problem I'm seeing....".

 FWIW there should be no problem with accessing the BD bit, which is in 
the CP0 Cause register.  It is a privileged register, but it's made 
available to the debugger at least by Linux, and I believe FreeBSD as 
well, with the BD bit faithfully reported.

 It's also directly accessible with bare metal targets, though I can 
imagine there might be complications depending on what debug firmware does 
(MDI comes to mind, though I can't remember the details offhand), but 
addressing that can be probably delegated to the relevant bare metal debug 
stub.

 Historically we've been avoiding placing breakpoints (either regular ones 
or temporary single-stepping ones) in branch delay slots though, so why 
should we start with the inclusion of MIPSr6 support?  I guess we could 
(and that could help in some debug scenarios), provided such a change has 
been sufficiently verified, but that ought to be a separate submission.

 In any case we have code already for delay slot avoidance, so I don't 
think there's a need for R6 to make any extra arrangements apart from 
letting that code know what the extra instructions are in R6 that have a 
branch delay slot (either a regular one or a "forbidden" one).

 As I say, FWIW.

  Maciej