Message ID | BLU437-SMTP6538A6B785A169EE9AED80F78B0@phx.gbl |
---|---|
State | New, archived |
Headers |
Received: (qmail 31399 invoked by alias); 17 Mar 2016 22:42:02 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 31382 invoked by uid 89); 17 Mar 2016 22:42:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=Device, Drivers, stepi, Class X-HELO: BLU004-OMC2S33.hotmail.com Received: from blu004-omc2s33.hotmail.com (HELO BLU004-OMC2S33.hotmail.com) (65.55.111.108) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA256 encrypted) ESMTPS; Thu, 17 Mar 2016 22:41:59 +0000 Received: from BLU437-SMTP65 ([65.55.111.73]) by BLU004-OMC2S33.hotmail.com over TLS secured channel with Microsoft SMTPSVC(7.5.7601.23008); Thu, 17 Mar 2016 15:41:57 -0700 X-TMN: [dUPjjkwJZCWZtTZ/Nf7FYgbr2NJbWv8A] Message-ID: <BLU437-SMTP6538A6B785A169EE9AED80F78B0@phx.gbl> From: Cristiano De Alti <cristiano_dealti@hotmail.com> To: gdb-patches@sourceware.org CC: Cristiano De Alti <cristiano_dealti@hotmail.com> Subject: [PATCH] Minor fix in AVR prologue scan. Date: Thu, 17 Mar 2016 23:41:50 +0100 MIME-Version: 1.0 Content-Type: text/plain |
Commit Message
Cristiano
March 17, 2016, 10:41 p.m. UTC
I use GDB from Eclipse with Avarice to debug AVR microcontrollers. While single stepping (stepi) through the instructions of a normal function prologue, stepping over the sbiw r28, 0x0f instruction, which is near the end of the prologue, causes the following change in the backtrace output: Before stepi: #0 0x000042b0 in HID_Device_USBTask (HIDInterfaceInfo=0x80b8b8) at ../../LUFA/Drivers/USB/Class/Device/HIDClassDevice.c:157 #1 0x000002e0 in main () at HIDRadio.c:83 After stepi: #0 0x000042b2 in HID_Device_USBTask (HIDInterfaceInfo=0x8000ff) at ../../LUFA/Drivers/USB/Class/Device/HIDClassDevice.c:157 #1 0x00017170 in ?? () Note that the frame #1 is inconsistent. If you now return from frame #0 with the finish command, the target does not stop at the return address at main. Instead, the target starts running and it's necessary to manually stop it. This is not a big issue and I'm not an expert of the gdb internals but the fix seems easy enough. gdb/ChangeLog: 2016-03-17 Cristiano De Alti <cristiano_dealti@hotmail.com> * avr-tdep.c (avr_scan_prologue): fix comparison to detect the prologue limits. --- gdb/avr-tdep.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
Comments
On 03/17/2016 07:41 PM, Cristiano De Alti wrote: > I use GDB from Eclipse with Avarice to debug AVR microcontrollers. > While single stepping (stepi) through the instructions of a normal > function prologue, stepping over the sbiw r28, 0x0f instruction, which > is near the end of the prologue, causes the following change in the > backtrace output: > > Before stepi: > #0 0x000042b0 in HID_Device_USBTask (HIDInterfaceInfo=0x80b8b8) at > ../../LUFA/Drivers/USB/Class/Device/HIDClassDevice.c:157 > #1 0x000002e0 in main () at HIDRadio.c:83 > > After stepi: > #0 0x000042b2 in HID_Device_USBTask (HIDInterfaceInfo=0x8000ff) at > ../../LUFA/Drivers/USB/Class/Device/HIDClassDevice.c:157 > #1 0x00017170 in ?? () > > Note that the frame #1 is inconsistent. > > If you now return from frame #0 with the finish command, the target does > not stop at the return address at main. Instead, the target starts > running and it's necessary to manually stop it. > > This is not a big issue and I'm not an expert of the gdb internals but > the fix seems easy enough. > > gdb/ChangeLog: > 2016-03-17 Cristiano De Alti <cristiano_dealti@hotmail.com> > * avr-tdep.c (avr_scan_prologue): fix comparison to detect > the prologue limits. > --- > gdb/avr-tdep.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c > index 993e92b..a8080db 100644 > --- a/gdb/avr-tdep.c > +++ b/gdb/avr-tdep.c > @@ -833,25 +833,26 @@ avr_scan_prologue (struct gdbarch *gdbarch, CORE_ADDR pc_beg, CORE_ADDR pc_end, > or signal handler functions, which is why we set the prologue type > when we saw the beginning of the prologue previously. */ > > - if (vpc + sizeof (img_sig) < len > + if (vpc + sizeof (img_sig) <= len Does changing < to <= mean the prologue size is being calculated incorrectly, off-by-one? If you end up with a case of having to scan AVR_MAX_PROLOGUE_SIZE, you will eventually check 17 4-byte instructions rather than 16 4-byte ones. Not sure if that is correct by looking at the code. Usually backtrace hiccups like these tend to come from small variations of prologues generated by the compiler, and GDB needs to be told what to expect from such a prologue. If this is a new variation of prologue, adding handling for a new pattern would be the appropriate solution. > && memcmp (prologue + vpc, img_sig, sizeof (img_sig)) == 0) > { > + info->size += locals_size; > vpc += sizeof (img_sig); > } > - else if (vpc + sizeof (img_int) < len > + else if (vpc + sizeof (img_int) <= len > && memcmp (prologue + vpc, img_int, sizeof (img_int)) == 0) > { > + info->size += locals_size; > vpc += sizeof (img_int); > } > - if (vpc + sizeof (img) < len > + if (vpc + sizeof (img) <= len > && memcmp (prologue + vpc, img, sizeof (img)) == 0) > { > + info->size += locals_size; > info->prologue_type = AVR_PROLOGUE_NORMAL; > vpc += sizeof (img); > } > > - info->size += locals_size; > - What is the reason for moving the above and replicating it across multiple if blocks? It may cause GDB to not set info->size under certain conditions. Is this what is expected?
Thanks for reviewing this. Please see below. On 03/18/2016 02:13 PM, Luis Machado wrote: > On 03/17/2016 07:41 PM, Cristiano De Alti wrote: >> I use GDB from Eclipse with Avarice to debug AVR microcontrollers. >> While single stepping (stepi) through the instructions of a normal >> function prologue, stepping over the sbiw r28, 0x0f instruction, which >> is near the end of the prologue, causes the following change in the >> backtrace output: >> >> Before stepi: >> #0 0x000042b0 in HID_Device_USBTask (HIDInterfaceInfo=0x80b8b8) at >> ../../LUFA/Drivers/USB/Class/Device/HIDClassDevice.c:157 >> #1 0x000002e0 in main () at HIDRadio.c:83 >> >> After stepi: >> #0 0x000042b2 in HID_Device_USBTask (HIDInterfaceInfo=0x8000ff) at >> ../../LUFA/Drivers/USB/Class/Device/HIDClassDevice.c:157 >> #1 0x00017170 in ?? () >> >> Note that the frame #1 is inconsistent. >> >> If you now return from frame #0 with the finish command, the target does >> not stop at the return address at main. Instead, the target starts >> running and it's necessary to manually stop it. >> >> This is not a big issue and I'm not an expert of the gdb internals but >> the fix seems easy enough. >> >> gdb/ChangeLog: >> 2016-03-17 Cristiano De Alti <cristiano_dealti@hotmail.com> >> * avr-tdep.c (avr_scan_prologue): fix comparison to detect >> the prologue limits. >> --- >> gdb/avr-tdep.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c >> index 993e92b..a8080db 100644 >> --- a/gdb/avr-tdep.c >> +++ b/gdb/avr-tdep.c >> @@ -833,25 +833,26 @@ avr_scan_prologue (struct gdbarch *gdbarch, >> CORE_ADDR pc_beg, CORE_ADDR pc_end, >> or signal handler functions, which is why we set the >> prologue type >> when we saw the beginning of the prologue previously. */ >> >> - if (vpc + sizeof (img_sig) < len >> + if (vpc + sizeof (img_sig) <= len > > Does changing < to <= mean the prologue size is being calculated > incorrectly, off-by-one? > Correct. You can compare the contents of prologue and img if the size of img is <= (len - vpc). Actually, I've tested this only for AVR_PROLOGUE_NORMAL and corrected the comparison for the other prologue types. > If you end up with a case of having to scan AVR_MAX_PROLOGUE_SIZE, you > will eventually check 17 4-byte instructions rather than 16 4-byte ones. > Not sure if that is correct by looking at the code. > Me neither. I don't have such long prologues. > Usually backtrace hiccups like these tend to come from small variations > of prologues generated by the compiler, and GDB needs to be told what to > expect from such a prologue. > > If this is a new variation of prologue, adding handling for a new > pattern would be the appropriate solution. No variation. AFAIK it's the usual prologue. Note that the original code worked most of the times. I've clearly indicated in the commit one case where it failed (stepi over sbiw r28, 0x0f instruction). > >> && memcmp (prologue + vpc, img_sig, sizeof (img_sig)) == 0) >> { >> + info->size += locals_size; >> vpc += sizeof (img_sig); >> } >> - else if (vpc + sizeof (img_int) < len >> + else if (vpc + sizeof (img_int) <= len >> && memcmp (prologue + vpc, img_int, sizeof (img_int)) == 0) >> { >> + info->size += locals_size; >> vpc += sizeof (img_int); >> } >> - if (vpc + sizeof (img) < len >> + if (vpc + sizeof (img) <= len >> && memcmp (prologue + vpc, img, sizeof (img)) == 0) >> { >> + info->size += locals_size; >> info->prologue_type = AVR_PROLOGUE_NORMAL; >> vpc += sizeof (img); >> } >> >> - info->size += locals_size; >> - > > What is the reason for moving the above and replicating it across > multiple if blocks? It may cause GDB to not set info->size under certain > conditions. Is this what is expected? > Updating info->size only when img has been matched prevents the above frame corruption. I have no evidence that info->size needs to be set under other conditions. The original code always incrementing info->size is wrong however. Again, only tested for AVR_PROLOGUE_NORMAL and assumed it works the same for the other prologue types. >
diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c index 993e92b..a8080db 100644 --- a/gdb/avr-tdep.c +++ b/gdb/avr-tdep.c @@ -833,25 +833,26 @@ avr_scan_prologue (struct gdbarch *gdbarch, CORE_ADDR pc_beg, CORE_ADDR pc_end, or signal handler functions, which is why we set the prologue type when we saw the beginning of the prologue previously. */ - if (vpc + sizeof (img_sig) < len + if (vpc + sizeof (img_sig) <= len && memcmp (prologue + vpc, img_sig, sizeof (img_sig)) == 0) { + info->size += locals_size; vpc += sizeof (img_sig); } - else if (vpc + sizeof (img_int) < len + else if (vpc + sizeof (img_int) <= len && memcmp (prologue + vpc, img_int, sizeof (img_int)) == 0) { + info->size += locals_size; vpc += sizeof (img_int); } - if (vpc + sizeof (img) < len + if (vpc + sizeof (img) <= len && memcmp (prologue + vpc, img, sizeof (img)) == 0) { + info->size += locals_size; info->prologue_type = AVR_PROLOGUE_NORMAL; vpc += sizeof (img); } - info->size += locals_size; - /* Fall through. */ }