From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id ADADD3858D20 for ; Wed, 1 May 2024 14:56:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org ADADD3858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org ADADD3858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714575415; cv=none; b=niNR8oh8iWbGsfgmUDj7M7uO2W/+KR8f0Fo69sJ2tC6b64mg+IcgcDG3LuVNtcJ/h4AcdgxqAnS8zSUMHFUrBAPJd/xhf9psXjI6vBit39DtLnZPH53p1bxh391/HDxYX3DHoD0T3ALuZVeT0k1N5lEUYEfZSPl/AIPKGoDBgzE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714575415; c=relaxed/simple; bh=g1GxvrRosAEViMVwb4LOr2zNaEHbDNq1ulV/0XUJoM8=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=iSUucQ8E+zwKOdn8WjwxcteclgRxoOgj3il1QMhBRTPBeKn//hOG1lQ4bxp9tckuY7e+xpdzozmoLMk0iJqWuYfh7FfeIt1VhPLPnP1Af/SPFvHF1xutn9XO9eQy/1Ldrd35kQsILeSgAQu3NIRO2Y4hRKYUv/sOK+vo5Z03Ow4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1714575413; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=EcrWqbo8TecxfuyLftqXVzeuf9igLI8sjZgiA41vj10=; b=Gqh3C6+CzOnC8u7AMFXYKoRZBmpJun77S3QD1FMq/7+JV8rrRsBMN64oG6Schzz1xbI5+s 95wZUEt9gqRqD7KIskddbe8lH/ExZzLar/ORmIOz+hhV0LhFo1wjU8b0q8imLuIVkpQWap EEWR2L0eSxJLfq2DC96EQUgFOG20qgk= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-8-tXdqLbV8OBu1vFI1oZiVwA-1; Wed, 01 May 2024 10:56:52 -0400 X-MC-Unique: tXdqLbV8OBu1vFI1oZiVwA-1 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-34d99cc6c3dso984536f8f.3 for ; Wed, 01 May 2024 07:56:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714575410; x=1715180210; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=EcrWqbo8TecxfuyLftqXVzeuf9igLI8sjZgiA41vj10=; b=FpzQui+IWnF4gqaYhm94UA7rW0TYngsmCcKx7orKnjJaifaO7Pi7c+VPi5rbDd2cBv /Rryo3m3YVw8CtrmYoZeCpPcXem9WWFX4xROXYbIZ/xxu9+LXxTELqqh70CRan/J8scJ oWjoK5WAG8xBJtWF8OEOh/eGVDOBM0sKqBFhxvTD3pNUWerVJFOZKGWgDit0nF0WZBvh ykAs/QFALbVLNdchtwdSjiDOzmbVJl+Z52a9hAPiA/vwRaVi4t34cmAxjIlhpoaEpxM4 BpfdMQUq/MIjbdCymw0d4zK4YETgCV0PQiGV8BKSkUc42Ym7SXyzDe9i+Ytr+8xI1NBo +vTQ== X-Forwarded-Encrypted: i=1; AJvYcCVNBdDHjkhdG+YiCt3q0Zicxg0gyuHD6nteCC8StaNBqSjsN/ku0/OE3HdDQgOPy7vnRTwD8gOOisyvAJ/Flm7pkVapA36VhldUfw== X-Gm-Message-State: AOJu0YyKSpTgzz69Zp1xexr3/u4bWJTF0zKY3exRU/NH5d0o5INP5eu7 rLEAwzphG/0vFlHYkGB2YKLRezQze5ZbHaZG3oFbgIyinu0qRpNOblLwUTGMzsmdFuepLbKEjsM OA1L9jmSiBeVo3/+bcUEjt8HJiJV+LLfYMGs4Aaihi1LjWkCavFys0QT8l2OSrhnWxE8= X-Received: by 2002:adf:ce09:0:b0:34b:b0ac:c63c with SMTP id p9-20020adfce09000000b0034bb0acc63cmr2153435wrn.66.1714575410484; Wed, 01 May 2024 07:56:50 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHV0xsKjCZo6NnDecOo5/xZ2JkEFmVrjiMdb/YLAssgQyJruDBU3gM8759Q79+qwaJNUQ2xnA== X-Received: by 2002:adf:ce09:0:b0:34b:b0ac:c63c with SMTP id p9-20020adfce09000000b0034bb0acc63cmr2153414wrn.66.1714575409762; Wed, 01 May 2024 07:56:49 -0700 (PDT) Received: from localhost (185.223.159.143.dyn.plus.net. [143.159.223.185]) by smtp.gmail.com with ESMTPSA id n13-20020adfe78d000000b0034dd063e8dasm1698883wrm.86.2024.05.01.07.56.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 May 2024 07:56:49 -0700 (PDT) From: Andrew Burgess To: Tom Tromey Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [PATCH 1/5] Remove call to dwarf2_per_objfile::adjust from ranges readers In-Reply-To: <87y18wdjfh.fsf@tromey.com> References: <20240416-dwarf-race-relocate-2-v1-0-1fc912e95e87@tromey.com> <20240416-dwarf-race-relocate-2-v1-1-1fc912e95e87@tromey.com> <87cyq8pbnp.fsf@redhat.com> <87y18wdjfh.fsf@tromey.com> Date: Wed, 01 May 2024 15:56:48 +0100 Message-ID: <87r0eloa1b.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-6.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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: Tom Tromey writes: >>>>>> "Andrew" == Andrew Burgess writes: > > Andrew> So, I don't know the details of the DWARF reader too well, so my > Andrew> attempt to review this patch might be just wasting your time. > > Not at all. Thank you for looking at it. > >>> dwarf2_per_objfile::adjust applies gdbarch_adjust_dwarf2_addr to an >>> address, leaving the result unrelocated. However, this adjustment is >>> only needed for text-section symbols -- it isn't needed for any sort > > Andrew> I didn't know if the use of the word 'symbols' here was significant. > Andrew> gdbarch_adjust_dwarf2_addr operates on addresses, and I guess symbol > Andrew> could be a synonym for address in some contexts. But the addresses here > Andrew> do seem to be .text addresses, so clearly there's some important > Andrew> distinction that I'm not understanding. > > The basic idea of this series is to note that gdbarch_adjust_dwarf2_addr > is only really needed when computing the address of a full symbol. In > other cases, it is just extra work. > > Part of this observation is from looking at the sole implementation of > this hook. This is an abstraction violation, of course, and so maybe > the hook ought to be renamed. Certainly other arches should not use it > -- in fact, even MIPS shouldn't use it, it's a giant hack, and probably > incorrectly implemented to boot (for example, is it intentional that > minsym lookups here examine all objfiles? Because they do). > > Anyway, the MIPS hook implementation looks to see if a given DWARF > symbol is really a MIPS16 (or MicroMIPS) symbol. > > However, this information isn't relevant to the first round of DWARF > scanning. For one thing, in the scanner, symbols don't have addresses. > So, fixing up the address doesn't matter. Now, text addresses are > needed a little -- to map an address to a CU. However, the "un-fixed" > address is perfectly suitable for this as well (and these mappings are > done by ranges anyway). > > Andrew> A follow on question. Looking through gdb/dwarf/ there seem to > Andrew> be several other places where the addrmap_mutable::set_empty is > Andrew> called, and in at least some of those places the addresses are > Andrew> still being adjusted. E.g.: > > Andrew> dwarf2_ranges_read > Andrew> cooked_indexer::check_bounds > Andrew> cooked_indexer::scan_attributes > > Andrew> Why do these not need changing? > > These are all changed in patch #2. > > Andrew> And an additional question. Are lookups in these maps not done > Andrew> via the two ::find_per_cu functions? Which are passed, and are > Andrew> documented to expected an un-adjusted (i.e. have > Andrew> text_section_offset removed) address. > > Andrew> If we don't add text_section_offset in to begin with doesn't > Andrew> that cause problems? Or maybe the bit I'm missing is that the > Andrew> two paths you've changed already are adjusted? > > Yes, the lookups might be done via find_per_cu. And yes, this takes an > unrelocated address. > > The current code labors under the misapprehension that this gdbarch > transform is meaningful to the index. IIRC the previous psymtab code > had this same incorrect idea, and so I preserved it in the cooked > indexer without looking too deeply into it. > > Whether the addresses are relocated or not is a bit of a red herring. > The 'adjust' method took this into account, by applying the runtime > offset, calling the gdbarch method, and then un-applying the offset. > > However, I believe there's just no need to call this arch hook at all in > the indexes -- only for full symbols, where it affects the MIPS ABI. > > > I hope this helps. And if not, please let me know and I can try some > more. I feel like I'm not explaining it very well. What you've written sounds sensible, but mapping it back onto the code is tough. That said, my opinion, for what it's worth, is that we should move ahead and merge this series, and figure out any actual impacts for MIPS if/when they surface. It sounds like you don't think the original approach was entirely correct anyway, so if any issues do crop up, and someone wants to try and address them, then it sounds like you probably nudge them towards an alternative approach anyway. So on the understanding that I can't really comment on what the impact might be for MIPS, but that it's more important to get this threading issue fixed: Approved-By: Andrew Burgess Thanks, Andrew