From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com [IPv6:2a00:1450:4864:20::12f]) by sourceware.org (Postfix) with ESMTPS id 3D7133858D20 for ; Mon, 26 Jun 2023 08:14:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3D7133858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lf1-x12f.google.com with SMTP id 2adb3069b0e04-4f004cc54f4so3662025e87.3 for ; Mon, 26 Jun 2023 01:14:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687767272; x=1690359272; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=BtCevkfQnABhg+zFfDV7truGZ/BzsznmSlTupQzIUFQ=; b=g7G25AsFLjUwx0VahAqWWduKlRYdL9RAD6B9dnPyt/1I3evD1CGDyNyfPvPuEU2vWR RfU3oiscS/QMVjrpfOl9ZbRZ1xZZYuE5mQpRshf7j1x5FMnTNeyLy/3u/GHNc61lltRo uEGL0JYweOM1OnWZlWvOVB+mdl7PwY3CpJ/4y1Xi+5IUTmWxihIfrj9WXegMy9ky/oJX mmTLLxtZdPCeTY++AdwaPSbK7l84H2STSSQVDvnPyDKuUL7jQWkoH2OzKIkjO+rEZazl VIWGcDVaolq2ed0AbReFw49B2S8AX1Y53le0+bYJsPUZg1wOOFiM1K/ufGUp06LmLjQK 78/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687767272; x=1690359272; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=BtCevkfQnABhg+zFfDV7truGZ/BzsznmSlTupQzIUFQ=; b=kox5pn0z6VSmea37IQwf9hPIg7QuPhrCV5UNHQByQVOwoEQ2C7LR5Uw53dWDHZNO0P DocKHKnUoYtwtIX5G8vPqm79as9Kn0Dujxeod9OjJfa2R70Zs23sKPnidZOiWPGZ7yQu a7d+4tV4rjwDbyzMVBKmiKiy0jA56QsFu6yfDJJzz6JBsbNJWhXZMWYgC8MbyJWKKJ3u eIzL/g7wxIpMXC6N+1IAtviVY+k8Yhxx8R+1vd8NsMO9yyhGpTWoQM6J0jPL8+l94BnH hs0msHG6h3hYOK5UkSzA7QsyEdXdGTxkPEWgqLzQxxRl1L262Hgo4+bMqgT8vbvtHJ7J o4jg== X-Gm-Message-State: AC+VfDxRPXaxBb5ID7q7nHALKbHVYRhvlm+hyx0/PwalcUbV06mgJmgz NNwtMZSRfeKr4OdxSaDTjTB2kfADr3sDHA== X-Google-Smtp-Source: ACHHUZ7ayrzizOhBJp/JY+fJx2lLGwNe0nUgS+Vr//vJ34y5TnwH8DfsdwFGt+TDaMS28S6qKmgBeQ== X-Received: by 2002:a05:6512:280b:b0:4fa:b380:4c03 with SMTP id cf11-20020a056512280b00b004fab3804c03mr2299327lfb.56.1687767271799; Mon, 26 Jun 2023 01:14:31 -0700 (PDT) Received: from fedora.. (78-73-77-63-no2450.tbcn.telia.com. [78.73.77.63]) by smtp.gmail.com with ESMTPSA id j18-20020ac24552000000b004f858249931sm786588lfm.93.2023.06.26.01.14.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 Jun 2023 01:14:31 -0700 (PDT) From: Simon Farre To: gdb-patches@sourceware.org Cc: tom@tromey.com, eliz@gnu.org, Simon Farre Subject: [PATCH v2] gdb/DAP Introduce new methods to to Pretty Printers Date: Mon, 26 Jun 2023 10:14:19 +0200 Message-ID: <20230626081419.8789-1-simon.farre.cx@gmail.com> X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: v2. I've added some additional notes as per Eli's Request to the documentation and fixed formatting. v1. This patch introduces the `num_children` method to the NoOpArray-pretty printer as well as fixing a bug where some types accidentally uses NoOpScalar pretty printers. It also cleans up the retrieval of variables during a "variables" request (or an "evaluate" request) reducing the complexity. Caching of variables would be fine if we actually kept variables around during step/continues that stops inside the same frame we're currently at - this is how I did it for my DA-implementation Midas, to be able to be faster than most other DA:s; meaning, I kept the `gdb.Value`s around until we've left the frame they were created in. However, I think this breaks with the DAP-specification that states that variableReferences should only ever be valid "during a stop". So unless we actually build some "remembering mechanism" that keeps variableReferences and invalidates them only once we figure out we're not in the same frame anymore, caching the values (children) is not important. Regardless, that kind of functionality would rely on mechanisms other than the ones I've removed here. Going forward, Pretty Printers of "array type" should be expected (though not required) to provide the `num_children` method, in order to be able to provide a performant DAP-experience. They should also provide `children_range`, a method meant to retrieve a sub-range of children of a Pretty Printer wrapped variable without having to iterate over all prior elements of the range. This method is appropriate on pretty printers that wrap std::vector, arrays, etc. Even though these methods are not required, there's no good reason to not implement them, because it provides an absolutely astronomical performance difference. I've provided the method for the NoOpArrayPrinter. After hacking the std-library pretty printer for std::vector to provide these two methods, servicing one "variables" request of the "Locals" scope where a std::vector of 10k elements exist, went from 35 seconds (!!!) down to 0.4 ms. For a 10k element of std::vector it went from 100 ms down to the same 0.4ms, also. The same went for retrieving a sub-range of elements 9800-9900; it went from 35 seconds (due to having to iterate over _all_ prior children) down to 350ms, for std::vector - the still-very-high time count is because of std::string's Pretty Printer implementation which is incredibly slow. For the std::vector, retrieving that same sub-range was in the ~10 ms range. These two new optional methods have been added to documentation as well as an example that mimicks the layout of `std::vector`, to show an example of how they can be implemented. --- gdb/doc/python.texi | 55 +++++++++++++++++++++++++ gdb/python/lib/gdb/dap/scopes.py | 70 ++++++++++++++++++++++++-------- gdb/python/lib/gdb/dap/varref.py | 68 +++++++++---------------------- gdb/python/lib/gdb/printing.py | 42 +++++++++++++++++-- 4 files changed, 165 insertions(+), 70 deletions(-) diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi index 9cb97a9597d..eddc1a96a94 100644 --- a/gdb/doc/python.texi +++ b/gdb/doc/python.texi @@ -1707,6 +1707,26 @@ Children may be hidden from display based on the value of @samp{set print max-depth} (@pxref{Print Settings}). @end defun +@defun pretty_printer.children_range (self, start, end) +This method is optional but @value{GDBN} will call this method to +be able to more effeciently compute a sub-range of children +of the pretty-printers value. If the pretty printer is for an +array-like type, it is recommended that this method is implemented +as the DAP interpreter will make extended use of it. For an example on how +to best implement this method @pxref{Writing a Pretty-Printer}. Not +implementing this and @code{num_children} can have drastic effects on +the performance of a DAP-interpreter debug session. If this method is not +implemented GDB will do eager evaluation of variables in a DAP-session +which will come at a performance cost. +@end defun + +@defun pretty_printer.num_children (self) +@value{GDBN} can call this method to query how many children this pretty +printer will return from a @code{pretty_printer.children} call. This method +is used by the DAP interpreter to be able to display paginated results, as +well as use @code{pretty_printer.children_range} to be more effecient. +@end defun + @defun pretty_printer.display_hint (self) The CLI may call this method and use its result to change the formatting of a value. The result will also be supplied to an MI @@ -1965,10 +1985,43 @@ These are the types we are going to pretty-print: @smallexample struct foo @{ int a, b; @}; struct bar @{ struct foo x, y; @}; +struct vec @{ foo* start; foo* current; foo* end; @} + @end smallexample Here are the printers: +@smallexample +class vecPrinter: + """Print a vec of foo objects.""" + + def __init__(self, val): + self.val = val + + def to_string(self): + return "" + + def num_children(self): + first = self.val["start"] + last = self.val["current"] + return last - first + + def children_range(self, start, end): + ptr = self.val["start"] + for i in range(start, end): + yield (str(i), (ptr + i).dereference()) + + def children(self): + ptr = self.val["start"] + size = int(self.num_children()) + for i in range(0, size): + yield (str(i), (ptr + i).dereference()) + + def display_hint(self): + return "array" + +@end smallexample + @smallexample class fooPrinter: """Print a foo object.""" @@ -2003,6 +2056,7 @@ def build_pretty_printer(): "my_library") pp.add_printer('foo', '^foo$', fooPrinter) pp.add_printer('bar', '^bar$', barPrinter) + pp.add_printer('vec', '^vec$', barPrinter) return pp @end smallexample @@ -2025,6 +2079,7 @@ my_library.so: my_library foo bar + vec @end smallexample @node Type Printing API diff --git a/gdb/python/lib/gdb/dap/scopes.py b/gdb/python/lib/gdb/dap/scopes.py index 9b80dd9ce80..10d9a5a6b97 100644 --- a/gdb/python/lib/gdb/dap/scopes.py +++ b/gdb/python/lib/gdb/dap/scopes.py @@ -49,48 +49,76 @@ def _block_vars(block): return (args, locs) +class ScopePrinter: + def __init__(self, frame, variables): + self.variables = variables + self.frame = frame + + def to_string(self): + return "" + + @in_gdb_thread + def children(self): + for sym in self.variables: + if sym.needs_frame: + val = sym.value(self.frame) + else: + val = sym.value() + yield ((sym.print_name, val)) + + def num_children(self): + return len(self.variables) + + class ScopeReference(BaseReference): - def __init__(self, name, hint, frame, var_list): + def __init__(self, name, hint, frame, printer): super().__init__(name) self.hint = hint self.frame = frame self.func = frame.function() - self.var_list = var_list + self.printer = printer def to_object(self): result = super().to_object() result["presentationHint"] = self.hint # How would we know? result["expensive"] = False - result["namedVariables"] = len(self.var_list) + result["namedVariables"] = self.printer.num_children() if self.func is not None: result["line"] = self.func.line # FIXME construct a Source object return result def child_count(self): - return len(self.var_list) + return self.printer.num_children() + + +class RegisterFilePrinter: + def __init__(self, frame, registers): + self.registers = registers + self.frame = frame + + def to_string(self): + return "" @in_gdb_thread - def fetch_one_child(self, idx): - sym = self.var_list[idx] - if sym.needs_frame: - val = sym.value(self.frame) - else: - val = sym.value() - return (sym.print_name, val) + def children(self): + for reg in self.registers: + yield (reg.name, self.frame.read_register(reg)) + + def num_children(self): + return len(self.registers) class RegisterReference(ScopeReference): def __init__(self, name, frame): super().__init__( - name, "registers", frame, list(frame.architecture().registers()) + name, + "registers", + frame, + RegisterFilePrinter(frame, list(frame.architecture().registers())), ) - @in_gdb_thread - def fetch_one_child(self, idx): - return (self.var_list[idx].name, self.frame.read_register(self.var_list[idx])) - # Helper function to create a DAP scopes for a given frame ID. @in_gdb_thread @@ -101,9 +129,15 @@ def _get_scope(id): if block is not None: (args, locs) = _block_vars(block) if args: - scopes.append(ScopeReference("Arguments", "arguments", frame, args)) + scopes.append( + ScopeReference( + "Arguments", "arguments", frame, ScopePrinter(frame, args) + ) + ) if locs: - scopes.append(ScopeReference("Locals", "locals", frame, locs)) + scopes.append( + ScopeReference("Locals", "locals", frame, ScopePrinter(frame, locs)) + ) scopes.append(RegisterReference("Registers", frame)) return [x.to_object() for x in scopes] diff --git a/gdb/python/lib/gdb/dap/varref.py b/gdb/python/lib/gdb/dap/varref.py index 213151fd3d3..bc73db7f45d 100644 --- a/gdb/python/lib/gdb/dap/varref.py +++ b/gdb/python/lib/gdb/dap/varref.py @@ -17,7 +17,7 @@ import gdb from .startup import in_gdb_thread from .server import client_bool_capability from abc import abstractmethod - +from itertools import islice # A list of all the variable references created during this pause. all_variables = [] @@ -73,19 +73,9 @@ class BaseReference: """Call this to declare that this variable or scope has no children.""" self.ref = 0 - @abstractmethod - def fetch_one_child(self, index): - """Fetch one child of this variable. - - INDEX is the index of the child to fetch. - This should return a tuple of the form (NAME, VALUE), where - NAME is the name of the variable, and VALUE is a gdb.Value.""" - return - @abstractmethod def child_count(self): - """Return the number of children of this variable.""" - return + return None @in_gdb_thread def fetch_children(self, start, count): @@ -95,15 +85,19 @@ class BaseReference: COUNT is the number to return, with 0 meaning return all.""" if count == 0: count = self.child_count() - if self.children is None: - self.children = [None] * self.child_count() result = [] - for idx in range(start, start + count): - if self.children[idx] is None: - (name, value) = self.fetch_one_child(idx) - self.children[idx] = VariableReference(name, value) - result.append(self.children[idx]) - return result + stop = start + count if count is not None else None + + if hasattr(self.printer, "children_range"): + # Performant path + for (name, value) in self.printer.children_range(start, start + count): + result.append(VariableReference(name, value)) + return result + else: + # The extremely slow path, if type is a "array-like" + for (name, value) in islice(self.printer.children(), start, stop): + result.append(VariableReference(name, value)) + return result class VariableReference(BaseReference): @@ -124,35 +118,17 @@ class VariableReference(BaseReference): self.child_cache = None if not hasattr(self.printer, "children"): self.no_children() - self.count = None - else: - self.count = -1 - - def cache_children(self): - if self.child_cache is None: - # This discards all laziness. This could be improved - # slightly by lazily evaluating children, but because this - # code also generally needs to know the number of - # children, it probably wouldn't help much. A real fix - # would require an update to gdb's pretty-printer protocol - # (though of course that is probably also inadvisable). - self.child_cache = list(self.printer.children()) - return self.child_cache def child_count(self): - if self.count is None: - return None - if self.count == -1: - if hasattr(self.printer, "num_children"): - num_children = self.printer.num_children - else: - num_children = len(self.cache_children()) - self.count = num_children - return self.count + if hasattr(self.printer, "num_children"): + return self.printer.num_children() + return None def to_object(self): result = super().to_object() - result[self.result_name] = self.printer.to_string() + result[self.result_name] = ( + str(self.value) if self.ref == 0 else str(self.printer.to_string()) + ) num_children = self.child_count() if num_children is not None: if ( @@ -170,10 +146,6 @@ class VariableReference(BaseReference): result["type"] = str(self.value.type) return result - @in_gdb_thread - def fetch_one_child(self, idx): - return self.cache_children()[idx] - @in_gdb_thread def find_variable(ref): diff --git a/gdb/python/lib/gdb/printing.py b/gdb/python/lib/gdb/printing.py index 353427d000a..a5ff5a6d96c 100644 --- a/gdb/python/lib/gdb/printing.py +++ b/gdb/python/lib/gdb/printing.py @@ -278,6 +278,9 @@ class NoOpScalarPrinter: def to_string(self): return self.value.format_string(raw=True) + def num_children(self): + return 0 + class NoOpArrayPrinter: """A no-op pretty printer that wraps an array value.""" @@ -287,9 +290,6 @@ class NoOpArrayPrinter: (low, high) = self.value.type.range() self.low = low self.high = high - # This is a convenience to the DAP code and perhaps other - # users. - self.num_children = high - low + 1 def to_string(self): return "" @@ -301,6 +301,13 @@ class NoOpArrayPrinter: for i in range(self.low, self.high + 1): yield (i, self.value[i]) + def num_children(self): + return self.high - self.low + 1 + + def children_range(self, start, end): + for i in range(start, end): + yield (i, self.value[start]) + class NoOpStructPrinter: """A no-op pretty printer that wraps a struct or union value.""" @@ -316,6 +323,33 @@ class NoOpStructPrinter: if field.name is not None: yield (field.name, self.value[field]) + def num_children(self): + return len( + [ + x + for x in self.value.type.fields() + if x.name is not None and hasattr(x, "bitpos") + ] + ) + + +def is_structured_type(type): + """Attempts to find out if TYPE points to something that's structured, + like a struct or a class. Unfortunately type codes are not represented + as bitfields. This has the unfortunate side effect of e.g. + TYPE_CODE_TYPEDEF not being the same as TYPE_CODE_STRUCT, even if that + typedef resolves to a struct.""" + try: + fields = [ + x for x in type.fields() if x.name is not None and hasattr(x, "bitpos") + ] + return len(fields) > 0 or type.code in ( + gdb.TYPE_CODE_STRUCT, + gdb.TYPE_CODE_UNION, + ) + except TypeError: + return False + def make_visualizer(value): """Given a gdb.Value, wrap it in a pretty-printer. @@ -331,7 +365,7 @@ def make_visualizer(value): result = gdb.printing.NoOpArrayPrinter(value) (low, high) = value.type.range() result.n_children = high - low + 1 - elif value.type.code in (gdb.TYPE_CODE_STRUCT, gdb.TYPE_CODE_UNION): + elif is_structured_type(value.type): result = gdb.printing.NoOpStructPrinter(value) else: result = gdb.printing.NoOpScalarPrinter(value) -- 2.41.0