Re: [PATCH v4] gdb/dap - dataBreakpointInfo & setDataBreakpoints

[Tom Tromey <tom@tromey.com>]

>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> v4.
Simon> Dead imports remained and dead docs on types. Removed

Thank you for the patch.

Simon> Added tests for data breakpoint, both for a simple struct and vars[1]
Simon> which is a std::vector<Var>. To be able to test this, I had to throw in
Simon> the gcc pretty printers file, as I had no other way (it seemed to me) to
Simon> be able to source them from the test.

This is not a good approach because it means the gdb tests are dependent
on the particular implementation of the C++ library.

Instead it's better to write something custom -- like a small C program
that uses a linked list or whatever is convenient, and then also write a
pretty-printer for that data structure.

Simon> I suppose one way to actually make it make
Simon> sense, would be to customize a gdb.Breakpoint (i.e. deriving from it),
Simon> that behaves both as a watchpoint *and* as a finish breakpoint; as soon
Simon> as the finish breakpoint hits, remove the watchpoint.

Non-"location" watchpoints kind of work this way, but TBH they are not
very good.  I think they don't really self-disable, which might be
better.  And AFAIK they don't really work properly with DWARF location
lists.

Simon> +def _info_not_available(desc):
Simon> +    return {"dataId": None, "description": f"{desc}", "accessTypes": None}

This could just be a local function in _databreakpoint_info.
If it is separate it needs some kind of intro comment.

Simon> +
Simon> +@in_gdb_thread
Simon> +def _databreakpoint_info(
Simon> +    name: str, variablesReference: Optional[int] = None, frameId: Optional[int] = None

frameId isn't used.  I wonder if it should be.
It affects what sorts of expressions can be watched.

Simon> +    if variablesReference is not None:
Simon> +        variable = None
Simon> +        # Resolve variable expression until find_variable throws
Simon> +        try:
Simon> +            variable = find_variable(variablesReference)
Simon> +        except Exception as vr_not_found:
Simon> +            return _info_not_available(vr_not_found)

Maybe it'd be better to just wrap the whole body in try/except and
remove some of these smaller ones and extra calls to _info_not_available.

Simon> +    elif variablesReference is None:

I think this can just be 'else' because the previous test is for "not None".
Then the final "return" can be removed.

Simon> +def _sanitize_wp_input(breakpoints: Sequence) -> Sequence:
Simon> +    """Sanitize input so that it plays well with current design."""
Simon> +    res = []
Simon> +    for wp in breakpoints:
Simon> +        obj = {}
Simon> +        obj["condition"] = wp.get("condition")
Simon> +        obj["hitCondition"] = wp.get("hitCondition")
Simon> +        obj["accessType"] = wp.get("accessType")
Simon> +        obj["dataId"] = wp.get("dataId")
Simon> +        res.append(obj)
Simon> +    return res

I don't understand the need for this.  It seems like an identity
function, more or less -- extra fields shouldn't really affect anything.

Simon> +
Simon> +def _wp_type(type: str):

Needs a comment.

Simon> +@in_gdb_thread
Simon> +def _create_watchpoints(dataId, accessType, enabled=True):

This should probably be "_create_watchpoint", since it only makes one.

I don't think 'enabled' is ever used?

Simon> +    wp = gdb.Breakpoint(
Simon> +        spec=f"*{dataId}", type=gdb.BP_WATCHPOINT, wp_class=_wp_type(accessType)

I don't think this will fully work.
The dataId is computed by:

+        dataId = hex(int(gdb_value.address))

However, "*0xADDR" in gdb really means something like "*(int *)0xADDR".
That is, this watchpoint will only watch a single word.

It's tempting to use something like "*ADDR @ N_BYTES" as the expression.
I wonder if that's supported by all the language parsers, though.

Simon>      if isinstance(event, gdb.BreakpointEvent):
Simon>          # Ignore the expected stop, we hit a breakpoint instead.
Simon> -        _expected_stop = StopKinds.BREAKPOINT
Simon> +        if event.breakpoints[0].expression:
Simon> +            _expected_stop = StopKinds.DATABREAKPOINT
Simon> +        else:
Simon> +            _expected_stop = StopKinds.BREAKPOINT

This could also just examine Breakpoint.type which may be more clear.

 
Simon> +    def try_find_member(self, name: str) -> gdb.Value:
Simon> +        type = self.value.type

Needs some sort of doc string.

Simon> +        # Non-pretty printed values more common. Search type definition first
Simon> +        for field in type.fields():
Simon> +            if field.name == name:
Simon> +                return self.value[field]

I think it's better to just always use the wrapping pretty-printer and
just delete this code.

One way this could matter is if we fix the "multiple variables with the
same name" problem that I think can arise in some C++ inheritance
scenarios -- the printer might rename the fields for presentation
purposes.

Simon> +        for (child_name, value) in self.printer.children():
Simon> +            if child_name == name:
Simon> +                return value

It's possible the children are already cached.  I sent some patches to
change this area a little, but I suppose the way it should work is to
examine the cache if it exists.

This is an area where we could add random-access-by-name to the printer
extension methods.  Though that would require docs.

Tom