From patchwork Thu May 16 18:01:11 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom Tromey X-Patchwork-Id: 90304 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 23AB8384646C for ; Thu, 16 May 2024 18:01:51 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-io1-xd34.google.com (mail-io1-xd34.google.com [IPv6:2607:f8b0:4864:20::d34]) by sourceware.org (Postfix) with ESMTPS id B3C2D384AB42 for ; Thu, 16 May 2024 18:01:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B3C2D384AB42 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org B3C2D384AB42 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::d34 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715882479; cv=none; b=GDEEFDFbOwltUFEvbUNS0CWRTbEPvHFbqdEkI2m2jziuriTiDHn2yk4jogPNYrp1rbH90RU4hUci2emWLc7C/MYGc+abr4d6tKSZ47tSaiFWekTG3qsScbUckEOeK1hsp3vVAKx4WSYqfnJcZiFdpC+qdcazds/BjXX+kk8ZoEg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715882479; c=relaxed/simple; bh=FB/piAzB0Fh5BvW7S82uDTd8ruV4k7k3m9H/Q2J1tH8=; h=DKIM-Signature:From:Date:Subject:MIME-Version:Message-Id:To; b=lhRfJuwZlDiTChBdJWmrCy4PDTT6iAxPyG6ipu7/3TOxA3ZffZwkEHprm7/SENqZ7TI3RiySdmnIohBaGYBt8nSRV9bLweCTtq+4P8RNw/HZtDrKuFbQdKMjB8bZEcm353xO59i+qA41fv5pNYLz5GHR7vaAw1/oqCRTBLH4LVk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-io1-xd34.google.com with SMTP id ca18e2360f4ac-7e21742025aso58283039f.2 for ; Thu, 16 May 2024 11:01:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=adacore.com; s=google; t=1715882476; x=1716487276; darn=sourceware.org; h=to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=K3syEcd2+4zRGNoMO1gf+rxqspKrvh4Uvkhdh/aXivY=; b=T5LioF173MPlzekyVcRVUHZtET7Yelo5VRcSErCefWV1GxsdASEi4dO68JKrWdKfZi 99JVkIgtNNiXXtnhoFcZuzVvsmH70U+Mtmuh0v5fLrmFZGLdKo/NR0j9oPYFNUAgtQ6o u/M3lzNdI3IqfG/HKf8YUB11GV3f0d0FG+mszTkK8M5EK2NrPojcf1xtuzP9u6pLV2Qb YEwsMwAdhOaRtJFvuUSuRVEz2JmWhGG+xSP768MgFHWBD5aSpqwx7LLO9eJjFrlXLLGK gUGxsH64R5EW0sli2iTPOT7dUL33WKXrPGHFo2Y6dINIYzucNXAVKXHYs94yVxLPz0VP HGTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715882476; x=1716487276; h=to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=K3syEcd2+4zRGNoMO1gf+rxqspKrvh4Uvkhdh/aXivY=; b=LbXfUsqj1O42yTB0/xUjuAvAKClANwUbBJzIN2YwYWjkgA50qSDLxW8OReR0ijLSbA VOBfaFYBR+iWOgkS4ex36m8M/KKv7gnKVZnLTSieGxFqXS6prOtHKmixT46lVZeU4EMh JeFJqG1A8Ks7h4YDU+JXTbav427v38Prm7zS4clqrft0yOCAQjZM39sRFDbdlOyzjKfy nxjvf6ogUOAc/UQ2WwPhVTeyRL57YsAwii1LE/qLphglCYzHxKP+3XpATNObXkWlCzmO XBBPVRSpvGdDKqaHejF3aDvtlee9orfYvPhbODQmlBYzS6JQ7c8uS+KeeLgaQaTNHzZl CN1Q== X-Gm-Message-State: AOJu0YweC59wiQ8dkQj3wEI78XwgqTxdBkyznRTo/uo9j0D4FVr/avNg LL9x/vC6yv0kgn+ui9HNqNO2pzU7Owy+vAxa3z8s2NkxymlddnZcSrZHckUTuN8a0FxUGQF8B2E = X-Google-Smtp-Source: AGHT+IH4jgQfRyiu/7f77h3n73fQbEOMcSqct29uCbGfjjHUPf9QVZxBE5MkAjVByEd+Gkkr8FI68g== X-Received: by 2002:a05:6e02:1565:b0:36d:b604:7930 with SMTP id e9e14a558f8ab-36db6047c1fmr68056615ab.14.1715882472939; Thu, 16 May 2024 11:01:12 -0700 (PDT) Received: from localhost.localdomain (75-166-134-4.hlrn.qwest.net. [75.166.134.4]) by smtp.gmail.com with ESMTPSA id 8926c6da1cb9f-489376dc936sm4287426173.142.2024.05.16.11.01.12 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 May 2024 11:01:12 -0700 (PDT) From: Tom Tromey Date: Thu, 16 May 2024 12:01:11 -0600 Subject: [PATCH 1/3] Memoize gdb.Block and make them hashable MIME-Version: 1.0 Message-Id: <20240516-dap-global-scope-v1-1-07c493009505@adacore.com> References: <20240516-dap-global-scope-v1-0-07c493009505@adacore.com> In-Reply-To: <20240516-dap-global-scope-v1-0-07c493009505@adacore.com> To: gdb-patches@sourceware.org X-Mailer: b4 0.13.0 X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org In subsequent patches, it's handy if gdb.Block is hashable, so it can be stored in a set or a dictionary. However, doing this in a straightforward way is not really possible, because a block isn't truly immutable -- it can be invalidated. And, while this isn't a real problem for my use case (in DAP the maps are only used during a single stop), it seemed error-prone. This patch instead takes the approach of using the gdb.Block's own object identity to allow hashing. This seems fine because the contents don't affect the hashing. In order for this to work, though, the blocks have to be memoized -- two requests for the same block must return the same object. This also allows (actually, requires) the simplification of the rich-compare method for blocks. Reviewed-By: Alexandra Petlanova Hajkova --- gdb/python/py-block.c | 143 +++++++++++++++++++--------------- gdb/testsuite/gdb.python/py-block.exp | 4 + 2 files changed, 83 insertions(+), 64 deletions(-) diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c index 3de6200e7c2..62e93d55072 100644 --- a/gdb/python/py-block.c +++ b/gdb/python/py-block.c @@ -31,10 +31,6 @@ struct block_object { between a block and an object file. When a block is created also store a pointer to the object file for later use. */ struct objfile *objfile; - /* Keep track of all blocks with a doubly-linked list. Needed for - block invalidation if the source object file has been freed. */ - block_object *prev; - block_object *next; }; struct block_syms_iterator_object { @@ -76,32 +72,9 @@ struct block_syms_iterator_object { } \ } while (0) -/* This is called when an objfile is about to be freed. - Invalidate the block as further actions on the block would result - in bad data. All access to obj->symbol should be gated by - BLPY_REQUIRE_VALID which will raise an exception on invalid - blocks. */ -struct blpy_deleter -{ - void operator() (block_object *obj) - { - while (obj) - { - block_object *next = obj->next; - - obj->block = NULL; - obj->objfile = NULL; - obj->next = NULL; - obj->prev = NULL; - - obj = next; - } - } -}; - extern PyTypeObject block_syms_iterator_object_type CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("block_syms_iterator_object"); -static const registry::key +static const registry::key blpy_objfile_data_key; static PyObject * @@ -278,42 +251,54 @@ blpy_getitem (PyObject *self, PyObject *key) return nullptr; } +/* Deleter function for the hash table. */ + static void -blpy_dealloc (PyObject *obj) +block_object_del (void *obj) { block_object *block = (block_object *) obj; + block->block = nullptr; + block->objfile = nullptr; +} - if (block->prev) - block->prev->next = block->next; - else if (block->objfile) - blpy_objfile_data_key.set (block->objfile, block->next); - if (block->next) - block->next->prev = block->prev; - block->block = NULL; - Py_TYPE (obj)->tp_free (obj); +/* Hash function for the hash table. */ + +static hashval_t +block_object_hash (const void *obj) +{ + const block_object *block = (const block_object *) obj; + return htab_hash_pointer (block->block); +} + +/* Equality function for the hash table. Note that searches must be + done with a plain block. */ + +static int +block_object_eq (const void *a, const void *b) +{ + const block_object *blocka = (const block_object *) a; + const block *blockb = (const block *) b; + return blocka->block == blockb; } -/* Given a block, and a block_object that has previously been - allocated and initialized, populate the block_object with the - struct block data. Also, register the block_object life-cycle - with the life-cycle of the object file associated with this - block, if needed. */ +/* Called when a gdb.Block is destroyed. This removes it from the + hash. */ + static void -set_block (block_object *obj, const struct block *block, - struct objfile *objfile) +blpy_dealloc (PyObject *obj) { - obj->block = block; - obj->prev = NULL; - if (objfile) + block_object *block = (block_object *) obj; + + if (block->objfile != nullptr) { - obj->objfile = objfile; - obj->next = blpy_objfile_data_key.get (objfile); - if (obj->next) - obj->next->prev = obj; - blpy_objfile_data_key.set (objfile, obj); + htab_t table = blpy_objfile_data_key.get (block->objfile); + hashval_t hash = block_object_hash (block); + /* This will clear the contents of the block as a side + effect. */ + htab_remove_elt_with_hash (table, block->block, hash); } - else - obj->next = NULL; + + Py_TYPE (obj)->tp_free (obj); } /* Create a new block object (gdb.Block) that encapsulates the struct @@ -321,13 +306,32 @@ set_block (block_object *obj, const struct block *block, PyObject * block_to_block_object (const struct block *block, struct objfile *objfile) { - block_object *block_obj; + htab_t table = blpy_objfile_data_key.get (objfile); + if (table == nullptr) + { + table = htab_create_alloc (10, block_object_hash, block_object_eq, + block_object_del, xcalloc, xfree); + blpy_objfile_data_key.set (objfile, table); + } + + hashval_t hash = htab_hash_pointer (block); + block_object *result = (block_object *) htab_find_with_hash (table, block, + hash); + if (result != nullptr) + { + PyObject *py_result = (PyObject *) result; + Py_INCREF (py_result); + return py_result; + } + + result = PyObject_New (block_object, &block_object_type); + result->block = block; + result->objfile = objfile; - block_obj = PyObject_New (block_object, &block_object_type); - if (block_obj) - set_block (block_obj, block, objfile); + void **slot = htab_find_slot_with_hash (table, block, hash, INSERT); + *slot = result; - return (PyObject *) block_obj; + return (PyObject *) result; } /* Return struct block reference that is wrapped by this object. */ @@ -452,6 +456,20 @@ blpy_repr (PyObject *self) name, str.c_str ()); } +/* Hash function for block objects. */ + +static Py_hash_t +blpy_hash (PyObject *self) +{ + /* Python doesn't really expose its pointer hash function, so we use + our own. */ + Py_hash_t result = (Py_hash_t) htab_hash_pointer (self); + /* -1 has a special meaning for Python. */ + if (result == -1) + result = -2; + return result; +} + /* Implements the equality comparison for Block objects. All other comparison operators will throw NotImplemented, as they aren't valid for blocks. */ @@ -466,10 +484,7 @@ blpy_richcompare (PyObject *self, PyObject *other, int op) return Py_NotImplemented; } - block_object *self_block = (block_object *) self; - block_object *other_block = (block_object *) other; - - bool expected = self_block->block == other_block->block; + bool expected = self == other; bool equal = op == Py_EQ; return PyBool_FromLong (equal == expected); } @@ -542,7 +557,7 @@ PyTypeObject block_object_type = { 0, /*tp_as_number*/ 0, /*tp_as_sequence*/ &block_object_as_mapping, /*tp_as_mapping*/ - 0, /*tp_hash */ + blpy_hash, /*tp_hash */ 0, /*tp_call*/ 0, /*tp_str*/ 0, /*tp_getattro*/ diff --git a/gdb/testsuite/gdb.python/py-block.exp b/gdb/testsuite/gdb.python/py-block.exp index 99642a546a7..0e6851ddf8b 100644 --- a/gdb/testsuite/gdb.python/py-block.exp +++ b/gdb/testsuite/gdb.python/py-block.exp @@ -119,8 +119,12 @@ gdb_test "python print (block.is_valid())" "True" \ "Check block validity" gdb_test "python print (block_iter.is_valid())" "True" \ "Check block_iter validity" +gdb_test_no_output "python x = hash(block)" "block is hashable" +gdb_test "python print (type (x))" "" "block hash is integer" gdb_unload gdb_test "python print (block.is_valid())" "False" \ "Check block validity after unload" gdb_test "python print (block_iter.is_valid())" "False" \ "Check block_iter validity after unload" +gdb_test "python print (hash (block) == x)" "True" \ + "block hash did not change"