From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 28BFA3858D39 for ; Mon, 3 Apr 2023 19:06:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 28BFA3858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [10.0.0.170] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 8C0F61E0D2; Mon, 3 Apr 2023 15:06:22 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1680548782; bh=9jUzTHkpi6rAxmLCd/fUFgZJxKYQJjle1v5xtHuf7l0=; h=Date:Subject:To:References:From:In-Reply-To:From; b=aNsSsxbP1bPtIJwF78DuVSMkx4KLhfnw6xYkWe6n8hMybj29ykbgWxKh9tejZ6V52 /DDWiwoTx6XunE/LY5IAT6NVIGF/EOTkEl8DveDRxG1Tv4HZFTuGrJ99MFScYWykCL AF5Md99kiwY6jG/11gyBScmw/8SLxAXxqXlh5AmQ= Message-ID: <526e6b42-9bfc-17c8-6a0b-8581bf527af7@simark.ca> Date: Mon, 3 Apr 2023 15:06:22 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class. Content-Language: fr To: "Willgerodt, Felix" , "gdb-patches@sourceware.org" References: <20230321154626.448816-1-felix.willgerodt@intel.com> <20230321154626.448816-6-felix.willgerodt@intel.com> From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_PASS,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 List-Id: On 3/31/23 06:58, Willgerodt, Felix wrote: >> -----Original Message----- >> From: Simon Marchi >> Sent: Freitag, 24. März 2023 15:28 >> To: Willgerodt, Felix ; gdb- >> patches@sourceware.org >> Subject: Re: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class. >> >> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote: >>> @@ -163,6 +165,14 @@ btpy_insn_or_gap_new (thread_info *tinfo, >> Py_ssize_t number) >>> return recpy_gap_new (err_code, err_string, number); >>> } >>> >>> + const struct btrace_insn *insn = btrace_insn_get (&iter); >>> + gdb_assert (insn != nullptr); >>> + >>> + if (insn->iclass == BTRACE_INSN_AUX) >>> + return recpy_aux_new >>> + (iter.btinfo->aux_data.at (insn->aux_data_index).c_str (), number); >> >> Not really important, but recpy_aux_new could take a >> `const std::string &`, it would simplify callers a bit. > > Thanks, I will do that. > >>> + >>> + >> >> Remove one newline. > > Done. > >>> +/* Implementation of Auxiliary.data [str]. */ >>> + >>> +static PyObject * >>> +recpy_aux_data (PyObject *self, void *closure) >>> +{ >>> + const recpy_aux_object * const obj = (const recpy_aux_object *) self; >>> + >>> + return PyUnicode_FromString (obj->data); >>> +} >> >> Nothing new with this patch, since this is the same pattern used for >> other object, but just wondering: obj->data seems to be borrowed from >> the btrace backend. Are there some lifetime issues if you do: >> >> 1. Create a gdb.RecordAuxiliary object "b" >> 2. Issue a gdb command to clear the btrace data >> 3. Access "b.data" >> >> ? It seems to me like obj->data might point to freed data. > > About the pattern: > There is already "maint btrace clear" so I could test and debug this with > master. On master GDB prints an error before accessing any of these objects, > as we check if the trace is empty before accessing anything. Then, maybe if you record something after clearing the data? Something like: 1. Create a gdb.RecordAuxiliary object "b" 2. Issue "maint btrace clear" 3. Do one step (to make the trace non-empty again) 4. Access "b.data" > With my patch I could however see a nullptr access. But not at this part of the code. > I think I need to rethink the modelling of Auxiliary objects as new objects. > We model them as a class of instruction in btrace.c. > And the python lists we put them in are designed to be a list of a specific type of object. Which list are you referring to? Simon