From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x236.google.com (mail-lj1-x236.google.com [IPv6:2a00:1450:4864:20::236]) by sourceware.org (Postfix) with ESMTPS id 5EA053858D35 for ; Sun, 25 Jun 2023 09:21:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5EA053858D35 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-lj1-x236.google.com with SMTP id 38308e7fff4ca-2b44eddb52dso32389651fa.3 for ; Sun, 25 Jun 2023 02:21:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687684887; x=1690276887; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=D27JvS+U5XBqId/om7svaEfOKGfz9QFcTQf6W4z7SWY=; b=sWoxWXDNiLJAt98d54cVq6Ei03OTpa9+T42BcpR3bieDxKsOXKzoYSlzIHKW4oJWU0 yI2OAW6rx+P/BEu2OwwlMJ/AD8yaV2y56SwL8yRB12e9UcKI0NrMVICcP9kt1irM+gNs vdc7sWLaZ5iPdJojKAKJgPb0z79uPjxzJZr2x9bcIjtllnisAmyJTDBCSwG0okDJ1nrK ljCjmhkmD1dSYzZxDEGdb1IhOz/DfSL7kDDtbpkaWrnaUJ5Yq0tYHAY2Fz/3snHTuG6v Q3Lndxgd3ziYSlvcr/1htCEdpeX8rR7iMsz2MQz7qliEmX06yYzCFA8eVZ6unwXJ89Xl wN/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687684887; x=1690276887; 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=D27JvS+U5XBqId/om7svaEfOKGfz9QFcTQf6W4z7SWY=; b=CdQJjAT84rUZX4CM5g8FaAr3uCCsgeVYbtFrrsQzwfHkNAaP50kpO89m3ujj2ykqLV NYTcrZHb/T51UQdZNPU8Y4ubLABhq3lqVX4SEZ2WrNibUIigg2n4t7MLmJaAQO9pRItI M057S3usuOjr0BGHEh2/oP5hifwuMV1035ZPYNpxwyzMD/ngZsOwdMohknUQTtydUr6f 4XfsrgeIk/1y8JNp6Mpn7nZKp7L2y3JgpH7tbxwBJdrvXNYKblLPX5HoNczIqcX9MhOe EXOmwGI8faJMzZs6BznriN4n5dy/3zvvTT1CiNq1dFLe7LU5LMXx+f6/flNE9/curhnJ 6b6g== X-Gm-Message-State: AC+VfDwCv6LFGJrzFWBV/O09t9DEZf1U9Zxvl+ySZ/OgpHGdQQn32QXN 1CRbw5Cx9o5IDUMxD+5D83KtnwLpJUM4iA== X-Google-Smtp-Source: ACHHUZ5Ad0ki+m/slh/pAO4LFeE8ajApWdmrfXBEo0lp1skKF253KfhmE01lVXjHMK75wlm7wIDZQw== X-Received: by 2002:a2e:8805:0:b0:2b5:7fea:b361 with SMTP id x5-20020a2e8805000000b002b57feab361mr8961498ljh.40.1687684887096; Sun, 25 Jun 2023 02:21:27 -0700 (PDT) Received: from fedora.. (78-73-77-63-no2450.tbcn.telia.com. [78.73.77.63]) by smtp.gmail.com with ESMTPSA id f20-20020a2ea0d4000000b002b69efaf188sm112721ljm.67.2023.06.25.02.21.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 25 Jun 2023 02:21:26 -0700 (PDT) From: Simon Farre To: gdb-patches@sourceware.org Cc: tom@tromey.com, eliz@gnu.org, Simon Farre Subject: [PATCH v1] gdb/DAP Introduce new methods to to Pretty Printers Date: Sun, 25 Jun 2023 11:21:16 +0200 Message-ID: <20230625092116.252503-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: 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 | 52 ++++++++++++++++++++++++ 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, 162 insertions(+), 70 deletions(-) diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi index 9cb97a9597d..61a7e49d1df 100644 --- a/gdb/doc/python.texi +++ b/gdb/doc/python.texi @@ -1707,6 +1707,23 @@ 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. +@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. +@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 +1982,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 +2053,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 +2076,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