From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id UtBuJSQKRmefrgAAWB0awg (envelope-from ) for ; Tue, 26 Nov 2024 12:49:24 -0500 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=c59PvWa6; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 7D2461E097; Tue, 26 Nov 2024 12:49:24 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham autolearn_force=no version=4.0.0 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 1F9681E05C for ; Tue, 26 Nov 2024 12:49:23 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A115E3858C3A for ; Tue, 26 Nov 2024 17:49:22 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A115E3858C3A Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=c59PvWa6 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 27A003858D33 for ; Tue, 26 Nov 2024 17:48:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 27A003858D33 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 27A003858D33 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=1732643325; cv=none; b=lkId/mGmOQydFFT5lzdMODjKYh/KGs9SB4d9itpUyGrXWYSHjpgH9p44KA2FGH+/RJSrWjNDYgnOYxtohTlaw8pvKykPB4e+XB80v2end6fNh2nyOTs4xTNpuI9yLzh3mQl+037aR3bxJ8aQJGT53SYliibTFVZy88vl3/Ea3y8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1732643325; c=relaxed/simple; bh=nDmooGUxyBlwD9VkboRtisChHHTanhTPp7muItfrcnE=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=l93KvnAN2NCDiDIEuVR9/mKLbHHHngdGf8X9xPKqD8EbtcLjRDk9tUyPkJTZWDKx1osvAHFAyg4MBN+9bpYFUFpeeyxsQJOtT/nQESCeWIentpq6EheZ5hAtojrZtlMHBgQXFPVJjXbO+zj9VUU447aeS2I+5shY/m9wsKtGe9o= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 27A003858D33 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1732643324; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=aq6FDrRgFWlnXl9nAKiDcFDPXIt+GzDkcxMDPtkjtEs=; b=c59PvWa6Iyj8Ai7R92DBrP5ZOCLlmWlRH+tE3/q9SJrhHoZUjQKcZoL/4mzDcupYGRw3k8 BRleWghJnvZzWiRxZg6WVqbnNX61FuKzn2gjqL6qZORERLLWN3VChi5Kep7nfB3gJVJlW7 ArovtBizgiNK/10peGxuvtkrGdFdB0I= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-686-k4iaIDjxOiejkIvyjWLmHg-1; Tue, 26 Nov 2024 12:48:43 -0500 X-MC-Unique: k4iaIDjxOiejkIvyjWLmHg-1 X-Mimecast-MFC-AGG-ID: k4iaIDjxOiejkIvyjWLmHg Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-43498af7937so26871995e9.1 for ; Tue, 26 Nov 2024 09:48:43 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732643322; x=1733248122; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=aq6FDrRgFWlnXl9nAKiDcFDPXIt+GzDkcxMDPtkjtEs=; b=LNZck1gPcBKUc9G97xqfbrS01Fgreom97MJtPfT8ewq9zsbR5IW2LUFJgG6Vdx6XS4 PBAZJl0JPserivSlETW2+/Ido/zqCpJnRXvUhPC+/EZMXHCxM+UFIpwmSskWcqn6GQ+W +xkmxwbPVmimsNwKt2KG1cTqPUhdClfk/ivpUtZyuMOV7z/Ffc9++HyBh7aJCQhgL9O5 RBpvSqNtbxMLhZH97iYmMZ+UNxgIeOeGV1259T90nriaAGhKB+u8VNqDOUFIWOh+ELRX FRXKxJhLv6qwDI/0Z/B1NDUd2NSMR6WJ9AxVmKfCPX/eDEJ+RbzYA/uHHw1bZ1Vxw0PE zWhQ== X-Forwarded-Encrypted: i=1; AJvYcCUFBNsLnF8d4UzR76cE4uXiV6rGfcV1Av6FN4993vUzsy/FGGxiHUPMLlwPW0HO3LoQDpaPZE8f6iOeDA==@sourceware.org X-Gm-Message-State: AOJu0YxvEuWi+RLw/47JyTK705YTJGhABK+VE3bhfh94VcoWWBD+0HBG gtxzc/DgxXOBPFdBkNjU0DR6QaDY6ZJD9IfiWywwtHFZvka6tzM8q/oryyseHP13+r3rRv0Djm+ L27KOnTTZCnnJyS2AjABf8VN02YUy8ug5otQHvo4FXLrw7pxRsrqIUXA0/7U= X-Gm-Gg: ASbGncvn47xeO9/GqS03R9v5/g7AdnGCYBiSJpGWvwd4fE0fv/TV9yYlM1tDwkwEavP 3vZICygFI5UJU0MOB7VCpWLS+7dJVzGQ8LE/qheLgcURTyz5K4B2OHIQdSdS7GHi7TAsrSeVOvJ RXXmXvLACA7JY93lSxpjUN65AC9UHVoyjYHCbcEBJwxVNHWhejq37TVbE1xsGzYYU/Hlw1d2t0R bJZay3p3k1rNVdaMirQRbCS2rgy+jLIk0Fm6nInsgnq89CI8zPJLrMfDSN3Ffmqko7FZIchpB0d AQ== X-Received: by 2002:a05:600c:5102:b0:431:59ab:15cf with SMTP id 5b1f17b1804b1-434a9dc8227mr1695455e9.19.1732643322353; Tue, 26 Nov 2024 09:48:42 -0800 (PST) X-Google-Smtp-Source: AGHT+IF8RZI0haAZKJhSuriLOheuVgwxUZQgzuS1AS8pUAKGGvOS65FZQ9w9fIisO5YsGli9tHPtew== X-Received: by 2002:a05:600c:5102:b0:431:59ab:15cf with SMTP id 5b1f17b1804b1-434a9dc8227mr1695255e9.19.1732643321793; Tue, 26 Nov 2024 09:48:41 -0800 (PST) Received: from localhost (197.209.200.146.dyn.plus.net. [146.200.209.197]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3825fb2609csm14072762f8f.44.2024.11.26.09.48.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Nov 2024 09:48:41 -0800 (PST) From: Andrew Burgess To: Bernd Edlinger , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: handle DW_AT_entry_pc pointing at an empty sub-range In-Reply-To: References: <34cfe440ffd0e53843bfaf92494d29a6951fa9fd.1732114887.git.aburgess@redhat.com> <87y11bw6p3.fsf@redhat.com> <877c8rmlm2.fsf@redhat.com> Date: Tue, 26 Nov 2024 17:48:40 +0000 Message-ID: <87bjy1lwcn.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: JWGldlI69DSIVzyAwwoeTU8DOywsZJkv6pTTyBWZ_-c_1732643322 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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~public-inbox=simark.ca@sourceware.org Bernd Edlinger writes: > On 11/25/24 15:30, Andrew Burgess wrote: >> Bernd Edlinger writes: >> >>> Okay, I just wanted to point out that in my opinion the debug info which >>> points at the end of a sub-range is not incorrect, just maybe on a border >>> line, where the dwarf spec is unclear. So you should not say: >>> "after all, the DWARF spec is clear that such a range covers no code." >>> >>> But there are obviously not only cases where the entry_pc points at >>> an empty sub-range, but also in very rare cases the entry_pc points at >>> the end of a non-empty sub-range. >>> So could you please change the check in dwarf2_addr_in_block_ranges >>> from addr >= start && addr < end to addr >= start && addr <= end. >> >> Could you expand on why you believe that the DWARF spec is unclear in >> this regard. I came to my conclusion based on this text within the >> DWARF-5 specification, section 2.17.3 Non-Contiguous Address Ranges: >> >> Bounded range. This kind of entry defines an address range that is >> included in the range list. The starting address is the lowest address >> of the address range. The ending address is the address of the first >> location past the highest address of the address range >> >> This seems pretty clear (to me) that the end address is not part of the >> region covered by a range. >> > > Yes, but on the other hand, when we look at line table entries, each has a > PC and a VIEW number, and even the DW_AT_entry_pc has a DW_AT_GNU_entry_view, > just the range list does not have a view number, and that is inconsistent > with the concept of location views. > > Consider as a simple example an inline function: > > int f(int x) > { > x++; > return x; > } > > it will most likely just be compiled into one "inc eax" or similar, > and of course you may want to set a break point on the return statement, > to inspect 'x' after the increment, but that will be on 'pc == end' ! > > But if the location view number would not be missing from the rnglist > it would be obvious whether the corresponding view number is still within > subroutine and not outside. So in my opinion it is a defect in the > specification that it does not reflect this use case. You make an interesting argument that the specification is deficient. But I'm not sure how this helps with this discussion. I would like to avoid derailing this conversation with discussion of missing DWARF features. > >> Additionally, if we start to accept 'addr == end' then this is going to >> cause problems elsewhere. GDB will place a b/p at the 'end' address, >> but when GDB then performs block lookup, GDB will not return the block >> we expect, and so GDB will not report the inferior as having stopped in >> the scope that the user expects. >> > > No, because this is exactly what the core of my patch does, admittedly > I also modified the block lookup code a bit, to handle that case. > So I strongly disagree here: we have to accept 'addr == end' and other > corner cases, otherwise my patch won't work in the end, regardless of in > how many small bug-fixes it can be split up, because it depends exactly > on not ignoring any information while parsing the debug info. But accepting 'addr == end' only works if you also change the block lookup mechanism, which isn't part of this patch. This patch is based on the state of block lookup as it exists today. I've included a patch below which applies on top of this patch (i.e. the one this thread is about), it changes the check to accept 'addr == end' as you suggest. It also updates the test so that an inline function (bar) has DW_AT_entry_pc point at the 'end' address of a non-empty sub-range. Here's a GDB session with that patch applied: (gdb) b bar Breakpoint 1 at 0x401137 (gdb) r Starting program: /tmp/gdb/testsuite/outputs/gdb.dwarf2/dw2-entry-pc-in-empty-range/dw2-entry-pc-in-empty-range-4 Breakpoint 1, 0x0000000000401137 in foo () (gdb) maintenance info blocks Blocks at 0x401137: from objfile: [(objfile *) 0x3b11720] /tmp/gdb/testsuite/outputs/gdb.dwarf2/dw2-entry-pc-in-empty-range/dw2-entry-pc-in-empty-range-4 [(block *) 0x357e680] 0x401106..0x401185 entry pc: 0x401106 is global block symbol count: 1 is contiguous [(block *) 0x357e630] 0x401106..0x401185 entry pc: 0x401106 is static block is contiguous [(block *) 0x357e5e0] 0x401106..0x401185 entry pc: 0x401106 function: foo is contiguous (gdb) As you can see, GDB stops at an address which doesn't then resolve to the bar block. If/when the block lookup code is changed as you propose then this restriction (addr < end) can be relaxed. But it doesn't make sense to merge the relaxed restriction, without the block lookup changes. And no, I don't see that as a reason to merge all of the changes at once. I think splitting the original large change into many small steps is the correct approach. And sometimes that will mean that we check something in, and then revise it in a later commit. That's not a problem with this approach, it's an advantage of this approach. It makes it clearer how we got to the final destination. And each step is smaller, and easier to review. This is the preferred approach for GDB patches. I feel that, as the author of the original large change, you're looking ahead and you're frustrated that this code isn't inline with how you feel the code should finally look. But just because I hope we can check this code in first, doesn't mean that I will prevent this code being changed later on. As GDB evolves (e.g. if the block lookup code changes) then I'm happy for this code to evolve with it. To (I hope) offer you some confidence, I have, on my machine, created a branch with this patch (without the 'addr == end' change), followed by the next two patches I plan to post (once this is merged), and then, on top of that, I have rebased your original series. This includes all of your original tests completely unmodified. I have tested this merge with gcc versions 14.2.0, 13.3.0, 12.2.0, 11.5.0, 10.5.0, 9.5.0, 9.3.1, 8.4.0, 8.1.0, and in all cases, all of your original tests pass. I'd rather not post this merged branch just yet, as I'm worried that this might derail review of this patch even more, I really don't want to start discussing the next patches before they are even posted, but if it's the only way to move this patch forward then I could share the branch. I do plan to make this unified branch available when I post the next two patches I'd like to upstream, as I think it will actually help at that point. My hope is that you will be willing to accept this change on the understanding that this code might need to be modified in the future. For my part I also accept that this code might need to change in the future. Thanks, Andrew --- ### Patch to show 'addr == end' doesn't work (yet) ### diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index c178b13d96d..60daf032036 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -11351,7 +11351,7 @@ dwarf2_addr_in_block_ranges (CORE_ADDR addr, struct block *block) /* Check if ADDR is within any of the block's sub-ranges. */ for (const blockrange &br : block->ranges ()) { - if (addr >= br.start () && addr < br.end ()) + if (addr >= br.start () && addr <= br.end ()) return true; } diff --git a/gdb/testsuite/gdb.dwarf2/dw2-entry-pc-in-empty-range.exp b/gdb/testsuite/gdb.dwarf2/dw2-entry-pc-in-empty-range.exp index 79b1783b2ec..9e4fb781a8d 100644 --- a/gdb/testsuite/gdb.dwarf2/dw2-entry-pc-in-empty-range.exp +++ b/gdb/testsuite/gdb.dwarf2/dw2-entry-pc-in-empty-range.exp @@ -179,8 +179,8 @@ proc run_test { entry_label dwarf_version } { " $::foo_5\\.\\.$::foo_6"] } -foreach_with_prefix entry_label { foo_3 foo_4 } { - foreach_with_prefix dwarf_version { 4 5 } { +foreach_with_prefix entry_label { foo_2 } { + foreach_with_prefix dwarf_version { 4 } { run_test $entry_label $dwarf_version } }