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 E51263840C0F for ; Sun, 21 Jun 2020 03:32:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E51263840C0F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (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 BED5E1E793; Sat, 20 Jun 2020 23:32:57 -0400 (EDT) Subject: Re: [PATCH v2 3/3] gdb/jit: enable tracking multiple jitter objfiles To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: <4f7d8cfd9635484c5148e72f0941a0d9f369b7f3.1592299502.git.tankut.baris.aktemur@intel.com> From: Simon Marchi Message-ID: <40e500a7-81ab-280a-1e40-d737d9aeb93f@simark.ca> Date: Sat, 20 Jun 2020 23:32:56 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: <4f7d8cfd9635484c5148e72f0941a0d9f369b7f3.1592299502.git.tankut.baris.aktemur@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 21 Jun 2020 03:33:00 -0000 On 2020-06-16 5:49 a.m., Tankut Baris Aktemur wrote: > objf_data = get_jit_objfile_data (reg_symbol.objfile); > objf_data->register_code = reg_symbol.minsym; > objf_data->descriptor = desc_symbol.minsym; This made me realize that we now have per-JITer-objfile information in both jit_objfile_data and the new jiter_and_bp structure. I think it would be more consistent to bring it all in one of them. I see two way forward: 1. Move register_code and descriptor from jit_objfile_data to jiter_and_bp. jiter_and_bp becomes the source of truth for information about JITer objfiles. jit_objfile_data would only be used to decribe JITed objfiles. 2. Move the content of jiter_and_bp to jit_objfile_data. If we still think it's useful to have a list of JITer objfiles, jit_program_space_data::jiter_and_bps would become a list of objfiles known to be JITers: /* Objfiles in this program space that define the JIT interface symbols. */ std::forward_list jiters; I would prefer #2, because using registries is kind of our standard way to keep per-stuff data (where stuff is objfile, program_space, inferior, etc). And instead of having the "jiters" list, when we want to iterate on jiters, maybe we can just iterate on progspace->objfiles () and check those objfiles whose jit_objfile_data indicate they are JITers. The tradeoff is: it's a bit less efficient, but the code is simpler (one less list to maintain, that is possibly out of sync). If the operations that require iterating on the existing JITers are not on some hot path, it should be fine. While digging into this, there are two improvements I'd make (orthogonal to this patch): - Make jit.c use the newer / type-safe / c++-friendly objfile_key instead of objfile_data. - Split jit_objfile_data in two: one type for the JITers and one type for the JITed. Even today, it is used for both, and that's kind of confusing. I ended up trying all of this to make sure it made sense, instead of just throwing ideas out there, so I took the time to make a "clean" branch. See here: https://github.com/simark/binutils-gdb/tree/multiple-jiters Please tell me what you think about this. If you agree with the direction, I could officially post it to the list. There's one more thing I would improve (and I think it would also apply to your version of the patch): in jit_breakpoint_re_set_internal, we keep looking up symbols in the same libraries over and over. Maybe we could mark the objfiles to say that we have already looked up the symbols, as there's no point in looking them up again in the future. Either an objfile doesn't have them, and therefore will never have them. Or it has them and we already have them. We could then just skip to the step where we check if the relocated address has changed. Simon