testsuite/gdb.dwarf2: Fix for dw2-dos-drive failure on ARM

Message ID 529CF8B3.60906@linaro.org
State Accepted
Headers show

Commit Message

Omair Javaid Dec. 2, 2013, 9:16 p.m.
On 10/01/2013 08:34 PM, Pedro Alves wrote:
> On 10/01/2013 09:32 AM, Omair Javaid wrote:
>> On 19 September 2013 20:53, Pedro Alves <palves@redhat.com> wrote:
>>> Please don't top post.
>>>
>>> On 09/19/2013 04:23 PM, Omair Javaid wrote:
>>>> Thanks everyone for the feedback.
>>>>
>>>> I am getting following problem with 1byte text section in the dw2-dos-drive.exp
>>>>
>>>> (gdb) PASS: gdb.dwarf2/dw2-dos-drive.exp: set breakpoint pending off
>>>> break 'z:file.c':func
>>>> Cannot access memory at address 0x0
>>>>
>>>> When I change this to 4bytes the problem gets fixed. That is why I
>>>> thought this could be an unaligned illegal memory access but I accept
>>>> that the above comments verify that its not a alignment issue.
>>>>
>>>> Can anyone help me figure out what could be the cause of this problem?
>>>
>>> Breakpoint instructions on ARM are 4-byte wide.  It sounds like
>>> GDB is trying to read the memory at the breakpoint's address, and
>>> that fails (that error message comes from GDB, not the program).
>>> AFAICS, the test doesn't execute the compiled object's code, so
>>> GDB will try to read memory from the binary's sections.  As the
>>> section is only 1 byte long, and probably no other section is allocated
>>> contiguously, that'll fail...  To confirm, debug GDB under GDB,
>>> and put a break on throw_it or some such.  Then work up the stack
>>> to see where that is thrown, and why.
>>>
>>> --
>>> Pedro Alves
>>>
>>
>> I have verified the error is being thrown by gdb while its unable to
>> read the 4byte breakpoint address.
>> Heres the call stack:
>> Thread [1] (Suspended: Breakpoint hit.)
>> 38 throw_error() exceptions.c:444 0x0016728c
>> 37 memory_error() corefile.c:204 0x001d1fcc
>> 36 read_memory() corefile.c:223 0x001d201a
>> 35 read_memory_unsigned_integer() corefile.c:312 0x001d2166
>> 34 arm_skip_prologue() arm-tdep.c:1452 0x00054270
> 
> Right, though this is actually parsing the prologue:
> 
> static CORE_ADDR
> arm_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
> {
> ...
>   for (skip_pc = pc; skip_pc < limit_pc; skip_pc += 4)
>     {
>       inst = read_memory_unsigned_integer (skip_pc, 4, byte_order_for_code);
> 
> Some ports detect errors and instead return the PC as far
> as it was managed to be skip.
> E.g. rs6000-tdep.c:skip_prologue (rs6000==PowerPC):
> 
>       /* Fetch the instruction and convert it to an integer.  */
>       if (target_read_memory (pc, buf, 4))
> 	break;
>       op = extract_unsigned_integer (buf, 4, byte_order);
> 
> But not all do that.  SPARC also doesn't throw.  But others do throw
> an error like ARM.  I tried SH and that throws error like ARM;  MIPS
> and xtensa, from inspection, look like they'll throw but I haven't
> tried it.  AAarch64 throws like ARM, but that's not surprising.
> Anyway, there's no standard.
> 
>> 33 gdbarch_skip_prologue() gdbarch.c:2603 0x00176e5c
>> 32 skip_prologue_sal() symtab.c:2869 0x0013dad2
>> 31 find_function_start_sal() symtab.c:2782 0x0013d9aa
>> 30 symbol_to_sal() linespec.c:3622 0x0014f722
>> 29 convert_linespec_to_sals() linespec.c:2028 0x0014d6fa
>> 28 parse_linespec() linespec.c:2319 0x0014dc04
>> 27 decode_line_full() linespec.c:2430 0x0014df44
>> 26 parse_breakpoint_sals() breakpoint.c:9323 0x00108560
> ...
> 
>> I guess only way to address it is to either use the patch I have
>> posted or may be disable the test for arm? Any suggestions?
> 
> Another other way to handle this would be to make the prologue
> scanner cope with this, and not error out, like some ports do.  But
> it's not clear at all to me that's a useful behavior.  Even if we
> pretended we found the end of the prologue in this case, the address
> we would find in this particular case would never be a valid address
> to put a breakpoint at (the function's first address).  If we tried
> setting a breakpoint there, who knows what is it would be overwritten
> by the bytes that fall off the section (we can be 99.99% sure
> the next section would be aligned, and the gap wouldn't be used
> for anything, but still...  So, I think it might be better to leave
> the scanner as is, throwing the error while it has context about
> it, and let the user (or higher-level code) decide what to do.
> 
> Another way to tackle this could be to actually disable prologue
> skipping, by setting the breakpoint at exactly the func's first
> instruction, with the '*'/address operator:
> 
> -gdb_test "break 'z:file.c':func" {Breakpoint [0-9]+ at .*}
> +gdb_test "break *'z:file.c'::func" {Breakpoint [0-9]+ at .*}
> 
> This doesn't actually work, though I think that's a bug.  I'll
> file a PR.
> 
> But, even if it did, that converts a linespec to an expression,
> which may not be a universal solution, as tests with this issue
> might need to use a "real" linespec...
> 
> So, in the end, it'd be fine with me to just go in the
> direction of your original patch then.  But I think it deserves
> a comment:
> 
>  pc_start:
>         /* Enough space to fit one instruction.  */
> -       .byte   0
> +       .4byte  0
>  pc_end:
> 
> Could you resend your patch, with that change, a fixed commit
> log description and fixed ChangeLog?
> 
> Thanks,
> 

Sorry about responding late to this. I have attached the patch along with commit message and a ChangeLog.

Commit Log Message: 

	Avoid test failure due to error thrown from skip prologue code by
	an illegal memory access in case of single byte text section

gdb/testsuite/ChangeLog:

2013-12-02  Omair Javaid  <Omair.Javaid@linaro.org>

	* gdb.dwarf2/dw2-dos-drive.S: Changed text section size to 4 bytes

---
 gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--

Comments

Omair Javaid Jan. 15, 2014, 6:38 p.m. | #1
On 3 December 2013 02:16, Omair Javaid <omair.javaid@linaro.org> wrote:
>
> On 10/01/2013 08:34 PM, Pedro Alves wrote:
>> On 10/01/2013 09:32 AM, Omair Javaid wrote:
>>> On 19 September 2013 20:53, Pedro Alves <palves@redhat.com> wrote:
>>>> Please don't top post.
>>>>
>>>> On 09/19/2013 04:23 PM, Omair Javaid wrote:
>>>>> Thanks everyone for the feedback.
>>>>>
>>>>> I am getting following problem with 1byte text section in the dw2-dos-drive.exp
>>>>>
>>>>> (gdb) PASS: gdb.dwarf2/dw2-dos-drive.exp: set breakpoint pending off
>>>>> break 'z:file.c':func
>>>>> Cannot access memory at address 0x0
>>>>>
>>>>> When I change this to 4bytes the problem gets fixed. That is why I
>>>>> thought this could be an unaligned illegal memory access but I accept
>>>>> that the above comments verify that its not a alignment issue.
>>>>>
>>>>> Can anyone help me figure out what could be the cause of this problem?
>>>>
>>>> Breakpoint instructions on ARM are 4-byte wide.  It sounds like
>>>> GDB is trying to read the memory at the breakpoint's address, and
>>>> that fails (that error message comes from GDB, not the program).
>>>> AFAICS, the test doesn't execute the compiled object's code, so
>>>> GDB will try to read memory from the binary's sections.  As the
>>>> section is only 1 byte long, and probably no other section is allocated
>>>> contiguously, that'll fail...  To confirm, debug GDB under GDB,
>>>> and put a break on throw_it or some such.  Then work up the stack
>>>> to see where that is thrown, and why.
>>>>
>>>> --
>>>> Pedro Alves
>>>>
>>>
>>> I have verified the error is being thrown by gdb while its unable to
>>> read the 4byte breakpoint address.
>>> Heres the call stack:
>>> Thread [1] (Suspended: Breakpoint hit.)
>>> 38 throw_error() exceptions.c:444 0x0016728c
>>> 37 memory_error() corefile.c:204 0x001d1fcc
>>> 36 read_memory() corefile.c:223 0x001d201a
>>> 35 read_memory_unsigned_integer() corefile.c:312 0x001d2166
>>> 34 arm_skip_prologue() arm-tdep.c:1452 0x00054270
>>
>> Right, though this is actually parsing the prologue:
>>
>> static CORE_ADDR
>> arm_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>> {
>> ...
>>   for (skip_pc = pc; skip_pc < limit_pc; skip_pc += 4)
>>     {
>>       inst = read_memory_unsigned_integer (skip_pc, 4, byte_order_for_code);
>>
>> Some ports detect errors and instead return the PC as far
>> as it was managed to be skip.
>> E.g. rs6000-tdep.c:skip_prologue (rs6000==PowerPC):
>>
>>       /* Fetch the instruction and convert it to an integer.  */
>>       if (target_read_memory (pc, buf, 4))
>>       break;
>>       op = extract_unsigned_integer (buf, 4, byte_order);
>>
>> But not all do that.  SPARC also doesn't throw.  But others do throw
>> an error like ARM.  I tried SH and that throws error like ARM;  MIPS
>> and xtensa, from inspection, look like they'll throw but I haven't
>> tried it.  AAarch64 throws like ARM, but that's not surprising.
>> Anyway, there's no standard.
>>
>>> 33 gdbarch_skip_prologue() gdbarch.c:2603 0x00176e5c
>>> 32 skip_prologue_sal() symtab.c:2869 0x0013dad2
>>> 31 find_function_start_sal() symtab.c:2782 0x0013d9aa
>>> 30 symbol_to_sal() linespec.c:3622 0x0014f722
>>> 29 convert_linespec_to_sals() linespec.c:2028 0x0014d6fa
>>> 28 parse_linespec() linespec.c:2319 0x0014dc04
>>> 27 decode_line_full() linespec.c:2430 0x0014df44
>>> 26 parse_breakpoint_sals() breakpoint.c:9323 0x00108560
>> ...
>>
>>> I guess only way to address it is to either use the patch I have
>>> posted or may be disable the test for arm? Any suggestions?
>>
>> Another other way to handle this would be to make the prologue
>> scanner cope with this, and not error out, like some ports do.  But
>> it's not clear at all to me that's a useful behavior.  Even if we
>> pretended we found the end of the prologue in this case, the address
>> we would find in this particular case would never be a valid address
>> to put a breakpoint at (the function's first address).  If we tried
>> setting a breakpoint there, who knows what is it would be overwritten
>> by the bytes that fall off the section (we can be 99.99% sure
>> the next section would be aligned, and the gap wouldn't be used
>> for anything, but still...  So, I think it might be better to leave
>> the scanner as is, throwing the error while it has context about
>> it, and let the user (or higher-level code) decide what to do.
>>
>> Another way to tackle this could be to actually disable prologue
>> skipping, by setting the breakpoint at exactly the func's first
>> instruction, with the '*'/address operator:
>>
>> -gdb_test "break 'z:file.c':func" {Breakpoint [0-9]+ at .*}
>> +gdb_test "break *'z:file.c'::func" {Breakpoint [0-9]+ at .*}
>>
>> This doesn't actually work, though I think that's a bug.  I'll
>> file a PR.
>>
>> But, even if it did, that converts a linespec to an expression,
>> which may not be a universal solution, as tests with this issue
>> might need to use a "real" linespec...
>>
>> So, in the end, it'd be fine with me to just go in the
>> direction of your original patch then.  But I think it deserves
>> a comment:
>>
>>  pc_start:
>>         /* Enough space to fit one instruction.  */
>> -       .byte   0
>> +       .4byte  0
>>  pc_end:
>>
>> Could you resend your patch, with that change, a fixed commit
>> log description and fixed ChangeLog?
>>
>> Thanks,
>>
>
> Sorry about responding late to this. I have attached the patch along with commit message and a ChangeLog.
>
> Commit Log Message:
>
>         Avoid test failure due to error thrown from skip prologue code by
>         an illegal memory access in case of single byte text section
>
> gdb/testsuite/ChangeLog:
>
> 2013-12-02  Omair Javaid  <Omair.Javaid@linaro.org>
>
>         * gdb.dwarf2/dw2-dos-drive.S: Changed text section size to 4 bytes
>
> ---
>  gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S b/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
> index 682ba4e..f226912 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
> @@ -15,7 +15,7 @@
>
>         .text
>  pc_start:
> -       .byte   0
> +       .4byte  0
>  pc_end:
>
>         .section        .debug_info
> --

Ping! OK to commit?
Omair Javaid Jan. 16, 2014, 10:35 a.m. | #2
On Thu 16 Jan 2014 03:25:34 PM PKT, Pedro Alves wrote:
> On 12/02/2013 09:16 PM, Omair Javaid wrote:
>> On 10/01/2013 08:34 PM, Pedro Alves wrote:
>>> So, in the end, it'd be fine with me to just go in the
>>> direction of your original patch then.  But I think it deserves
>>> a comment:
>>>
>>>  pc_start:
>>>         /* Enough space to fit one instruction.  */
>>> -       .byte   0
>>> +       .4byte  0
>>>  pc_end:
>>>
>>> Could you resend your patch, with that change, a fixed commit
>>> log description and fixed ChangeLog?
>
>> Sorry about responding late to this. I have attached the patch along with commit message and a ChangeLog.
>>
>> Commit Log Message: 
>>
>> 	Avoid test failure due to error thrown from skip prologue code by
>> 	an illegal memory access in case of single byte text section
>
> Period at end of sentences.  That log if very incomplete though.
> See below for what I suggest.  (Note I tweaked the subject too)
>
> The best/easiest is to write that in the git commit log in your
> local tree, and send out the commit exactly as you propose
> to push.
>
>> gdb/testsuite/ChangeLog:
>>
>> 2013-12-02  Omair Javaid  <Omair.Javaid@linaro.org>
>>
>> 	* gdb.dwarf2/dw2-dos-drive.S: Changed text section size to 4 bytes
>
> Period at end of sentence.
>
>>
>> ---
>>  gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S b/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
>> index 682ba4e..f226912 100644
>> --- a/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
>> +++ b/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
>> @@ -15,7 +15,7 @@
>>  
>>  	.text
>>  pc_start:
>> -	.byte	0
>> +	.4byte	0
>
> Still misses comment.  The version below adds one similar to
> what I suggested before.
>
> If this looks good to you, I'll push it.
>
> Thanks.
>
> -- >8 --
> From: Omair Javaid <omair.javaid@linaro.org>
> Subject: Fix testsuite/gdb.dwarf2/dw2-dos-drive.exp on ARM.
>
> This test currently fails on ARM:
>
>   (gdb) PASS: gdb.dwarf2/dw2-dos-drive.exp: set breakpoint pending off
>   break 'z:file.c':func
>   Cannot access memory at address 0x0
>
> The error is GDB trying to read the prologue at the breakpoint's
> address, and failing:
>
>   38 throw_error() exceptions.c:444 0x0016728c
>   37 memory_error() corefile.c:204 0x001d1fcc
>   36 read_memory() corefile.c:223 0x001d201a
>   35 read_memory_unsigned_integer() corefile.c:312 0x001d2166
>   34 arm_skip_prologue() arm-tdep.c:1452 0x00054270
>
>   static CORE_ADDR
>   arm_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>   {
>   ...
>     for (skip_pc = pc; skip_pc < limit_pc; skip_pc += 4)
>       {
>         inst = read_memory_unsigned_integer (skip_pc, 4, byte_order_for_code);
>
>
> The test doesn't execute the compiled object's code, so GDB will try
> to read memory from the binary's sections.  Instructions on ARM are
> 4-byte wide, and thus ARM's prologue scanner reads in 4-byte chunks.
> As the section 'func' is put at is only 1 byte long, and no other
> section is allocated contiguously:
>
>   ...
>   Sections:
>   Idx Name          Size      VMA       LMA       File off  Algn
>     0 .text         00000001  00000000  00000000  00000034  2**0
>                     CONTENTS, ALLOC, LOAD, READONLY, CODE
>   ...
>
> ... the exec target fails the read the 4 bytes.
>
> Fix this by increasing the function's size.
>
> gdb/testsuite/ChangeLog:
> 2014-01-16  Omair Javaid  <Omair.Javaid@linaro.org>
>
> 	* gdb.dwarf2/dw2-dos-drive.S: Increase text section size to 4
> 	bytes.
> ---
>  gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S b/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
> index 6e9d360..84006f7 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
> @@ -15,7 +15,8 @@
>  
>  	.text
>  pc_start:
> -	.byte	0
> +        /* Enough space to fit at least one instruction.  */
> +	.4byte	0
>  pc_end:
>  
>  	.section	.debug_info

Changes look good. Thanks.
Kindly go ahead and push.

Patch

diff --git a/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S b/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
index 682ba4e..f226912 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
+++ b/gdb/testsuite/gdb.dwarf2/dw2-dos-drive.S
@@ -15,7 +15,7 @@ 
 
 	.text
 pc_start:
-	.byte	0
+	.4byte	0
 pc_end:
 
 	.section	.debug_info