[Patch, microblaze]: Communicate in larger blocks with the target.

[Patch, microblaze]: Communicate in larger blocks with the target.

[Ajit Kumar Agarwal]

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

Please find the following patch.    

 [Patch, microblaze]: Communicate in larger blocks with the target.
    
    Communicate in larger blocks with the target. The chunk of memory
    will be read from the target and then used in microblaze_analyze_prologue.
    The above process minimizes the transaction with the Debug Agent.
    
    ChangeLog:
    2014-06-17 Ajit Agarwal <ajitkum@xilinx.com>
    
        * microblaze-tdep.c (microblaze_analyze_prologue): Use of
        target_read_memory. Populate insn_block. Use of insn_block.
    
    Signed-off-by:Ajit Agarwal ajitkum@xilinx.com

Thanks & Regards
Ajit

[-- Attachment #2: 0001-Patch-microblaze-Communicate-in-larger-blocks-with-t.patch --]
[-- Type: application/octet-stream, Size: 2782 bytes --]

From ba8a22d10755ab7835f67b1ea553e089ad8767da Mon Sep 17 00:00:00 2001
From: Ajit Kumar Agarwal <ajitkum@xhdspdgnu.(none)>
Date: Tue, 17 Jun 2014 14:27:02 +0530
Subject: [PATCH] [Patch, microblaze]: Communicate in larger blocks with the target.

Communicate in larger blocks with the target. The chunk of memory
will be read from the target and then used in microblaze_analyze_prologue.
The above process minimizes the transaction with the Debug Agent.

ChangeLog:
2014-06-17 Ajit Agarwal <ajitkum@xilinx.com>

	* microblaze-tdep.c (microblaze_analyze_prologue): Use of
	target_read_memory. Populate insn_block. Use of insn_block.

Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
---
 gdb/microblaze-tdep.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
index 14c1b52..b1efbe2 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -234,6 +234,10 @@ microblaze_analyze_prologue (struct gdbarch *gdbarch, CORE_ADDR pc,
   int flags = 0;
   int save_hidden_pointer_found = 0;
   int non_stack_instruction_found = 0;
+  int n_insns; 
+  unsigned long *insn_block; 
+  gdb_byte *buf_block; 
+  int ti, tj; 
 
   /* Find the start of this function.  */
   find_pc_partial_function (pc, &name, &func_addr, &func_end);
@@ -272,10 +276,23 @@ microblaze_analyze_prologue (struct gdbarch *gdbarch, CORE_ADDR pc,
   microblaze_debug ("Scanning prologue: name=%s, func_addr=%s, stop=%s\n", 
 		    name, paddress (gdbarch, func_addr), 
 		    paddress (gdbarch, stop));
-
+  
+  /*Do a block read to minimize the transaction with the Debug Agent */ 
+  n_insns = (stop == func_addr) ? 1 : ((stop - func_addr) / INST_WORD_SIZE); 
+  insn_block = (unsigned long *)calloc(n_insns, sizeof(unsigned long)); 
+  buf_block = (gdb_byte *)calloc(n_insns * INST_WORD_SIZE, sizeof(gdb_byte)); 
+   
+  target_read_memory (func_addr, buf_block, n_insns * INST_WORD_SIZE ); 
+   
+  for (ti = 0; ti < n_insns; ti++) { 
+    insn_block[ti] = 0; 
+    for (tj = ti * INST_WORD_SIZE; tj < (ti + 1) * INST_WORD_SIZE; tj++) 
+      insn_block[ti] = (insn_block[ti] << 8) | buf_block[tj]; 
+  } 
+  
   for (addr = func_addr; addr < stop; addr += INST_WORD_SIZE)
     {
-      insn = microblaze_fetch_instruction (addr);
+      insn = insn_block[(addr - func_addr) / INST_WORD_SIZE];
       op = microblaze_decode_insn (insn, &rd, &ra, &rb, &imm);
       microblaze_debug ("%s %08lx\n", paddress (gdbarch, pc), insn);
 
@@ -401,6 +418,9 @@ microblaze_analyze_prologue (struct gdbarch *gdbarch, CORE_ADDR pc,
      part of the prologue.  */
   if (save_hidden_pointer_found)
     prologue_end_addr -= INST_WORD_SIZE;
+  
+  free(insn_block); 
+  free(buf_block); 
 
   return prologue_end_addr;
 }
-- 
1.7.1

Re: [Patch, microblaze]: Communicate in larger blocks with the target.

[Pedro Alves]

On 06/17/2014 10:03 AM, Ajit Kumar Agarwal wrote:
> Please find the following patch.    
> 
>  [Patch, microblaze]: Communicate in larger blocks with the target.
>     
>     Communicate in larger blocks with the target. The chunk of memory
>     will be read from the target and then used in microblaze_analyze_prologue.
>     The above process minimizes the transaction with the Debug Agent.

We have core infrustructure for this now, in the form of a
code cache that reads ahead.  Could you try using it?
All you have to do is replace target_read_memory calls that are
actually reading code, with target_read_code calls.  See i386-tdep.c
for example.

-- 
Pedro Alves

Re: [Patch, microblaze]: Communicate in larger blocks with the target.

[Pedro Alves]

On 06/17/2014 02:49 PM, Pedro Alves wrote:
> On 06/17/2014 10:03 AM, Ajit Kumar Agarwal wrote:
>> Please find the following patch.    
>>
>>  [Patch, microblaze]: Communicate in larger blocks with the target.
>>     
>>     Communicate in larger blocks with the target. The chunk of memory
>>     will be read from the target and then used in microblaze_analyze_prologue.
>>     The above process minimizes the transaction with the Debug Agent.
> 
> We have core infrustructure for this now, in the form of a
> code cache that reads ahead.  Could you try using it?
> All you have to do is replace target_read_memory calls that are
> actually reading code, with target_read_code calls.  See i386-tdep.c
> for example.

To be clear, I'm not talking about changing the new calls in your
patch, but instead, to change the existing calls.  Then your
patch won't be necessary.

-- 
Pedro Alves

RE: [Patch, microblaze]: Communicate in larger blocks with the target.

[Ajit Kumar Agarwal]

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Tuesday, June 17, 2014 10:05 PM
To: Pedro Alves; Ajit Kumar Agarwal; gdb-patches@sourceware.org
Cc: Michael Eager; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch, microblaze]: Communicate in larger blocks with the target.

On 06/17/2014 02:49 PM, Pedro Alves wrote:
> On 06/17/2014 10:03 AM, Ajit Kumar Agarwal wrote:
>> Please find the following patch.    
>>
>>  [Patch, microblaze]: Communicate in larger blocks with the target.
>>     
>>     Communicate in larger blocks with the target. The chunk of memory
>>     will be read from the target and then used in microblaze_analyze_prologue.
>>     The above process minimizes the transaction with the Debug Agent.
> 
> We have core infrustructure for this now, in the form of a code cache 
> that reads ahead.  Could you try using it?
> All you have to do is replace target_read_memory calls that are 
> actually reading code, with target_read_code calls.  See i386-tdep.c 
> for example.

>>To be clear, I'm not talking about changing the new calls in your patch, but instead, to change the existing calls.  Then your patch won't be necessary.

Thanks Pedro !!.  Would you mind explaining this in detail.
--
Pedro Alves

Re: [Patch, microblaze]: Communicate in larger blocks with the target.

[Pedro Alves]

On 06/17/2014 05:49 PM, Ajit Kumar Agarwal wrote:
> 
> 
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com] 
> Sent: Tuesday, June 17, 2014 10:05 PM
> To: Pedro Alves; Ajit Kumar Agarwal; gdb-patches@sourceware.org
> Cc: Michael Eager; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
> Subject: Re: [Patch, microblaze]: Communicate in larger blocks with the target.
> 
> On 06/17/2014 02:49 PM, Pedro Alves wrote:
>> On 06/17/2014 10:03 AM, Ajit Kumar Agarwal wrote:
>>> Please find the following patch.    
>>>
>>>  [Patch, microblaze]: Communicate in larger blocks with the target.
>>>     
>>>     Communicate in larger blocks with the target. The chunk of memory
>>>     will be read from the target and then used in microblaze_analyze_prologue.
>>>     The above process minimizes the transaction with the Debug Agent.
>>
>> We have core infrustructure for this now, in the form of a code cache 
>> that reads ahead.  Could you try using it?
>> All you have to do is replace target_read_memory calls that are 
>> actually reading code, with target_read_code calls.  See i386-tdep.c 
>> for example.
> 
>>> To be clear, I'm not talking about changing the new calls in your patch, but instead, to change the existing calls.  Then your patch won't be necessary.
> 
> Thanks Pedro !!.  Would you mind explaining this in detail.

See 0865b04a4dec8a458bee54081b5598a6268b0724.

-- 
Pedro Alves

RE: [Patch, microblaze]: Communicate in larger blocks with the target.

[Ajit Kumar Agarwal]

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

Hello Pedro:

Review feedback are incorporated. Could you please review and let me know if its okay.

[Patch, microblaze]: Use target_read_code in microblaze_fetch_instruction

This patch uses target_read_code instead of target_read_memory in
microblaze_fetch instruction in order to use cache memory accesses
requested in target_read_code.

ChangeLog:
2014-06-17 Ajit Agarwal <ajitkum@xilinx.com>

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

diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
index 14c1b52..4e5b2d5 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -126,7 +126,7 @@ microblaze_fetch_instruction (CORE_ADDR pc)
   gdb_byte buf[4];
 
   /* If we can't read the instruction at PC, return zero.  */
-  if (target_read_memory (pc, buf, sizeof (buf)))
+  if (target_read_code (pc, buf, sizeof (buf)))
     return 0;
 
   return extract_unsigned_integer (buf, 4, byte_order);
-- 
1.7.1
-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Tuesday, June 17, 2014 10:22 PM
To: Ajit Kumar Agarwal; gdb-patches@sourceware.org
Cc: Michael Eager; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch, microblaze]: Communicate in larger blocks with the target.

On 06/17/2014 05:49 PM, Ajit Kumar Agarwal wrote:
> 
> 
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Tuesday, June 17, 2014 10:05 PM
> To: Pedro Alves; Ajit Kumar Agarwal; gdb-patches@sourceware.org
> Cc: Michael Eager; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju 
> Mekala
> Subject: Re: [Patch, microblaze]: Communicate in larger blocks with the target.
> 
> On 06/17/2014 02:49 PM, Pedro Alves wrote:
>> On 06/17/2014 10:03 AM, Ajit Kumar Agarwal wrote:
>>> Please find the following patch.    
>>>
>>>  [Patch, microblaze]: Communicate in larger blocks with the target.
>>>     
>>>     Communicate in larger blocks with the target. The chunk of memory
>>>     will be read from the target and then used in microblaze_analyze_prologue.
>>>     The above process minimizes the transaction with the Debug Agent.
>>
>> We have core infrustructure for this now, in the form of a code cache 
>> that reads ahead.  Could you try using it?
>> All you have to do is replace target_read_memory calls that are 
>> actually reading code, with target_read_code calls.  See i386-tdep.c 
>> for example.
> 
>>> To be clear, I'm not talking about changing the new calls in your patch, but instead, to change the existing calls.  Then your patch won't be necessary.
> 
> Thanks Pedro !!.  Would you mind explaining this in detail.

See 0865b04a4dec8a458bee54081b5598a6268b0724.

--
Pedro Alves

[-- Attachment #2: 0001-Patch-microblaze-Use-target_read_code-in-microblaze_.patch --]
[-- Type: application/octet-stream, Size: 1168 bytes --]

From 9c629c55170e0f0a8165d1245b9326a03a0d327b Mon Sep 17 00:00:00 2001
From: Ajit Kumar Agarwal <ajitkum@xhdspdgnu.(none)>
Date: Tue, 17 Jun 2014 23:29:44 +0530
Subject: [PATCH] [Patch, microblaze]: Use target_read_code in microblaze_fetch_instruction

This patch uses target_read_code instead of target_read_memory in
microblaze_fetch instruction in order to use cache memory accesses
requested in target_read_code.

ChangeLog:
2014-06-17 Ajit Agarwal <ajitkum@xilinx.com>

	* microblaze-tdep.c (microblaze_fetch_instruction): Use of
	target_read_code.

Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
---
 gdb/microblaze-tdep.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
index 14c1b52..4e5b2d5 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -126,7 +126,7 @@ microblaze_fetch_instruction (CORE_ADDR pc)
   gdb_byte buf[4];
 
   /* If we can't read the instruction at PC, return zero.  */
-  if (target_read_memory (pc, buf, sizeof (buf)))
+  if (target_read_code (pc, buf, sizeof (buf)))
     return 0;
 
   return extract_unsigned_integer (buf, 4, byte_order);
-- 
1.7.1

RE: [Patch, microblaze]: Communicate in larger blocks with the target.

[Ajit Kumar Agarwal]

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

Sorry Pedro. The ChangeLog  was missing. Could you please review  and let me know if its okay.

    [Patch, microblaze]: Use target_read_code in microblaze_fetch_instruction
    
    This patch uses target_read_code instead of target_read_memory in
    microblaze_fetch instruction in order to use cache memory accesses
    requested in target_read_code.
    
    ChangeLog:
    2014-06-17 Ajit Agarwal <ajitkum@xilinx.com>
    
        * microblaze-tdep.c (microblaze_fetch_instruction): Use of
        target_read_code.
    
    Signed-off-by:Ajit Agarwal ajitkum@xilinx.com

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

diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
index 14c1b52..4e5b2d5 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -126,7 +126,7 @@ microblaze_fetch_instruction (CORE_ADDR pc)
   gdb_byte buf[4];
 
   /* If we can't read the instruction at PC, return zero.  */
-  if (target_read_memory (pc, buf, sizeof (buf)))
+  if (target_read_code (pc, buf, sizeof (buf)))
     return 0;
 
   return extract_unsigned_integer (buf, 4, byte_order);
-- 
1.7.1

Thanks & Regards
Ajit
-----Original Message-----
From: Ajit Kumar Agarwal 
Sent: Tuesday, June 17, 2014 11:40 PM
To: 'Pedro Alves'; gdb-patches@sourceware.org
Cc: Michael Eager; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: RE: [Patch, microblaze]: Communicate in larger blocks with the target.

Hello Pedro:

Review feedback are incorporated. Could you please review and let me know if its okay.

[Patch, microblaze]: Use target_read_code in microblaze_fetch_instruction

This patch uses target_read_code instead of target_read_memory in microblaze_fetch instruction in order to use cache memory accesses requested in target_read_code.

ChangeLog:
2014-06-17 Ajit Agarwal <ajitkum@xilinx.com>

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

diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c index 14c1b52..4e5b2d5 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -126,7 +126,7 @@ microblaze_fetch_instruction (CORE_ADDR pc)
   gdb_byte buf[4];
 
   /* If we can't read the instruction at PC, return zero.  */
-  if (target_read_memory (pc, buf, sizeof (buf)))
+  if (target_read_code (pc, buf, sizeof (buf)))
     return 0;
 
   return extract_unsigned_integer (buf, 4, byte_order);
--
1.7.1
-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com]
Sent: Tuesday, June 17, 2014 10:22 PM
To: Ajit Kumar Agarwal; gdb-patches@sourceware.org
Cc: Michael Eager; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch, microblaze]: Communicate in larger blocks with the target.

On 06/17/2014 05:49 PM, Ajit Kumar Agarwal wrote:
> 
> 
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Tuesday, June 17, 2014 10:05 PM
> To: Pedro Alves; Ajit Kumar Agarwal; gdb-patches@sourceware.org
> Cc: Michael Eager; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju 
> Mekala
> Subject: Re: [Patch, microblaze]: Communicate in larger blocks with the target.
> 
> On 06/17/2014 02:49 PM, Pedro Alves wrote:
>> On 06/17/2014 10:03 AM, Ajit Kumar Agarwal wrote:
>>> Please find the following patch.    
>>>
>>>  [Patch, microblaze]: Communicate in larger blocks with the target.
>>>     
>>>     Communicate in larger blocks with the target. The chunk of memory
>>>     will be read from the target and then used in microblaze_analyze_prologue.
>>>     The above process minimizes the transaction with the Debug Agent.
>>
>> We have core infrustructure for this now, in the form of a code cache 
>> that reads ahead.  Could you try using it?
>> All you have to do is replace target_read_memory calls that are 
>> actually reading code, with target_read_code calls.  See i386-tdep.c 
>> for example.
> 
>>> To be clear, I'm not talking about changing the new calls in your patch, but instead, to change the existing calls.  Then your patch won't be necessary.
> 
> Thanks Pedro !!.  Would you mind explaining this in detail.

See 0865b04a4dec8a458bee54081b5598a6268b0724.

--
Pedro Alves

[-- Attachment #2: 0001-Patch-microblaze-Use-target_read_code-in-microblaze_.patch --]
[-- Type: application/octet-stream, Size: 1168 bytes --]

From 9c629c55170e0f0a8165d1245b9326a03a0d327b Mon Sep 17 00:00:00 2001
From: Ajit Kumar Agarwal <ajitkum@xhdspdgnu.(none)>
Date: Tue, 17 Jun 2014 23:29:44 +0530
Subject: [PATCH] [Patch, microblaze]: Use target_read_code in microblaze_fetch_instruction

This patch uses target_read_code instead of target_read_memory in
microblaze_fetch instruction in order to use cache memory accesses
requested in target_read_code.

ChangeLog:
2014-06-17 Ajit Agarwal <ajitkum@xilinx.com>

	* microblaze-tdep.c (microblaze_fetch_instruction): Use of
	target_read_code.

Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
---
 gdb/microblaze-tdep.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
index 14c1b52..4e5b2d5 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -126,7 +126,7 @@ microblaze_fetch_instruction (CORE_ADDR pc)
   gdb_byte buf[4];
 
   /* If we can't read the instruction at PC, return zero.  */
-  if (target_read_memory (pc, buf, sizeof (buf)))
+  if (target_read_code (pc, buf, sizeof (buf)))
     return 0;
 
   return extract_unsigned_integer (buf, 4, byte_order);
-- 
1.7.1

Re: [Patch, microblaze]: Communicate in larger blocks with the target.

[Pedro Alves]

On 06/17/2014 07:12 PM, Ajit Kumar Agarwal wrote:
> Sorry Pedro. The ChangeLog  was missing. Could you please review  and let me know if its okay.

Yes, looks OK to me.

Michael is the microblaze maintainer so please wait for
his comments/approval.

Thanks,
-- 
Pedro Alves

Re: [Patch, microblaze]: Communicate in larger blocks with the target.

[Michael Eager]

On 06/17/14 11:12, Ajit Kumar Agarwal wrote:
> Sorry Pedro. The ChangeLog  was missing. Could you please review  and let me know if its okay.
>
>      [Patch, microblaze]: Use target_read_code in microblaze_fetch_instruction
>
>      This patch uses target_read_code instead of target_read_memory in
>      microblaze_fetch instruction in order to use cache memory accesses
>      requested in target_read_code.
>
>      ChangeLog:
>      2014-06-17 Ajit Agarwal <ajitkum@xilinx.com>
>
>          * microblaze-tdep.c (microblaze_fetch_instruction): Use of
>          target_read_code.
>
>      Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
>
> ---
>   gdb/microblaze-tdep.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
> index 14c1b52..4e5b2d5 100644
> --- a/gdb/microblaze-tdep.c
> +++ b/gdb/microblaze-tdep.c
> @@ -126,7 +126,7 @@ microblaze_fetch_instruction (CORE_ADDR pc)
>     gdb_byte buf[4];
>
>     /* If we can't read the instruction at PC, return zero.  */
> -  if (target_read_memory (pc, buf, sizeof (buf)))
> +  if (target_read_code (pc, buf, sizeof (buf)))
>       return 0;
>
>     return extract_unsigned_integer (buf, 4, byte_order);
>

Committed 3421196.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: [Patch, microblaze]: Communicate in larger blocks with the target.

[Michael Eager]

On 06/17/14 02:03, Ajit Kumar Agarwal wrote:
> Please find the following patch.
>
>   [Patch, microblaze]: Communicate in larger blocks with the target.
>
>      Communicate in larger blocks with the target. The chunk of memory
>      will be read from the target and then used in microblaze_analyze_prologue.
>      The above process minimizes the transaction with the Debug Agent.
>
>      ChangeLog:
>      2014-06-17 Ajit Agarwal <ajitkum@xilinx.com>
>
>          * microblaze-tdep.c (microblaze_analyze_prologue): Use of
>          target_read_memory. Populate insn_block. Use of insn_block.

There are coding standard issues:

1.  Comments have space after opening /*, end with a period, two spaces, */.

2.  Space before left paren in function call.

3.  Space after cast.

There may be others.  Please review GDB/GNU coding standards:
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards


+  n_insns = (stop == func_addr) ? 1 : ((stop - func_addr) / INST_WORD_SIZE);
...
    for (addr = func_addr; addr < stop; addr += INST_WORD_SIZE)

It looks to me that if (stop == func_addr), this loop will not be executed.
If that's as intended, then there's no need for the conditional expression, and
likely execution of the function can be terminated early.  If this is not intended,
then a gdb_assert (stop != func_addr) can be inserted and the conditional
expression removed.


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: [Patch, microblaze]: Communicate in larger blocks with the target.

[Tom Tromey]

>>>>> "Ajit" == Ajit Kumar Agarwal <ajit.kumar.agarwal@xilinx.com> writes:

Ajit> +  insn_block = (unsigned long *)calloc(n_insns, sizeof(unsigned long)); 
Ajit> +  buf_block = (gdb_byte *)calloc(n_insns * INST_WORD_SIZE, sizeof(gdb_byte)); 

Just for future reference -- since from other reviews it looks like the
patch will need bigger updates -- gdb doesn't use "calloc".  Instead it
uses wrapper around the standard allocation functions, e.g.,
xcalloc... though if one is allocating typed arrays then I think the
wrapper macros like XNEWVEC (see include/libiberty.h) are to be
preferred, as they are more type-safe.

Ajit> +  free(insn_block); 
Ajit> +  free(buf_block); 

Likewise gdb uses xfree.  Though in most cases one must use cleanups
instead, to be exception-safe.  I didn't look to see whether that was
the case for your patch.

Tom

Re: [Patch, microblaze]: Communicate in larger blocks with the target.

[Michael Eager]

On 06/17/14 02:03, Ajit Kumar Agarwal wrote:
> Please find the following patch.
>
>   [Patch, microblaze]: Communicate in larger blocks with the target.
>
>      Communicate in larger blocks with the target. The chunk of memory
>      will be read from the target and then used in microblaze_analyze_prologue.
>      The above process minimizes the transaction with the Debug Agent.
>
>      ChangeLog:
>      2014-06-17 Ajit Agarwal <ajitkum@xilinx.com>
>
>          * microblaze-tdep.c (microblaze_analyze_prologue): Use of
>          target_read_memory. Populate insn_block. Use of insn_block.
>
>      Signed-off-by:Ajit Agarwal ajitkum@xilinx.com

Please follow GNU coding conventions:

Remove trailing whitespace.

+  /*Do a block read to minimize the transaction with the Debug Agent */

Comments need space after /* and period and two spaces before */.

+  n_insns = (stop == func_addr) ? 1 : ((stop - func_addr) / INST_WORD_SIZE);

Recommend parens around conditional expression.

+  insn_block = (unsigned long *)calloc(n_insns, sizeof(unsigned long));
+  buf_block = (gdb_byte *)calloc(n_insns * INST_WORD_SIZE, sizeof(gdb_byte));

Spaces before function name.

+  for (ti = 0; ti < n_insns; ti++) {
+    insn_block[ti] = 0;
+    for (tj = ti * INST_WORD_SIZE; tj < (ti + 1) * INST_WORD_SIZE; tj++)
+      insn_block[ti] = (insn_block[ti] << 8) | buf_block[tj];
+  }

GNU does not use the "one true brace" format.  Please use GNU brace
and indent format.

+  free(insn_block);
+  free(buf_block);

Spaces before function name.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: [Patch, microblaze]: Communicate in larger blocks with the target.

[Michael Eager]

On 07/23/14 11:51, Michael Eager wrote:
> On 06/17/14 02:03, Ajit Kumar Agarwal wrote:
>> Please find the following patch.
>>
>>   [Patch, microblaze]: Communicate in larger blocks with the target.
>>
>>      Communicate in larger blocks with the target. The chunk of memory
>>      will be read from the target and then used in microblaze_analyze_prologue.
>>      The above process minimizes the transaction with the Debug Agent.
>>
>>      ChangeLog:
>>      2014-06-17 Ajit Agarwal <ajitkum@xilinx.com>
>>
>>          * microblaze-tdep.c (microblaze_analyze_prologue): Use of
>>          target_read_memory. Populate insn_block. Use of insn_block.
>>
>>      Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
>
> Please follow GNU coding conventions:

Never mind.  I overlooked the later patch revision.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077