[RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Jiong Wang]

[-- Attachment #1: Type: text/plain, Size: 290 bytes --]

for tilegx,  when skip prologue, if the start_pc is a plt stub address, then
stop to go further, just return the start_pc.


gdb/ChangeLog:

        * tilegx-tdep.c (tilegx_skip_prologue): simplify the handling for
        plt stub.


please review.

---
Regards,
Jiong
Tilera Corporation.

[-- Attachment #2: 0003-fix-plt-stub-issue.patch --]
[-- Type: text/x-patch, Size: 575 bytes --]

---
 gdb/tilegx-tdep.c |    4 ++++
 1 files changed, 4 insertions(+)

diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index 2f1d824..6432edb 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -746,6 +746,10 @@ tilegx_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
 	  return sal.end;
     }
 
+  /* for plt stub, just return the start pc */
+  if (in_plt_section (pc, NULL))
+    return pc;
+
   /* Otherwise, try to skip prologue the hard way.  */
   return tilegx_analyze_prologue (gdbarch,
 				  pc, pc + 8 * TILEGX_BUNDLE_SIZE_IN_BYTES,
-- 
1.7.10.3

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Joel Brobecker]

> for tilegx,  when skip prologue, if the start_pc is a plt stub address, then
> stop to go further, just return the start_pc.
> 
> gdb/ChangeLog:
> 
>        * tilegx-tdep.c (tilegx_skip_prologue): simplify the handling for
>        plt stub.

Can you provide an example where this becomes necessary? I don't
see a problem with the patch per se, but I don't remember seeing
other ports doing this...

-- 
Joel

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Jiong Wang]

>> for tilegx,  when skip prologue, if the start_pc is a plt stub address, then
>> stop to go further, just return the start_pc.
>>
>> gdb/ChangeLog:
>>
>>         * tilegx-tdep.c (tilegx_skip_prologue): simplify the handling for
>>         plt stub.
> Can you provide an example where this becomes necessary? I don't
> see a problem with the patch per se, but I don't remember seeing
> other ports doing this...
>
yes, there are some tricky thing.

   for the simple hello.c

#include <stdio.h>
int main(int argc, char **argv)
{
printf("hello, %d\n", argc);
return 0
}

without this patch
===
-bash-4.1$ gcc -o hello.tile hello.c
-bash-4.1$ ./gdb hello.tile
(gdb) b printf
Error accessing memory address 0x10a40: Success.

after this patch
===
(gdb) b printf
Breakpoint 1 at 0x10a40


this is because tilegx skip_prologue will invoke 
tilegx_analyze_prologue, which
will prefetch 32*8 bytes.

while for when the address is in plt stub,  you can see it near the 
eh_frame_hdr section

   [14] .plt                         0000000000010a00 000a00 0000a0 28  
AX  0   0 64
   ...
   [16] .eh_frame_hdr     0000000000010ac0 000ac0 000024 00   A  0 0  4
   [17] .eh_frame             0000000000010ae8 000ae8 0000b4 00   A 0   0  8

the .eh_frame_hdr aligns to 4, there is a hole between .eh_frame_hdr and 
.eh_frame, and this
will cause trouble for section_table_xfer_memory_partial.

after fetch memory starting from 0x10ac0 to 0x10ae4, then the memaddr 
will be 0x10ae4 in section_table_xfer_memory_partial,
while this function did not consider this hole situation, so goes to 
line 666, error occured.

for other targets, like x86, I have done a brief exploration,  seems 
they do not have large prefetch window

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Pedro Alves]

On 01/18/2013 03:12 PM, Jiong Wang wrote:
> 
>>> for tilegx,  when skip prologue, if the start_pc is a plt stub address, then
>>> stop to go further, just return the start_pc.
>>>
>>> gdb/ChangeLog:
>>>
>>>         * tilegx-tdep.c (tilegx_skip_prologue): simplify the handling for
>>>         plt stub.
>> Can you provide an example where this becomes necessary? I don't
>> see a problem with the patch per se, but I don't remember seeing
>> other ports doing this...
>>
> yes, there are some tricky thing.
> 
>   for the simple hello.c
> 
> #include <stdio.h>
> int main(int argc, char **argv)
> {
> printf("hello, %d\n", argc);
> return 0
> }
> 
> without this patch
> ===
> -bash-4.1$ gcc -o hello.tile hello.c
> -bash-4.1$ ./gdb hello.tile
> (gdb) b printf
> Error accessing memory address 0x10a40: Success.
> 
> after this patch
> ===
> (gdb) b printf
> Breakpoint 1 at 0x10a40
> 
> 
> this is because tilegx skip_prologue will invoke tilegx_analyze_prologue, which
> will prefetch 32*8 bytes.
> 
> while for when the address is in plt stub,  you can see it near the eh_frame_hdr section
> 
>   [14] .plt                         0000000000010a00 000a00 0000a0 28  AX  0   0 64
>   ...
>   [16] .eh_frame_hdr     0000000000010ac0 000ac0 000024 00   A  0 0  4
>   [17] .eh_frame             0000000000010ae8 000ae8 0000b4 00   A 0   0  8
> 
> the .eh_frame_hdr aligns to 4, there is a hole between .eh_frame_hdr and .eh_frame, and this
> will cause trouble for section_table_xfer_memory_partial.
> 
> after fetch memory starting from 0x10ac0 to 0x10ae4, then the memaddr will be 0x10ae4 in section_table_xfer_memory_partial,
> while this function did not consider this hole situation, so goes to line 666, error occured.

Right.  It's this issue that needs solving.  Otherwise you're just
papering over the problem.

> 
> for other targets, like x86, I have done a brief exploration,  seems they do not have large prefetch window



-- 
Pedro Alves

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Jiong Wang]

On 01/19/2013 01:29 AM, Pedro Alves wrote:
> On 01/18/2013 03:12 PM, Jiong Wang wrote:
>>>> for tilegx,  when skip prologue, if the start_pc is a plt stub address, then
>>>> stop to go further, just return the start_pc.
>>>>
>>>> gdb/ChangeLog:
>>>>
>>>>          * tilegx-tdep.c (tilegx_skip_prologue): simplify the handling for
>>>>          plt stub.
>>> Can you provide an example where this becomes necessary? I don't
>>> see a problem with the patch per se, but I don't remember seeing
>>> other ports doing this...
>>>
>> yes, there are some tricky thing.
>>
>>    for the simple hello.c
>>
>> #include <stdio.h>
>> int main(int argc, char **argv)
>> {
>> printf("hello, %d\n", argc);
>> return 0
>> }
>>
>> without this patch
>> ===
>> -bash-4.1$ gcc -o hello.tile hello.c
>> -bash-4.1$ ./gdb hello.tile
>> (gdb) b printf
>> Error accessing memory address 0x10a40: Success.
>>
>> after this patch
>> ===
>> (gdb) b printf
>> Breakpoint 1 at 0x10a40
>>
>>
>> this is because tilegx skip_prologue will invoke tilegx_analyze_prologue, which
>> will prefetch 32*8 bytes.
>>
>> while for when the address is in plt stub,  you can see it near the eh_frame_hdr section
>>
>>    [14] .plt                         0000000000010a00 000a00 0000a0 28  AX  0   0 64
>>    ...
>>    [16] .eh_frame_hdr     0000000000010ac0 000ac0 000024 00   A  0 0  4
>>    [17] .eh_frame             0000000000010ae8 000ae8 0000b4 00   A 0   0  8
>>
>> the .eh_frame_hdr aligns to 4, there is a hole between .eh_frame_hdr and .eh_frame, and this
>> will cause trouble for section_table_xfer_memory_partial.
>>
>> after fetch memory starting from 0x10ac0 to 0x10ae4, then the memaddr will be 0x10ae4 in section_table_xfer_memory_partial,
>> while this function did not consider this hole situation, so goes to line 666, error occured.
> Right.  It's this issue that needs solving.  Otherwise you're just
> papering over the problem.
Ping,

is this patch OK to commit?

---
Regards,
Jiong
>
>> for other targets, like x86, I have done a brief exploration,  seems they do not have large prefetch window
>
>


-- 
Regards,
Jiong. Wang
Tilera Corporation.

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Yao Qi]

On 01/18/2013 11:12 PM, Jiong Wang wrote:
> this is because tilegx skip_prologue will invoke
> tilegx_analyze_prologue, which
> will prefetch 32*8 bytes.
>
> while for when the address is in plt stub,  you can see it near the
> eh_frame_hdr section
>
>     [14] .plt                         0000000000010a00 000a00 0000a0 28
> AX  0   0 64
>     ...
>     [16] .eh_frame_hdr     0000000000010ac0 000ac0 000024 00   A  0 0  4
>     [17] .eh_frame             0000000000010ae8 000ae8 0000b4 00   A 0   0  8
>
> the .eh_frame_hdr aligns to 4, there is a hole between .eh_frame_hdr and
> .eh_frame, and this
> will cause trouble for section_table_xfer_memory_partial.
>
> after fetch memory starting from 0x10ac0 to 0x10ae4, then the memaddr
> will be 0x10ae4 in section_table_xfer_memory_partial,
> while this function did not consider this hole situation, so goes to
> line 666, error occured.

Wang Jiong,

AFAICT, the root cause of this problem is GDB prefetches too much 
contents in one time that exceeds the boundary of a section.

At the beginning of tilegx_analyze_prologue, I notice this comment

   /* To cut down on round-trip overhead, we fetch multiple bundles
      at once.  These variables describe the range of memory we have
      prefetched.  */

Can't we fetch one bundle in one time?  Fetching multiple bundles causes 
this problem, so we have to disable it.

Even we still decide to use fetching multiple bundle in one time, we 
should take care of the boundary and existing code does this, see this 
comment,

	  /* Figure out how many bytes to fetch.  Don't span a page
	     boundary since that might cause an unnecessary memory
	     error.  */

Looks existing code takes care of not crossing the page boundary, 
similarly, we should also take care of not crossing the section 
boundary.  What do you think?

-- 
Yao (齐尧)

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Jiong Wang]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]


> On 01/18/2013 11:12 PM, Jiong Wang wrote:
>> this is because tilegx skip_prologue will invoke
>> tilegx_analyze_prologue, which
>> will prefetch 32*8 bytes.
>>
>> while for when the address is in plt stub,  you can see it near the
>> eh_frame_hdr section
>>
>>     [14] .plt                         0000000000010a00 000a00 0000a0 28
>> AX  0   0 64
>>     ...
>>     [16] .eh_frame_hdr     0000000000010ac0 000ac0 000024 00 A  0 0  4
>>     [17] .eh_frame             0000000000010ae8 000ae8 0000b4 00   A 
>> 0   0  8
>>
>> the .eh_frame_hdr aligns to 4, there is a hole between .eh_frame_hdr and
>> .eh_frame, and this
>> will cause trouble for section_table_xfer_memory_partial.
>>
>> after fetch memory starting from 0x10ac0 to 0x10ae4, then the memaddr
>> will be 0x10ae4 in section_table_xfer_memory_partial,
>> while this function did not consider this hole situation, so goes to
>> line 666, error occured.
>
> Wang Jiong,
>
> AFAICT, the root cause of this problem is GDB prefetches too much 
> contents in one time that exceeds the boundary of a section.
>
> At the beginning of tilegx_analyze_prologue, I notice this comment
>
>   /* To cut down on round-trip overhead, we fetch multiple bundles
>      at once.  These variables describe the range of memory we have
>      prefetched.  */
>
> Can't we fetch one bundle in one time?  Fetching multiple bundles 
> causes this problem, so we have to disable it.

I think we should keep prefetching multiple instruction bundles to cut 
down on round-trip overhead, just as the comment explained.

>
> Even we still decide to use fetching multiple bundle in one time, we 
> should take care of the boundary and existing code does this, see this 
> comment,
>
>       /* Figure out how many bytes to fetch.  Don't span a page
>          boundary since that might cause an unnecessary memory
>          error.  */
>
> Looks existing code takes care of not crossing the page boundary, 
> similarly, we should also take care of not crossing the section 
> boundary.  What do you think?

thanks, check section boundary looks better, and I think we can remove 
the old page boundary check, please CR the new patch

gdb/ChangeLog:

         * tilegx-tdep.c (tilegx_skip_prologue): when prefetching
         multiple instruction bundles, check section boundary
         instead of page boundary.

Regards,
Jiong






[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: section.patch.patch --]
[-- Type: text/plain; charset="gb18030"; name="section.patch.patch", Size: 1204 bytes --]

---
 gdb/tilegx-tdep.c | 12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index 2c4e349..f8a6255 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -424,15 +424,18 @@ tilegx_analyze_prologue (struct gdbarch* gdbarch,
       /* Retrieve the next instruction.  */
       if (next_addr - instbuf_start >= instbuf_size)
 	{
-	  /* Figure out how many bytes to fetch.  Don't span a page
+	  /* Figure out how many bytes to fetch.  Don't span a section
 	     boundary since that might cause an unnecessary memory
 	     error.  */
-	  unsigned int size_on_same_page = 4096 - (next_addr & 4095);
+	  unsigned int size_on_same_section;
+	  struct obj_section *s = find_pc_section(next_addr);
+	  gdb_assert(s != NULL);
+	  size_on_same_section =
+	    s->the_bfd_section->vma + s->the_bfd_section->size - next_addr;
 
 	  instbuf_size = sizeof instbuf;
 
-	  if (instbuf_size > size_on_same_page)
-	    instbuf_size = size_on_same_page;
+	  if (instbuf_size > size_on_same_section)
+	    instbuf_size = size_on_same_section;
 	  instbuf_start = next_addr;
 
 	  status = safe_frame_unwind_memory (next_frame, instbuf_start,
-- 
1.8.1

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Yao Qi]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb18030"; format=flowed, Size: 1718 bytes --]

On 02/20/2013 10:48 AM, Jiong Wang wrote:
> gdb/ChangeLog:
>
>           * tilegx-tdep.c (tilegx_skip_prologue): when prefetching
                                                    ^^^^ "When"
>           multiple instruction bundles, check section boundary
>           instead of page boundary.

Is it possible that the section layouts on two pages?  I mean, if is 
possible that NEXT_ADDR is within section FOO and page A, but the end of 
section FOO is within page A + 1.  If this is true, we need to check to 
the min (page boundary, section boundary), otherwise, we don't have to 
worry about it.

> ---
>   gdb/tilegx-tdep.c | 12 ++++++++----
>   1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
> index 2c4e349..f8a6255 100644
> --- a/gdb/tilegx-tdep.c
> +++ b/gdb/tilegx-tdep.c
> @@ -424,15 +424,18 @@ tilegx_analyze_prologue (struct gdbarch* gdbarch,
>         /* Retrieve the next instruction.  */
>         if (next_addr - instbuf_start >= instbuf_size)
>   	{
> -	  /* Figure out how many bytes to fetch.  Don't span a page
> +	  /* Figure out how many bytes to fetch.  Don't span a section
>   	     boundary since that might cause an unnecessary memory
>   	     error.  */
> -	  unsigned int size_on_same_page = 4096 - (next_addr & 4095);
> +	  unsigned int size_on_same_section;
> +	  struct obj_section *s = find_pc_section(next_addr);

A space is needed between find_pc_section and "(".  A blank line is 
needed here as well.

> +	  gdb_assert(s != NULL);
A space is needed between gdb_assert and "(".

I have no other comments on this patch.  It looks good now, but it still 
needs a review and approval from maintainers.

-- 
Yao (ÆëÒ¢)

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Jiong Wang]

[-- Attachment #1: Type: text/plain, Size: 1985 bytes --]

On 02/20/2013 12:10 PM, Yao Qi wrote:
> Is it possible that the section layouts on two pages?  I mean, if is 
> possible that NEXT_ADDR is within section FOO and page A, but the end 
> of section FOO is within page A + 1.  If this is true, we need to 
> check to the min (page boundary, section boundary), otherwise, we 
> don't have to worry about it.

    sure, and I guess the page boundary check is for the risk that the 
next page is not in memory that need to be paged in.
    but some of the time, we just need to analyze a few instructions 
then we could get the result, so we only cross a page when necessary, 
but this do not make sense for disk file access.

    after a second think, I fell it's reasonable that 
"section_table_xfer_memory_partial" do not handle those gap between 
sections, because there is no bit on the disk file for those gap, while 
if the debuggee is loaded and under running, then target_read_memory 
will use ptrace to fetch runtime memory, then those gap has physical map 
in memory, and set to zero.

    for x86, this is a issue also. for a simple testcase

char *fmt = "x%d\n";
int main(int argc, char **argv)
{
         printf(fmt, argc);
         return 0;
}

gcc test.c
gdb a.out
(gdb) x/10 fmt
0x4005c0 <__dso_handle+8>:    174335352    Cannot access memory at 
address 0x4005c4
(gdb) b main
Breakpoint 1 at 0x4004e0
(gdb) r
Starting program: /home/jiwang/GDB-TEST/a.out
Breakpoint 1, 0x00000000004004e0 in main ()
(gdb) x/10 fmt
0x4005c0 <__dso_handle+8>:    174335352    0    990059265 44
0x4005d0:    4    -552    72    -236
0x4005e0:    112    -184

so, I think fix this issue by checking section boundary simultaneously 
is a bit strange, the clean and proper way is to stop skip_prologue 
analysis when the pc is in plt stub.

below is the old patch, any one comments on this?


gdb/ChangeLog:

        * tilegx-tdep.c (tilegx_skip_prologue): simplify the handling for
        plt stub.

-- 
Regards,
Jiong. Wang
Tilera Corporation.


[-- Attachment #2: 0001-xx.patch --]
[-- Type: text/x-patch, Size: 588 bytes --]

---
 gdb/tilegx-tdep.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index 2c4e349..2452232 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -758,6 +758,10 @@ tilegx_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
         return max (start_pc, post_prologue_pc);
     }
 
+  /* for plt stub, just return the start pc */
+  if (in_plt_section (start_pc, NULL))
+    return start_pc;
+
   /* Otherwise, try to skip prologue the hard way.  */
   return tilegx_analyze_prologue (gdbarch,
 				  start_pc,
-- 
1.7.10.4

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Jiong Wang]

On 02/21/2013 09:40 PM, Jiong Wang wrote:
> On 02/20/2013 12:10 PM, Yao Qi wrote:
>> Is it possible that the section layouts on two pages?  I mean, if is 
>> possible that NEXT_ADDR is within section FOO and page A, but the end 
>> of section FOO is within page A + 1.  If this is true, we need to 
>> check to the min (page boundary, section boundary), otherwise, we 
>> don't have to worry about it.
>
>    sure, and I guess the page boundary check is for the risk that the 
> next page is not in memory that need to be paged in.
>    but some of the time, we just need to analyze a few instructions 
> then we could get the result, so we only cross a page when necessary, 
> but this do not make sense for disk file access.
>
>    after a second think, I fell it's reasonable that 
> "section_table_xfer_memory_partial" do not handle those gap between 
> sections, because there is no bit on the disk file for those gap, 
> while if the debuggee is loaded and under running, then 
> target_read_memory will use ptrace to fetch runtime memory, then those 
> gap has physical map in memory, and set to zero.
>
>    for x86, this is a issue also. for a simple testcase
>
> char *fmt = "x%d\n";
> int main(int argc, char **argv)
> {
>         printf(fmt, argc);
>         return 0;
> }
>
> gcc test.c
> gdb a.out
> (gdb) x/10 fmt
> 0x4005c0 <__dso_handle+8>:    174335352    Cannot access memory at 
> address 0x4005c4
> (gdb) b main
> Breakpoint 1 at 0x4004e0
> (gdb) r
> Starting program: /home/jiwang/GDB-TEST/a.out
> Breakpoint 1, 0x00000000004004e0 in main ()
> (gdb) x/10 fmt
> 0x4005c0 <__dso_handle+8>:    174335352    0    990059265 44
> 0x4005d0:    4    -552    72    -236
> 0x4005e0:    112    -184
>
> so, I think fix this issue by checking section boundary simultaneously 
> is a bit strange, the clean and proper way is to stop skip_prologue 
> analysis when the pc is in plt stub.
>
> below is the old patch, any one comments on this?
>

Ping, could anyone have a review on this?

>
> gdb/ChangeLog:
>
>        * tilegx-tdep.c (tilegx_skip_prologue): simplify the handling for
>        plt stub.
>


-- 
Regards,
Jiong. Wang
Tilera Corporation.

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Pedro Alves]

On 02/21/2013 01:40 PM, Jiong Wang wrote:
> On 02/20/2013 12:10 PM, Yao Qi wrote:
>> Is it possible that the section layouts on two pages?  I mean, if is possible that NEXT_ADDR is within section FOO and page A, but the end of section FOO is within page A + 1.  If this is true, we need to check to the min (page boundary, section boundary), otherwise, we don't have to worry about it.
> 
>    sure, and I guess the page boundary check is for the risk that the next page is not in memory that need to be paged in.

No, it should be for the case of the next page not being mapped at all.  Like when straddling a segment.
Paging should be transparent to ptrace.

>    but some of the time, we just need to analyze a few instructions then we could get the result, so we only cross a page when necessary, but this do not make sense for disk file access.
> 
>    after a second think, I fell it's reasonable that "section_table_xfer_memory_partial" do not handle those gap between sections, because there is no bit on the disk file for those gap, while if the debuggee is loaded and under running, then target_read_memory will use ptrace to fetch runtime memory, then those gap has physical map in memory, and set to zero.
> 

Right.

>    for x86, this is a issue also. for a simple testcase
> 
> char *fmt = "x%d\n";
> int main(int argc, char **argv)
> {
>         printf(fmt, argc);
>         return 0;
> }
> 
> gcc test.c
> gdb a.out
> (gdb) x/10 fmt
> 0x4005c0 <__dso_handle+8>:    174335352    Cannot access memory at address 0x4005c4
> (gdb) b main
> Breakpoint 1 at 0x4004e0
> (gdb) r
> Starting program: /home/jiwang/GDB-TEST/a.out
> Breakpoint 1, 0x00000000004004e0 in main ()
> (gdb) x/10 fmt
> 0x4005c0 <__dso_handle+8>:    174335352    0    990059265 44
> 0x4005d0:    4    -552    72    -236
> 0x4005e0:    112    -184
> 
> so, I think fix this issue by checking section boundary simultaneously is a bit strange, the clean and proper way is to stop skip_prologue analysis when the pc is in plt stub.

I do agree that trying to find the end of the prologue of a plt stub is futile.  We know plt stubs
aren't "normal" functions, and don't have prologues.

But, I do think the prologue analyzer still has a problem.  You should see this
same issue with any small normal function (with no debug info) that happens to
end up close enough to the end of its section.

I suggest limiting the end address of the analysis with something like
in tilegx_skip_prologue

+ /* Don't straddle a section boundary.  */
+ s = find_pc_section (start_pc);
+ end_pc = start_pc + 8 * TILEGX_BUNDLE_SIZE_IN_BYTES;
+ if (s != NULL)
+   end_pc = min (end_pc, obj_section_endaddr (s));

  return tilegx_analyze_prologue (gdbarch,
				  start_pc,
-				  start_pc + 8 * TILEGX_BUNDLE_SIZE_IN_BYTES,
+				  end_pc,

and also, make tilegx_analyze_prologue never touch memory
over end_addr.  It doesn't seem to take that care currently?

-- 
Pedro Alves

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Jiong Wang]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

On 03/01/2013 07:30 PM, Pedro Alves wrote
> I do agree that trying to find the end of the prologue of a plt stub is futile.  We know plt stubs
> aren't "normal" functions, and don't have prologues.
>
> But, I do think the prologue analyzer still has a problem.  You should see this
> same issue with any small normal function (with no debug info) that happens to
> end up close enough to the end of its section.

   I agree, but normally there will be crtn* files which will placed 
after the noraml
function, so that the distance to the section with different alignment, 
for example .eh_frame_hdr,
will be long enough to prevent this happen.

>
> I suggest limiting the end address of the analysis with something like
> in tilegx_skip_prologue
>
> + /* Don't straddle a section boundary.  */
> + s = find_pc_section (start_pc);
> + end_pc = start_pc + 8 * TILEGX_BUNDLE_SIZE_IN_BYTES;
> + if (s != NULL)
> +   end_pc = min (end_pc, obj_section_endaddr (s));
>
>    return tilegx_analyze_prologue (gdbarch,
> 				  start_pc,
> -				  start_pc + 8 * TILEGX_BUNDLE_SIZE_IN_BYTES,
> +				  end_pc,
>
> and also, make tilegx_analyze_prologue never touch memory
> over end_addr.  It doesn't seem to take that care currently?
    I think this fix make sense, and tilegx_analyze_prologue has glitch, 
it will ignore "end_pc" to some extent.
  the prefetch buffer in tilegx_analyze_prologue should consider the end_pc.

    I have tested the new patch by rerun dejagnu, please review, thanks.

    gdb/ChangeLog:

         * tilegx-tdep.c (tilegx_analyze_prologue): Improve the evaluation
         of "instbuf_size".
         (tilegx_skip_prologue): Improve the evaluation of the end address
         for prologue analyze.

-- 
Regards,
Jiong. Wang
Tilera Corporation.


[-- Attachment #2: new.patch --]
[-- Type: text/x-patch, Size: 1471 bytes --]

---
 gdb/tilegx-tdep.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index f45c20f..b398507 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -433,6 +433,8 @@ tilegx_analyze_prologue (struct gdbarch* gdbarch,
 
 	  if (instbuf_size > size_on_same_page)
 	    instbuf_size = size_on_same_page;
+
+	  instbuf_size = min (instbuf_size, (end_addr - next_addr));
 	  instbuf_start = next_addr;
 
 	  status = safe_frame_unwind_memory (next_frame, instbuf_start,
@@ -745,7 +747,8 @@ tilegx_analyze_prologue (struct gdbarch* gdbarch,
 static CORE_ADDR
 tilegx_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
 {
-  CORE_ADDR func_start;
+  CORE_ADDR func_start, end_pc;
+  struct obj_section *s = NULL;
 
   /* This is the preferred method, find the end of the prologue by
      using the debugging information.  */
@@ -758,10 +761,16 @@ tilegx_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
         return max (start_pc, post_prologue_pc);
     }
 
+  /* Don't straddle a section boundary.  */
+  s = find_pc_section (start_pc);
+  end_pc = start_pc + 8 * TILEGX_BUNDLE_SIZE_IN_BYTES;
+  if (s != NULL)
+    end_pc = min (end_pc, obj_section_endaddr (s));
+
   /* Otherwise, try to skip prologue the hard way.  */
   return tilegx_analyze_prologue (gdbarch,
 				  start_pc,
-				  start_pc + 8 * TILEGX_BUNDLE_SIZE_IN_BYTES,
+				  end_pc,
 				  NULL, NULL);
 }
 

Re: [RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Pedro Alves]

On 03/01/2013 02:07 PM, Jiong Wang wrote:
> 
>         * tilegx-tdep.c (tilegx_analyze_prologue): Improve the evaluation
>         of "instbuf_size".
>         (tilegx_skip_prologue): Improve the evaluation of the end address
>         for prologue analyze.

"improve" is a "Why?", and subjective.  ChangeLogs are objective, and document
the "What" changed.  I suggest:

         * tilegx-tdep.c (tilegx_analyze_prologue): Limit bundle reading to END_ADDR.
         (tilegx_skip_prologue): Limit prologue analysis to section end.

>  tilegx_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
>  {
> -  CORE_ADDR func_start;
> +  CORE_ADDR func_start, end_pc;
> +  struct obj_section *s = NULL;

No need to initialize 's' here, it's unconditionally initialized below.

> +  s = find_pc_section (start_pc);

OK with these changes.

Thanks.

-- 
Pedro Alves

[COMMITTED][RFC/TileGX 2/6] simplify the handling of skip prologue for plt stub

[Jiong Wang]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On 03/01/2013 11:59 PM, Pedro Alves wrote:
> "improve" is a "Why?", and subjective.  ChangeLogs are objective, and document
> the "What" changed.  I suggest:
>
>           * tilegx-tdep.c (tilegx_analyze_prologue): Limit bundle reading to END_ADDR.
>           (tilegx_skip_prologue): Limit prologue analysis to section end.
>
>>   tilegx_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
>>   {
>> -  CORE_ADDR func_start;
>> +  CORE_ADDR func_start, end_pc;
>> +  struct obj_section *s = NULL;
> No need to initialize 's' here, it's unconditionally initialized below.
>
>> +  s = find_pc_section (start_pc);
> OK with these changes.
>
> Thanks.
>
thanks, all fixed, and committed.

-- 
Regards,
Jiong. Wang
Tilera Corporation.


[-- Attachment #2: committed.patch --]
[-- Type: text/x-patch, Size: 2022 bytes --]

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.15197
diff -u -r1.15197 ChangeLog
--- gdb/ChangeLog	1 Mar 2013 21:18:18 -0000	1.15197
+++ gdb/ChangeLog	2 Mar 2013 01:31:04 -0000
@@ -1,3 +1,10 @@
+2013-03-01  Jiong Wang  <jiwang@tilera.com>
+	    Pedro Alves  <palves@redhat.com>
+
+	* tilegx-tdep.c (tilegx_analyze_prologue): Limit bundle reading
+	to END_ADDR.
+	(tilegx_skip_prologue): Limit prologue analysis to section end.
+
 2013-03-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* dwarf2loc.c (call_site_find_chain_1): New variable save_callee_pc,
Index: gdb/tilegx-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/tilegx-tdep.c,v
retrieving revision 1.9
diff -u -r1.9 tilegx-tdep.c
--- gdb/tilegx-tdep.c	1 Mar 2013 10:45:28 -0000	1.9
+++ gdb/tilegx-tdep.c	2 Mar 2013 01:31:29 -0000
@@ -433,6 +433,8 @@
 
 	  if (instbuf_size > size_on_same_page)
 	    instbuf_size = size_on_same_page;
+
+	  instbuf_size = min (instbuf_size, (end_addr - next_addr));
 	  instbuf_start = next_addr;
 
 	  status = safe_frame_unwind_memory (next_frame, instbuf_start,
@@ -745,7 +747,8 @@
 static CORE_ADDR
 tilegx_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
 {
-  CORE_ADDR func_start;
+  CORE_ADDR func_start, end_pc;
+  struct obj_section *s;
 
   /* This is the preferred method, find the end of the prologue by
      using the debugging information.  */
@@ -758,10 +761,16 @@
         return max (start_pc, post_prologue_pc);
     }
 
+  /* Don't straddle a section boundary.  */
+  s = find_pc_section (start_pc);
+  end_pc = start_pc + 8 * TILEGX_BUNDLE_SIZE_IN_BYTES;
+  if (s != NULL)
+    end_pc = min (end_pc, obj_section_endaddr (s));
+
   /* Otherwise, try to skip prologue the hard way.  */
   return tilegx_analyze_prologue (gdbarch,
 				  start_pc,
-				  start_pc + 8 * TILEGX_BUNDLE_SIZE_IN_BYTES,
+				  end_pc,
 				  NULL, NULL);
 }