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.129.124]) by sourceware.org (Postfix) with ESMTPS id BC2013858C20 for ; Thu, 10 Mar 2022 13:20:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BC2013858C20 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-401-fiTsWcDKOBGPQUyURS8NLg-1; Thu, 10 Mar 2022 08:19:59 -0500 X-MC-Unique: fiTsWcDKOBGPQUyURS8NLg-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A3D728031E1; Thu, 10 Mar 2022 13:19:57 +0000 (UTC) Received: from [10.97.116.153] (ovpn-116-153.gru2.redhat.com [10.97.116.153]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7B9B1105C89F; Thu, 10 Mar 2022 13:19:54 +0000 (UTC) Message-ID: <49264d6f-d66c-774e-f57b-9eb2efd767d5@redhat.com> Date: Thu, 10 Mar 2022 10:19:49 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH] Updated, fix reverse stepping multiple contiguous PC ranges To: Luis Machado , Carl Love , gdb-patches@sourceware.org Cc: Rogerio Alves , nd@arm.com References: <442684e3f81aa1df073960bd45918106acefa2b9.camel@us.ibm.com> <81c5bece-012b-cad6-8349-71bb255ff242@arm.com> From: Bruno Larsen In-Reply-To: <81c5bece-012b-cad6-8349-71bb255ff242@arm.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Thu, 10 Mar 2022 13:20:03 -0000 On 3/10/22 08:13, Luis Machado wrote: > Hi! > > On 3/8/22 20:21, Bruno Larsen via Gdb-patches wrote: >> On 2/23/22 13:39, Carl Love via Gdb-patches wrote: >>> >>> GCC maintainers: >>> >>> The following patch was posted by Luis Machado on 2/1/2021.  There was >>> a little discussion on the patch but it was never fully reviewed and >>> approved.  The email for Luis no longer >>> works. >>> >>> As of 2/21/2022, the patch does not compile.  I made a small fix to get >>> it to compile.  I commented out the original line in gdb/infrun.c and >>> added a new line with the fix and the comment //carll fix to indicate >>> what I changed.  Clearly, the comment needs to be removed if the patch >>> is accepted but I wanted to show what I changed. >>> >>> That said, I tested the patch on Powerpc and it fixed the 5 test >>> failures in gdb.reverse/solib-precsave.exp and 5 test failures in >>> gdb.reverse/solib-reverse.exp.  I tested on Intel 64-bit.  The two >>> tests solib-precsave.exp and solib-reverse.exp both initially passed on >>> Intel.  No additional regression failures were seen with the patch. >>> >>> Please let me know if you have comments on the patch or if it is >>> acceptable as is.  Thank you. >>> >>>                       Carl Love >> >> Hello Carl! >> >> Thanks for looking at this. Since I don't test on aarch64 often, I am not sure if I see regressions or racy testcases, but it does fix the issue you mentioned, and there doesn't seem to be regressions on x86_64 hardware. I have a few nits, but the main feedback is: could you add a testcase for this, using the dwarf assembler and manually creating contiguous PC ranges, so we can confirm that this is not regressed in the future on any hardware? >> >> Also, I can't approve a patch, but with the testcase this patch is mostly ok by me >> >>> ----------------------------------------------------------- >>> From: Luis Machado >>> Date: Mon, 21 Feb 2022 23:11:23 +0000 >>> Subject: [PATCH] Fix reverse stepping multiple contiguous PC ranges >>> >>> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04, I noticed some >>> failures in gdb.reverse/solib-precsave.exp and gdb.reverse/solib-reverse.exp. >>> >>> The failure happens around the following code: >>> >>> 38  b[1] = shr2(17);            /* middle part two */ >>> 40  b[0] = 6;   b[1] = 9;       /* generic statement, end part two */ >>> 42  shr1 ("message 1\n");       /* shr1 one */ >>> >>> Normal execution: >>> >>> - step from line 1 will land on line 2. >>> - step from line 2 will land on line 3. >>> >>> Reverse execution: >>> >>> - step from line 3 will land on line 2. >>> - step from line 2 will land on line 2. >>> - step from line 2 will land on line 1. >>> >>> The problem here is that line 40 contains two contiguous but distinct >>> PC ranges, like so: >>> >>> Line 40 - [0x7ec ~ 0x7f4] >>> Line 40 - [0x7f4 ~ 0x7fc] >>> >>> When stepping forward from line 2, we skip both of these ranges and land on >>> line 42. When stepping backward from line 3, we stop at the start PC of the >>> second (or first, going backwards) range of line 40. >>> >>> This happens because we have this check in infrun.c:process_event_stop_test: >>> >>>        /* When stepping backward, stop at beginning of line range >>>           (unless it's the function entry point, in which case >>>           keep going back to the call point).  */ >>>        CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc; >>>        if (stop_pc == ecs->event_thread->control.step_range_start >>>            && stop_pc != ecs->stop_func_start >>>            && execution_direction == EXEC_REVERSE) >>>          end_stepping_range (ecs); >>>        else >>>          keep_going (ecs); >>> >>> Since we've reached ecs->event_thread->control.step_range_start, we stop >>> stepping backwards. >>> >>> The right thing to do is to look for adjacent PC ranges for the same line, >>> until we notice a line change. Then we take that as the start PC of the >>> range. >>> >>> Another solution I thought about is to merge the contiguous ranges when >>> we are reading the line tables. Though I'm not sure if we really want to process >>> that data as opposed to keeping it as the compiler created, and then working >>> around that. >>> >>> In any case, the following patch addresses this problem. >>> >>> I'm not particularly happy with how we go back in the ranges (using "pc - 1"). >>> Feedback would be welcome. >>> >>> Validated on aarch64-linux/Ubuntu 20.04/18.04. >>> >>> Ubuntu 18.04 doesn't actually run into these failures because the compiler >>> doesn't generate distinct PC ranges for the same line. >>> >>> gdb/ChangeLog: >>> >>> YYYY-MM-DD  Luis Machado >>> >>>          * infrun.c (process_event_stop_test): Handle backward stepping >>>          across multiple ranges for the same line. >>>          * symtab.c (find_line_range_start): New function. >>>          * symtab.h (find_line_range_start): New prototype. >>> >>> >>> Co-authored-by: Carl Love >>> --- >>>   gdb/infrun.c | 24 +++++++++++++++++++++++- >>>   gdb/symtab.c | 35 +++++++++++++++++++++++++++++++++++ >>>   gdb/symtab.h | 16 ++++++++++++++++ >>>   3 files changed, 74 insertions(+), 1 deletion(-) >>> >>> diff --git a/gdb/infrun.c b/gdb/infrun.c >>> index 376a541faf6..997042d3e45 100644 >>> --- a/gdb/infrun.c >>> +++ b/gdb/infrun.c >>> @@ -6782,11 +6782,33 @@ if (ecs->event_thread->control.proceed_to_finish >>>        have software watchpoints).  */ >>>         ecs->event_thread->control.may_range_step = 1; >>> +      /* When we are stepping inside a particular line range, in reverse, >>> +     and we are sitting at the first address of that range, we need to >>> +     check if this address also shows up in another line range as the >>> +     end address. >>> + >>> +     If so, we need to check what line such a step range points to. >>> +     If it points to the same line as the current step range, that >>> +     means we need to keep going in order to reach the first address >>> +     of the line range.  We repeat this until we eventually get to the >>> +     first address of a particular line we're stepping through.  */ >>> +      CORE_ADDR range_start = ecs->event_thread->control.step_range_start; >>> +      if (execution_direction == EXEC_REVERSE) >>> +    { >>> +      gdb::optional real_range_start >>> +        //     = find_line_range_start (ecs->event_thread->suspend.stop_pc); >>> +        = find_line_range_start (ecs->event_thread->stop_pc()); //carll fi> + >>> + >>> +      if (real_range_start.has_value ()) >>> +        range_start = *real_range_start; >>> +    } >>> + >>>         /* When stepping backward, stop at beginning of line range >>>        (unless it's the function entry point, in which case >>>        keep going back to the call point).  */ >>>         CORE_ADDR stop_pc = ecs->event_thread->stop_pc (); >>> -      if (stop_pc == ecs->event_thread->control.step_range_start >>> +      if (stop_pc == range_start >>>         && stop_pc != ecs->stop_func_start >> >> I think this could be moved to the line above. >> > > Do you mean moving the range_start check downwards below the ecs->stop_func_start check? I meant that they can be in the same line: if (stop_pc == range_start && stop_pc != ecs->stop_func_start > >>>         && execution_direction == EXEC_REVERSE) >>>       end_stepping_range (ecs); >>> diff --git a/gdb/symtab.c b/gdb/symtab.c >>> index 1a39372aad0..c40739919d1 100644 >>> --- a/gdb/symtab.c >>> +++ b/gdb/symtab.c >>> @@ -3425,6 +3425,41 @@ find_pc_line (CORE_ADDR pc, int notcurrent) >>>     return sal; >>>   } >>> +/* See symtah.h.  */ >>> + >>> +gdb::optional >>> +find_line_range_start (CORE_ADDR pc) >>> +{ >>> +  struct symtab_and_line current_sal = find_pc_line (pc, 0); >>> + >>> +  if (current_sal.line == 0) >>> +    return {}; >>> + >>> +  struct symtab_and_line prev_sal = find_pc_line (current_sal.pc - 1, 0); >>> + >>> +  /* If the previous entry is for a different line, that means we are already >>> +     at the entry with the start PC for this line.  */ >>> +  if (prev_sal.line != current_sal.line) >>> +    return current_sal.pc; >>> + >>> +  /* Otherwise, keep looking for entries for the same line but with >>> +     smaller PC's.  */ >>> +  bool done = false; >>> +  CORE_ADDR prev_pc; >>> +  while (!done) >>> +    { >>> +      prev_pc = prev_sal.pc; >>> + >>> +      prev_sal = find_pc_line (prev_pc - 1, 0); >>> + >>> +      /* Did we notice a line change?  If so, we are done with the search.  */ >>> +      if (prev_sal.line != current_sal.line) >>> +    done = true; >> >> Shouldn't prev_sal.line also be checked here and return an empty optional? I am not sure when that happens, so please enlighten me if there is no need to check. >> > > I went through this again, and I don't think prev-sal.line needs to be checked. At this point we know current_sal.line is sane, so anything that differs from the current line, we bail out and return the address we currently have. > > Does that make sense? > It does. My main point was asking if finding a 0 was cause for leaving with an error, instead of leaving with the answer that could be wrong. But the base answer is already wrong, so this would probably be a less wrong answer, so this is fine -- Cheers! Bruno Larsen