This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 4/4] Support ranged and masked breakpoints


[hardware range breakpoints]

> +  add_com ("hbreak-range", class_breakpoint, hbreak_range_command, _("\
> +Set a hardware assisted breakpoint for an address range.\n\
> +The address range should be specified in one of the following formats:\n\
> +\n\
> +   start-address, end-address\n\
> +   start-address, +length\n\
> +   start-line, end-line\n\
> +\n\
> +The breakpoint will stop execution of your program whenever the inferior\n\
> +executes any address within the [start-address, end-address] interval."));

A small detail: Typically, the second argument of the range is excluded.
For instance, that's what we do with the disass. Also, if you're watching
8 bytes at address ADDR, then you're watching 2 words, at ADDR and ADDR+4.
If the end range is ADDR+length, then ADDR+8 is excluded...  I think it's
important to be precise about this, because it will determine whether
some of the comparison operators should be strict or not.  See question
below.

I think that the arguments for this command should follow the same
semantics as the various break commands.  Otherwise, I am wondering
if users might get confused that one of the break commands follow
a different syntax?

    hbreak-range LOCATION, LOCATION_OR_LENGTH

where:

   LOCATION := any valid breakpoint location expression
   LOCATION_OR_LENGTH := '+' expression_as_long | LOCATION

In terms of implementation, this allows you do parse the first
argument as a linespec (use decode_line_1), and then check
the start of the second argument for a '+' and parse-and-eval-as-addr
or decode_line_1 again.

The checking for a 0x address suffix to see if the argument is
an address or a line is very limitative: Ada users might prefer to
use the ada notation (16#deadbeef#) and some users might be interested
in using a convenience variable, or any valid expression returning
something that can be used as an address. Same for expression the
length...

One thing I'd also like to change is our view on this functionality,
and just imagine that we could have a software version of the range
breakpoints.  So, I'd like to suggest that we get rid of the h in
the range-breakpoint command. And I'd also like to change the
implementation to treat range-breakpoints generally, with as little
considering for the hardware-vs-software implementation - with a
check at the beginning of the range-breakpoint that would error out
if the target does not support hardware range-breakpoints... Other
than that, I'd like to replace checks for h/w range-breakpoints into
checks for range-breakpoints in general, unless it does not make
sense.  I think it'll work, and make it easier for anyone interested
in implementing a software version of this feature.

Also on the user interface front, there is also the issue of temporary
breakpoints. I think the way we handle it in GDB sucks, using different
commands rather than flags, but that's the way it is. So we might have
to look at tbreak-range as well.  Perhaps we can leave that for later,
as this should be trivial to implement once we have the non-temporary
version.  However, this does influence this patch since the temporary
attribute of a breakpoint, hardware or not, is independent of whether
it is implemented using hardware capabilities or not.

In terms of implementation, I'm almost thinking that this type of
breakpoint should be implemented using the breakpoint_ops structure.
It would cause a bit of code redundancy, but your patch is typically
doing:

  if (is_range_breakpoint (...))
    {
      do something
    }
  else
    {
      do the regular thing;
    }

I'll let you explore this and let me know what you think. I'm 50/50
on this suggestion, as it has pros and cons.  The advantage is that
you no longer have to check for the type of hw_breakpoint that you have.
I can immediately assume that this is the type that you have.  The
downside is a bit of code duplication in how we display the information.
We might be able to work around this by factorizing the code out of
print_it_typical, print_one_breakpoint_location, etc...

> +  bpt->target_info.length = bpt->ranged_hw_bp_length;

If this component is set to zero by default for normal breakpoints,
then I think that it will be sufficient to use it as a flag to determine
whether our breakpoint is range breakpoint or not.  That way, we can
avoid the point_kind flag entirely.

The one problem remaining is: how does the core know whether range
breakpoints are supported or not. I'm torn.  I don't think that
informing the user at insertion time that range breakpoints are not
supported is all that nice.  So I think we need to ask the target.

Yay, we have have to_can_use_hw_breakpoint routine in target_ops.
Oh, but wait, it's actually used for watchpoints:

    #define target_can_use_hardware_watchpoint(TYPE,CNT,OTHERTYPE) \
     (*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE);

/me is swearing while his brain is overheating (yes I know, one wonders
how something so small could generate so much heat...).

I don't see any other way except providing the equivalent for
hardware breakpoints (range or not, btw). So we need to fix the
unfortunate naming first, which I'll try to do tomorrow, and then
define a new target_ops routine that will return non-zero if the
kind of h/w breakpoint we need is supported.

I had a few notes that I wrote while reading the patch.

> -	  && breakpoint_address_match (bpt->pspace->aspace, bpt->address,
> -				       aspace, pc))
> +	  && ((breakpoint_address_match (bpt->pspace->aspace, bpt->address,
> +					aspace, pc) && ((bpt->address == pc)))
                                                       ^^^^^^^^^^^^^^^^^^^^^^

Too many parentheses, but it also seems redundant with the check
already performed in breakpoint_address_match?

> +	       || (bpt->owner->hw_point_flag == HW_POINT_RANGED_BREAK
> +		   && breakpoint_address_match_range (bpt->pspace->aspace,
> +						      bpt->address,
> +						      bpt->ranged_hw_bp_length,
> +						      aspace, pc)
> +		   && pc >= bpt->address
> +		   && pc < (bpt->address + bpt->ranged_hw_bp_length))))

Similarly, the pc range check is already do by breakpoint_address_match_range.  
The only tiny difference if in the second check where you have a strict
comparison here whereas breakpoint_address_match_range uses a
less-or-equal comparison.  Intuitively, I'd think that the strict
comparison is the correct one, but it depends on how length is computed.

> +
> +	if (b->hw_point_flag == HW_POINT_RANGED_BREAK)
> +	  printf_filtered (_("Range "));
>  	printf_filtered (_("Hardware assisted breakpoint %d"), b->number);

This does not work for translation. But that's Ok, because I think
that seeing Hardware with a capital H in "Range Hardware assisted"
seemed a little odd. Also, "range" should qualify breakpoint, so
I think a better way of print the message would be:

    if (is_range_breakpoint)
      printf_filtered (_("Hardware-assisted range breakpoint %d"),
                       b->number);
    else
      [...]

Note that this would become OBE if you elect to use specific
breakpoint_ops.


-- 
Joel


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]