[3/6] Call print_insn_mep in mep_gdb_print_insn

Message ID 1484560977-8693-4-git-send-email-yao.qi@linaro.org
State New, archived
Headers

Commit Message

Yao Qi Jan. 16, 2017, 10:02 a.m. UTC
  opcodes/mep-dis.c:mep_print_insn has already had the code to
handle the case when info->section is NULL,

  /* Picking the right ISA bitmask for the current context is tricky.  */
  if (info->section)
    {
    }
  else /* sid or gdb */
    {
    }

so that we can still cal print_insn_mep even section can't be found.
On the other hand, user can disassemble an arbitrary address which
doesn't map to any section at all.

gdb:

2017-01-10  Yao Qi  <yao.qi@linaro.org>

	* mep-tdep.c (mep_gdb_print_insn): Set info->arch
	to bfd_arch_mep.  Don't return 0 if section is not
	found.  Call print_insn_mep.
---
 gdb/mep-tdep.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
  

Comments

Luis Machado Jan. 17, 2017, 2:19 p.m. UTC | #1
On 01/16/2017 04:02 AM, Yao Qi wrote:
> opcodes/mep-dis.c:mep_print_insn has already had the code to
> handle the case when info->section is NULL,
>
>   /* Picking the right ISA bitmask for the current context is tricky.  */
>   if (info->section)
>     {
>     }
>   else /* sid or gdb */
>     {
>     }
>
> so that we can still cal print_insn_mep even section can't be found.
> On the other hand, user can disassemble an arbitrary address which
> doesn't map to any section at all.
>
> gdb:
>
> 2017-01-10  Yao Qi  <yao.qi@linaro.org>
>
> 	* mep-tdep.c (mep_gdb_print_insn): Set info->arch
> 	to bfd_arch_mep.  Don't return 0 if section is not
> 	found.  Call print_insn_mep.
> ---
>  gdb/mep-tdep.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/mep-tdep.c b/gdb/mep-tdep.c
> index 68b0c4b..b1dcc86 100644
> --- a/gdb/mep-tdep.c
> +++ b/gdb/mep-tdep.c
> @@ -1266,13 +1266,12 @@ mep_pseudo_register_write (struct gdbarch *gdbarch,
>  
>  /* Disassembly.  */
>
> -/* The mep disassembler needs to know about the section in order to
> -   work correctly.  */

Instead of removing the comment, should we point out what the effects of 
having/not having section info will be?

>  static int
>  mep_gdb_print_insn (bfd_vma pc, disassemble_info * info)
>  {
>    struct obj_section * s = find_pc_section (pc);
>
> +  info->arch = bfd_arch_mep;
>    if (s)
>      {
>        /* The libopcodes disassembly code uses the section to find the
> @@ -1280,12 +1279,9 @@ mep_gdb_print_insn (bfd_vma pc, disassemble_info * info)
>           the me_module index, and the me_module index to select the
>           right instructions to print.  */
>        info->section = s->the_bfd_section;
> -      info->arch = bfd_arch_mep;
> -	
> -      return print_insn_mep (pc, info);
>      }
> -
> -  return 0;
> +
> +  return print_insn_mep (pc, info);
>  }
>
>  
>

Otherwise looks OK.
  
Yao Qi Jan. 24, 2017, 10:08 a.m. UTC | #2
On 17-01-17 08:19:18, Luis Machado wrote:
> On 01/16/2017 04:02 AM, Yao Qi wrote:
> >
> >-/* The mep disassembler needs to know about the section in order to
> >-   work correctly.  */
> 
> Instead of removing the comment, should we point out what the
> effects of having/not having section info will be?
> 

That is the implementation detail of print_insn_mep (provided by opcodes),
and gdb should not know about.  Both gdb and objdump are the clients of
opcodes, and they need to provide precise information as much as it can.
However, they shouldn't know that what should print_insn_mep do.
  
Luis Machado Jan. 24, 2017, 1:40 p.m. UTC | #3
On 01/24/2017 04:08 AM, Yao Qi wrote:
> On 17-01-17 08:19:18, Luis Machado wrote:
>> On 01/16/2017 04:02 AM, Yao Qi wrote:
>>>
>>> -/* The mep disassembler needs to know about the section in order to
>>> -   work correctly.  */
>>
>> Instead of removing the comment, should we point out what the
>> effects of having/not having section info will be?
>>
>
> That is the implementation detail of print_insn_mep (provided by opcodes),
> and gdb should not know about.  Both gdb and objdump are the clients of
> opcodes, and they need to provide precise information as much as it can.
> However, they shouldn't know that what should print_insn_mep do.
>

Fair enough. But in that case we still have a chunk of code in that 
function explaining why the section is being used. Should that go away too?

       /* The libopcodes disassembly code uses the section to find the
          BFD, the BFD to find the ELF header, the ELF header to find
          the me_module index, and the me_module index to select the
          right instructions to print.  */
  

Patch

diff --git a/gdb/mep-tdep.c b/gdb/mep-tdep.c
index 68b0c4b..b1dcc86 100644
--- a/gdb/mep-tdep.c
+++ b/gdb/mep-tdep.c
@@ -1266,13 +1266,12 @@  mep_pseudo_register_write (struct gdbarch *gdbarch,
 
 /* Disassembly.  */
 
-/* The mep disassembler needs to know about the section in order to
-   work correctly.  */
 static int
 mep_gdb_print_insn (bfd_vma pc, disassemble_info * info)
 {
   struct obj_section * s = find_pc_section (pc);
 
+  info->arch = bfd_arch_mep;
   if (s)
     {
       /* The libopcodes disassembly code uses the section to find the
@@ -1280,12 +1279,9 @@  mep_gdb_print_insn (bfd_vma pc, disassemble_info * info)
          the me_module index, and the me_module index to select the
          right instructions to print.  */
       info->section = s->the_bfd_section;
-      info->arch = bfd_arch_mep;
-	
-      return print_insn_mep (pc, info);
     }
-  
-  return 0;
+
+  return print_insn_mep (pc, info);
 }