[PATCH-ping] VAX: Forced decoding of function entry masks (eg. for disassembling ROM images)

[PATCH-ping] VAX: Forced decoding of function entry masks (eg. for disassembling ROM images)

[Jan-Benedict Glaw]

Hi!

I'd like to ping for my last patch, introducing the capability to
better disassemble ROM or MOP boot images with eg. objdump. Andreas'
objection about a limited number of entry addresses is fixed.

2005-03-21  Jan-Benedict Glaw  <jbglaw@lug-owl.de>

	opcodes/
	* vax-dis.c: (struct private): Add a bfd_vma pointer to store
	supplied function entry mask addresses.
	(init_private_data): New; fill in supplied entry mask addresses.
	(free_private_data): New; free() entry mask addresses.
	(is_function_entry): Check if a given address is a function's
	start address by looking at supplied entry mask addresses and
	symbol information, if available.
	(print_insn_vax): Use init_private_data(), is_function_entry()
	and free_private_data().

	binutils/doc/
	* binutils.texi: Document new VAX disassembler-specific option
	-M entry:0xfooba8.


diff -Nurp src-fresh/binutils/doc/binutils.texi src-hacked/binutils/doc/binutils.texi
--- src-fresh/binutils/doc/binutils.texi	2005-03-21 13:26:04.000000000 +0100
+++ src-hacked/binutils/doc/binutils.texi	2005-03-21 20:09:33.000000000 +0100
@@ -1793,6 +1793,13 @@ rather than names, for the selected type
 You can list the available values of @var{ABI} and @var{ARCH} using
 the @option{--help} option.
 
+For VAX, you can specify function entry addresses with
+@option{-M entry:0xf00ba}. You can use this multiple times to properly
+disassemble VAX binary files that don't contain symbol tables (like
+ROM dumps). In these cases, the function entry mask would otherwise
+be decoded as VAX instructions, which would probably lead to the
+rest of the function being wrongly disassembled.
+
 @item -p
 @itemx --private-headers
 Print information that is specific to the object file format.  The exact
diff -Nurp src-fresh/opcodes/vax-dis.c src-hacked/opcodes/vax-dis.c
--- src-fresh/opcodes/vax-dis.c	2005-03-14 14:07:03.000000000 +0100
+++ src-hacked/opcodes/vax-dis.c	2005-03-21 16:29:49.000000000 +0100
@@ -17,17 +17,38 @@
    along with this program; if not, write to the Free Software
    Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  */
 
+#include <setjmp.h>
+#include <string.h>
 #include "sysdep.h"
 #include "opcode/vax.h"
 #include "dis-asm.h"
 
+/* Maximum length of an instruction.  */
+#define MAXLEN 25
+
+struct private
+{
+  /* Points to first byte not fetched.  */
+  bfd_byte *max_fetched;
+  bfd_byte the_buffer[MAXLEN];
+  bfd_vma insn_start;
+  jmp_buf bailout;
+  /* Entry mask handling  */
+  int num_entry_addr;
+  bfd_vma *entry_addr;
+};
+
 /* Local function prototypes */
 static int fetch_data PARAMS ((struct disassemble_info *, bfd_byte *));
 static int print_insn_arg
   PARAMS ((const char *, unsigned char *, bfd_vma, disassemble_info *));
 static int print_insn_mode
   PARAMS ((const char *, int, unsigned char *, bfd_vma, disassemble_info *));
-
+static int init_private_data
+  PARAMS ((struct disassemble_info *, struct private *));
+static void free_private_data PARAMS ((struct private *));
+static bfd_boolean is_function_entry
+  PARAMS ((struct disassemble_info *, bfd_vma addr));
 
 static char *reg_names[] =
 {
@@ -74,20 +95,6 @@ static char *entry_mask_bit[] =
   (p += 4, FETCH_DATA (info, p), \
    (COERCE32 ((((((p[-1] << 8) + p[-2]) << 8) + p[-3]) << 8) + p[-4])))
 
-/* Maximum length of an instruction.  */
-#define MAXLEN 25
-
-#include <setjmp.h>
-
-struct private
-{
-  /* Points to first byte not fetched.  */
-  bfd_byte *max_fetched;
-  bfd_byte the_buffer[MAXLEN];
-  bfd_vma insn_start;
-  jmp_buf bailout;
-};
-
 /* Make sure that bytes from INFO->PRIVATE_DATA->BUFFER (inclusive)
    to ADDR (exclusive) are valid.  Returns 1 for success, longjmps
    on error.  */
@@ -95,6 +102,74 @@ struct private
   ((addr) <= ((struct private *)(info->private_data))->max_fetched \
    ? 1 : fetch_data ((info), (addr)))
 
+
+/* Init our private data. This decodes supplied entry addresses, which can
+   be useful to disassemble ROM images, since there's no symbol table.  */
+static int
+init_private_data (info, priv)
+    struct disassemble_info *info;
+    struct private *priv;
+{
+  char *tmp;
+
+  priv->num_entry_addr = 0;
+  priv->entry_addr = NULL;
+
+  if (info->disassembler_options)
+    {
+      tmp = info->disassembler_options;
+      while ((tmp = strstr (tmp, "entry:")))
+	{
+	  tmp += strlen ("entry:");
+	  priv->entry_addr = realloc (priv->entry_addr, sizeof (bfd_vma)
+				      * (priv->num_entry_addr + 1));
+	  if (!priv->entry_addr)
+	    return -1;
+	  priv->entry_addr[priv->num_entry_addr++] = bfd_scan_vma (tmp, NULL,
+			  					   0);
+	}
+    }
+
+  return 0;
+}
+
+/* Free memory allocated by init_private_data()  */
+static void
+free_private_data (priv)
+    struct private *priv;
+{
+  if (priv->entry_addr)
+    free (priv->entry_addr);
+}
+
+/* Check if the given address is a known function entry. Either there must
+   be a symbol of function type at this address, or the address must be
+   a forced entry point. The later helps in disassembling ROM images, because
+   there's no symbol table at all. Forced entry points can be given by
+   supplying several -M options to objdump: -M entry:0xffbb7730 .  */
+static bfd_boolean
+is_function_entry (info, addr)
+      struct disassemble_info *info;
+      bfd_vma addr;
+{
+  int i;
+  struct private *priv = info->private_data;
+
+  /* Check if there's a BSF_FUNCTION symbol at our address.  */
+  if (info->symbols
+      && info->symbols[0]
+      && (info->symbols[0]->flags & BSF_FUNCTION)
+      && addr == bfd_asymbol_value (info->symbols[0]))
+    return TRUE;
+
+  /* Check for forced function entry address.  */
+  for (i = 0; i < priv->num_entry_addr; i++)
+    if (priv->entry_addr[i] == addr)
+      return TRUE;
+
+  return FALSE;
+}
+
 static int
 fetch_data (info, addr)
      struct disassemble_info *info;
@@ -133,6 +208,9 @@ print_insn_vax (memaddr, info)
   struct private priv;
   bfd_byte *buffer = priv.the_buffer;
 
+  if (init_private_data (info, &priv) < 0)
+    return -1;
+
   info->private_data = (PTR) &priv;
   priv.max_fetched = priv.the_buffer;
   priv.insn_start = memaddr;
@@ -140,6 +218,7 @@ print_insn_vax (memaddr, info)
   if (setjmp (priv.bailout) != 0)
     {
       /* Error return.  */
+      free_private_data (&priv);
       return -1;
     }
 
@@ -157,10 +236,7 @@ print_insn_vax (memaddr, info)
     }
 
   /* Decode function entry mask.  */
-  if (info->symbols
-      && info->symbols[0]
-      && (info->symbols[0]->flags & BSF_FUNCTION)
-      && memaddr == bfd_asymbol_value (info->symbols[0]))
+  if (is_function_entry (info, memaddr))
     {
       int i = 0;
       int register_mask = buffer[1] << 8 | buffer[0];
@@ -174,6 +250,7 @@ print_insn_vax (memaddr, info)
 
       (*info->fprintf_func) (info->stream, " >");
 
+      free_private_data (&priv);
       return 2;
     }
 
@@ -194,6 +271,7 @@ print_insn_vax (memaddr, info)
       /* Handle undefined instructions. */
       (*info->fprintf_func) (info->stream, ".word 0x%x",
 			     (buffer[0] << 8) + buffer[1]);
+      free_private_data (&priv);
       return 2;
     }
 
@@ -216,6 +294,7 @@ print_insn_vax (memaddr, info)
 	(*info->fprintf_func) (info->stream, ",");
     }
 
+  free_private_data (&priv);
   return arg - buffer;
 }
 
MfG, JBG

-- 
AWEK microdata GmbH -- Am Wellbach 4 -- 33609 Bielefeld

Re: [PATCH-ping] VAX: Forced decoding of function entry masks (eg. for disassembling ROM images)

[Nick Clifton]

Hi Jan-Benedict,


> I'd like to ping for my last patch, introducing the capability to
> better disassemble ROM or MOP boot images with eg. objdump. Andreas'
> objection about a limited number of entry addresses is fixed.

Sorry - it was in my queue to look at, but I am little behind at the moment.

> 2005-03-21  Jan-Benedict Glaw  <jbglaw@lug-owl.de>
> 
> 	opcodes/
> 	* vax-dis.c: (struct private): Add a bfd_vma pointer to store
> 	supplied function entry mask addresses.
> 	(init_private_data): New; fill in supplied entry mask addresses.
> 	(free_private_data): New; free() entry mask addresses.
> 	(is_function_entry): Check if a given address is a function's
> 	start address by looking at supplied entry mask addresses and
> 	symbol information, if available.
> 	(print_insn_vax): Use init_private_data(), is_function_entry()
> 	and free_private_data().
> 
> 	binutils/doc/
> 	* binutils.texi: Document new VAX disassembler-specific option
> 	-M entry:0xfooba8.

Approved please apply, but...


> +  /* Entry mask handling  */

comment formatting.  Should be: /* Entry mask handling.  */

> +static int init_private_data
> +  PARAMS ((struct disassemble_info *, struct private *));
> +static void free_private_data PARAMS ((struct private *));
> +static bfd_boolean is_function_entry
> +  PARAMS ((struct disassemble_info *, bfd_vma addr));

We are using ISO C() now, so there is no need for the PARAMS macro.  In 
fact if you place your function declarations carefully in the source 
file then there is no need for static prototypes at all.

> +static int
> +init_private_data (info, priv)
> +    struct disassemble_info *info;
> +    struct private *priv;

Again ISO C90 means that you can include the argument declarations in 
the parameter list, ie:

   static int
   init_private_data (struct disassemble_info *info, struct private* priv)
   {


Finally - it would be nice if you could come up with a new test for the 
GAS testsuite that checks this functionality.

Cheers
   Nick

Re: [PATCH-ping] VAX: Forced decoding of function entry masks (eg. for disassembling ROM images)

[Jan-Benedict Glaw]

On Thu, Mar 24, 2005 at 12:04:10PM +0000, Nick Clifton wrote:

Hi Nick!

> > I'd like to ping for my last patch, introducing the capability to
> > better disassemble ROM or MOP boot images with eg. objdump. Andreas'
> > objection about a limited number of entry addresses is fixed.
> 
> Sorry - it was in my queue to look at, but I am little behind at the moment.

No problem. I just didn't know about that:)

> > 2005-03-21  Jan-Benedict Glaw  <jbglaw@lug-owl.de>
> > 
> > 	opcodes/
> > 	* vax-dis.c: (struct private): Add a bfd_vma pointer to store
> > 	supplied function entry mask addresses.
> > 	(init_private_data): New; fill in supplied entry mask addresses.
> > 	(free_private_data): New; free() entry mask addresses.
> > 	(is_function_entry): Check if a given address is a function's
> > 	start address by looking at supplied entry mask addresses and
> > 	symbol information, if available.
> > 	(print_insn_vax): Use init_private_data(), is_function_entry()
> > 	and free_private_data().
> > 
> > 	binutils/doc/
> > 	* binutils.texi: Document new VAX disassembler-specific option
> > 	-M entry:0xfooba8.
> 
> Approved please apply, but...

I cannot do that on my own. AFAICT, I've only done all the FSF
paperwork, but that doesn't come along with write privilege (and
I'm not the VAX port maintainer).

> > +static int init_private_data
> > +  PARAMS ((struct disassemble_info *, struct private *));
> > +static void free_private_data PARAMS ((struct private *));
> > +static bfd_boolean is_function_entry
> > +  PARAMS ((struct disassemble_info *, bfd_vma addr));
> 
> We are using ISO C() now, so there is no need for the PARAMS macro.  In 
> fact if you place your function declarations carefully in the source 
> file then there is no need for static prototypes at all.

That's known. With *this* patch, I only wanted to introduce this
functionality. I'm about to prepare other patches to move the VAX bits
over to C90. That's on my list. (I also wrote that when the floating
point stuff was C90'fied by IIRC Alan yesterday or the day before...)

> > +static int
> > +init_private_data (info, priv)
> > +    struct disassemble_info *info;
> > +    struct private *priv;
> 
> Again ISO C90 means that you can include the argument declarations in 
> the parameter list, ie:

That'll come along with one of the next patches. I don't like fucking
up one file by mixing K&R C with C90, but I don't like messing up a
patch that introduces functionality /and/ formatting either. As I said,
C90'ification is on the list.

> Finally - it would be nice if you could come up with a new test for the 
> GAS testsuite that checks this functionality.

Testsuite is another thing. Basically, there's no for VAX, at least not
one that'll really do exhaustive tests. But this is on my list, too.

Near-time TODOs for me are:

	- This patch (which I mostly consider being done)
	- C90'fication
	- Testsuite updates to check correct code generation for _at
	  least_ all commonly used opcodes and all addressing modes.
	- Cut the long opcode table out of the vax.h header file and
	  place it into it's own vax-opc.c file as most other
	  architectures do it.
	- Maybe implement {en,de}coding of VAX vector opcodes

Longer term TODOs:
	- Make SMP VAXen to properly run with Linux
	- Find someone to work with me on GCC (throw out the CC0
	  stuff out of the .md file, really build a HEAD GCC for
	  NetBSD, introduce vax-dec-linux-gnu)
	- Fix glibc for vax-dec-linux-gnu

Long term TODO:
	- Make DEC^H^H^HCompaq^H^H^H^H^H^HHP to produce VAXen again:-)

MfG, JBG

-- 
AWEK microdata GmbH -- Am Wellbach 4 -- 33609 Bielefeld

Re: [PATCH-ping] VAX: Forced decoding of function entry masks (eg. for disassembling ROM images)

[Nick Clifton]

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

Hi Jan-Benedict,

> I cannot do that on my own. AFAICT, I've only done all the FSF
> paperwork, but that doesn't come along with write privilege (and
> I'm not the VAX port maintainer).

OK - well I was going through the process of applying your patch when I 
discovered several more, err not bugs, but features, which I thought 
needed to be addressed.  Specifically:

   * The new feature of the disassembler should be mentioned in the 
binutils NEWS file.

   * Parsing the disassembler options string every time print_insn_vax() 
is called seems to be hugely wasteful.  Surely it is better to parse it 
just once.

   * When free_private_data() is called it does not reset the 
entry_array and num_entry_fields to zero, so that the next time 
init_private_data is called it will try to realloc freed memory.

   * What is the purpose of the private structure anyway ?  It seems 
simpler to just a couple of static variables here.

Rather than just complain however, I have produced an alternative 
version of your patch which addresses most of these issues (attached). 
What do you think ?  (Also, if you do like it, please could you test it 
as I have now way of doing so myself).

Cheers
   Nick



[-- Attachment #2: vax.dis.patch --]
[-- Type: text/plain, Size: 7634 bytes --]

opcodes/
2005-03-21  Jan-Benedict Glaw  <jbglaw@lug-owl.de>

	* vax-dis.c: (entry_addr): New varible:  An array of user supplied
	function entry mask addresses.
	(num_entry_addr): New variable: The number of elements in entry_addr.
	(parse_disassembler_options): New function.  Fills in the entry_addr
	array.
	(is_function_entry): Check if a given address is a function's
	start address by looking at supplied entry mask addresses and
	symbol information, if available.
	(print_insn_vax): Use parse_disassembler_options and is_function_entry.

binutils
2005-03-21  Jan-Benedict Glaw  <jbglaw@lug-owl.de>

	* doc/binutils.texi: Document new VAX disassembler-specific option
	-M entry:0xfooba8.
	* NEWS: Mention the new option.

Index: opcodes/vax-dis.c
===================================================================
RCS file: /cvs/src/src/opcodes/vax-dis.c,v
retrieving revision 1.9
diff -c -3 -p -r1.9 vax-dis.c
*** opcodes/vax-dis.c	14 Mar 2005 09:35:26 -0000	1.9
--- opcodes/vax-dis.c	24 Mar 2005 14:45:57 -0000
***************
*** 17,22 ****
--- 17,24 ----
     along with this program; if not, write to the Free Software
     Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  */
  
+ #include <setjmp.h>
+ #include <string.h>
  #include "sysdep.h"
  #include "opcode/vax.h"
  #include "dis-asm.h"
*************** static char *entry_mask_bit[] =
*** 77,91 ****
  /* Maximum length of an instruction.  */
  #define MAXLEN 25
  
- #include <setjmp.h>
- 
  struct private
  {
    /* Points to first byte not fetched.  */
!   bfd_byte *max_fetched;
!   bfd_byte the_buffer[MAXLEN];
!   bfd_vma insn_start;
!   jmp_buf bailout;
  };
  
  /* Make sure that bytes from INFO->PRIVATE_DATA->BUFFER (inclusive)
--- 79,91 ----
  /* Maximum length of an instruction.  */
  #define MAXLEN 25
  
  struct private
  {
    /* Points to first byte not fetched.  */
!   bfd_byte * max_fetched;
!   bfd_byte   the_buffer[MAXLEN];
!   bfd_vma    insn_start;
!   jmp_buf    bailout;
  };
  
  /* Make sure that bytes from INFO->PRIVATE_DATA->BUFFER (inclusive)
*************** fetch_data (info, addr)
*** 119,124 ****
--- 119,202 ----
    return 1;
  }
  
+ /* Entry mask handling.  */
+ static unsigned int  num_entry_addr = 0;
+ static bfd_vma *     entry_addr = NULL;
+ 
+ /* Parse the VAX specific disassembler options.  These contain functions
+    entry addresses, which can be useful to disassemble ROM images, since
+    there's no symbol table.  Returns TRUE upon success, FALSE otherwise.  */
+ 
+ static bfd_boolean
+ parse_disassembler_options (char * options)
+ {
+   const char * entry_switch = "entry:";
+ 
+   while ((options = strstr (options, entry_switch)))
+     {
+       options += strlen (entry_switch);
+ 
+       /* FIXME: This is not a very efficient way of extending the table.  */
+       entry_addr = realloc (entry_addr, sizeof (bfd_vma)
+ 			    * (num_entry_addr + 1));
+       if (entry_addr == NULL)
+ 	return FALSE;
+       entry_addr[num_entry_addr ++] = bfd_scan_vma (options, NULL, 0);
+     }
+ 
+   return TRUE;
+ }
+ 
+ #if 0 /* FIXME:  Ideally the disassembler should have target specific
+ 	 initialisation and termination function pointers.  Then
+ 	 parse_disassembler_options could be the init function and
+ 	 free_entry_array (below) could be the termination routine.
+ 	 Until then there is no way for the disassembler to tell us
+ 	 that it has finished and that we no longer need the entry
+ 	 array, so this routine is suprpess for now.  It does mean
+ 	 that we leak memory, but only to the extent that we do not
+ 	 free it just before the disassembler is about to terminate
+ 	 anyway.  */
+ 
+ /* Free memory allocated to our entry array.  */
+ 
+ static void
+ free_entry_array (struct private *priv)
+ {
+   if (entry_addr)
+     {
+       free (entry_addr);
+       entry_addr = NULL;
+       num_entry_addr = 0;
+     }
+ }
+ #endif
+ /* Check if the given address is a known function entry. Either there must
+    be a symbol of function type at this address, or the address must be
+    a forced entry point.  The later helps in disassembling ROM images, because
+    there's no symbol table at all.  Forced entry points can be given by
+    supplying several -M options to objdump: -M entry:0xffbb7730.  */
+ 
+ static bfd_boolean
+ is_function_entry (struct disassemble_info *info, bfd_vma addr)
+ {
+   unsigned int i;
+ 
+   /* Check if there's a BSF_FUNCTION symbol at our address.  */
+   if (info->symbols
+       && info->symbols[0]
+       && (info->symbols[0]->flags & BSF_FUNCTION)
+       && addr == bfd_asymbol_value (info->symbols[0]))
+     return TRUE;
+ 
+   /* Check for forced function entry address.  */
+   for (i = 0; i < num_entry_addr; i++)
+     if (entry_addr[i] == addr)
+       return TRUE;
+ 
+   return FALSE;
+ }
+ 
  /* Print the vax instruction at address MEMADDR in debugged memory,
     on INFO->STREAM.  Returns length of the instruction, in bytes.  */
  
*************** print_insn_vax (memaddr, info)
*** 137,142 ****
--- 215,228 ----
    priv.max_fetched = priv.the_buffer;
    priv.insn_start = memaddr;
  
+   if (info->disassembler_options)
+     {
+       parse_disassembler_options (info->disassembler_options);
+ 
+       /* To avoid repeated parsing of these options, we remove them here.  */
+       info->disassembler_options = NULL;
+     }
+ 
    if (setjmp (priv.bailout) != 0)
      {
        /* Error return.  */
*************** print_insn_vax (memaddr, info)
*** 157,166 ****
      }
  
    /* Decode function entry mask.  */
!   if (info->symbols
!       && info->symbols[0]
!       && (info->symbols[0]->flags & BSF_FUNCTION)
!       && memaddr == bfd_asymbol_value (info->symbols[0]))
      {
        int i = 0;
        int register_mask = buffer[1] << 8 | buffer[0];
--- 243,249 ----
      }
  
    /* Decode function entry mask.  */
!   if (is_function_entry (info, memaddr))
      {
        int i = 0;
        int register_mask = buffer[1] << 8 | buffer[0];
Index: binutils/doc/binutils.texi
===================================================================
RCS file: /cvs/src/src/binutils/doc/binutils.texi,v
retrieving revision 1.69
diff -c -3 -p -r1.69 binutils.texi
*** binutils/doc/binutils.texi	15 Mar 2005 17:45:19 -0000	1.69
--- binutils/doc/binutils.texi	24 Mar 2005 14:45:58 -0000
*************** rather than names, for the selected type
*** 1793,1798 ****
--- 1793,1805 ----
  You can list the available values of @var{ABI} and @var{ARCH} using
  the @option{--help} option.
  
+ For VAX, you can specify function entry addresses with @option{-M
+ entry:0xf00ba}.  You can use this multiple times to properly
+ disassemble VAX binary files that don't contain symbol tables (like
+ ROM dumps).  In these cases, the function entry mask would otherwise
+ be decoded as VAX instructions, which would probably lead the the rest
+ of the function being wrongly disassembled.
+ 
  @item -p
  @itemx --private-headers
  Print information that is specific to the object file format.  The exact
Index: binutils/NEWS
===================================================================
RCS file: /cvs/src/src/binutils/NEWS,v
retrieving revision 1.49
diff -c -3 -p -r1.49 NEWS
*** binutils/NEWS	15 Mar 2005 17:45:18 -0000	1.49
--- binutils/NEWS	24 Mar 2005 14:45:58 -0000
***************
*** 1,5 ****
--- 1,8 ----
  -*- text -*-
  
+ * Add "-M entry:<addr>" switch to objdump to specify a function entry address
+   when disassembling VAX binaries.
+ 
  * Add "--globalize-symbol <name>" and "--globalize-symbols <filename>" switches
    to objcopy to convert local symbols into global symbols.
  

Re: [PATCH-ping] VAX: Forced decoding of function entry masks (eg. for disassembling ROM images)

[Jan-Benedict Glaw]

On Thu, Mar 24, 2005 at 03:11:40PM +0000, Nick Clifton wrote:

>    * The new feature of the disassembler should be mentioned in the 
> binutils NEWS file.

That's missing, right.

>    * Parsing the disassembler options string every time print_insn_vax() 
> is called seems to be hugely wasteful.  Surely it is better to parse it 
> just once.

But how shall it free the memory after the whole disassembly run is
finished? Of course, I'd leak the allocated memory (there are other
memory leaks, though), but I don't see an "easy" way to signal "all
work is done, you're finished" to the disassembly backend.

>    * When free_private_data() is called it does not reset the 
> entry_array and num_entry_fields to zero, so that the next time 
> init_private_data is called it will try to realloc freed memory.

init_private_data() explicitely sets these two fields to 0 / NULL
before it starts to parse the backend-specific string. Seems correct
to me 8)

>    * What is the purpose of the private structure anyway ?  It seems 
> simpler to just a couple of static variables here.

I didn't program this in the first run:)  Hey, I was about to be
born when the VAXen were first sold!  However, a lot of xxx-dis.c files
seem to have copied it at some time.

> *************** fetch_data (info, addr)
> *** 119,124 ****
> --- 119,202 ----
>     return 1;
>   }
>   
> + /* Entry mask handling.  */
> + static unsigned int  num_entry_addr = 0;
> + static bfd_vma *     entry_addr = NULL;
> + 
> + /* Parse the VAX specific disassembler options.  These contain functions
> +    entry addresses, which can be useful to disassemble ROM images, since
> +    there's no symbol table.  Returns TRUE upon success, FALSE otherwise.  */

s/functions/function/, but not sure...

> + static bfd_boolean
> + parse_disassembler_options (char * options)
> + {
> +   const char * entry_switch = "entry:";
> + 
> +   while ((options = strstr (options, entry_switch)))
> +     {
> +       options += strlen (entry_switch);
> + 
> +       /* FIXME: This is not a very efficient way of extending the table.  */
> +       entry_addr = realloc (entry_addr, sizeof (bfd_vma)
> + 			    * (num_entry_addr + 1));

Maybe an estimation like

	num_expected_entries = strlen (options)
			       / (strlen (entry_switch) + 5);

would be a start.

> +       if (entry_addr == NULL)
> + 	return FALSE;
> +       entry_addr[num_entry_addr ++] = bfd_scan_vma (options, NULL, 0);
> +     }
> + 
> +   return TRUE;
> + }
> + 
> + #if 0 /* FIXME:  Ideally the disassembler should have target specific
> + 	 initialisation and termination function pointers.  Then
> + 	 parse_disassembler_options could be the init function and
> + 	 free_entry_array (below) could be the termination routine.
> + 	 Until then there is no way for the disassembler to tell us
> + 	 that it has finished and that we no longer need the entry
> + 	 array, so this routine is suprpess for now.  It does mean

s/suprpess/suppressed/

> + 	 that we leak memory, but only to the extent that we do not
> + 	 free it just before the disassembler is about to terminate
> + 	 anyway.  */

...if libopcodes/libbfd is used by objdump.

> + /* Free memory allocated to our entry array.  */
> + 
> + static void
> + free_entry_array (struct private *priv)
> + {
> +   if (entry_addr)
> +     {
> +       free (entry_addr);
> +       entry_addr = NULL;
> +       num_entry_addr = 0;
> +     }
> + }

priv is unused here because the entry list was moved out of the
private structure at all.

> + #endif
> + /* Check if the given address is a known function entry. Either there must
> +    be a symbol of function type at this address, or the address must be
> +    a forced entry point.  The later helps in disassembling ROM images, because
> +    there's no symbol table at all.  Forced entry points can be given by
> +    supplying several -M options to objdump: -M entry:0xffbb7730.  */
> + 
> + static bfd_boolean
> + is_function_entry (struct disassemble_info *info, bfd_vma addr)
> + {
> +   unsigned int i;
> + 
> +   /* Check if there's a BSF_FUNCTION symbol at our address.  */
> +   if (info->symbols
> +       && info->symbols[0]
> +       && (info->symbols[0]->flags & BSF_FUNCTION)
> +       && addr == bfd_asymbol_value (info->symbols[0]))
> +     return TRUE;
> + 
> +   /* Check for forced function entry address.  */
> +   for (i = 0; i < num_entry_addr; i++)
> +     if (entry_addr[i] == addr)
> +       return TRUE;
> + 
> +   return FALSE;
> + }
> + 
>   /* Print the vax instruction at address MEMADDR in debugged memory,
>      on INFO->STREAM.  Returns length of the instruction, in bytes.  */
>   
> *************** print_insn_vax (memaddr, info)
> *** 137,142 ****
> --- 215,228 ----
>     priv.max_fetched = priv.the_buffer;
>     priv.insn_start = memaddr;
>   
> +   if (info->disassembler_options)
> +     {
> +       parse_disassembler_options (info->disassembler_options);
> + 
> +       /* To avoid repeated parsing of these options, we remove them here.  */
> +       info->disassembler_options = NULL;
> +     }
> + 

Hopefully, the caller didn't malloc()/strdup()/... memory for the string
supplied in info->disassembler_options ...

Objdump just uses concat() to create an argument, but there may be
other programs out there that actually use libbfd and libopcodes as
real libraries... At least, I don't know if one would expect that the
backend modifies this... (Custom disassembly programs using libbfd
and libopcodes really exist. A friend had written one for disassembling
PPC code; he's currently working on getting through MCA-PPCs boot-up
functions...)

>     if (setjmp (priv.bailout) != 0)
>       {
>         /* Error return.  */
> *************** print_insn_vax (memaddr, info)
> *** 157,166 ****
>       }
>   
>     /* Decode function entry mask.  */
> !   if (info->symbols
> !       && info->symbols[0]
> !       && (info->symbols[0]->flags & BSF_FUNCTION)
> !       && memaddr == bfd_asymbol_value (info->symbols[0]))
>       {
>         int i = 0;
>         int register_mask = buffer[1] << 8 | buffer[0];
> --- 243,249 ----
>       }
>   
>     /* Decode function entry mask.  */
> !   if (is_function_entry (info, memaddr))
>       {
>         int i = 0;
>         int register_mask = buffer[1] << 8 | buffer[0];
> Index: binutils/doc/binutils.texi
> ===================================================================
> RCS file: /cvs/src/src/binutils/doc/binutils.texi,v
> retrieving revision 1.69
> diff -c -3 -p -r1.69 binutils.texi
> *** binutils/doc/binutils.texi	15 Mar 2005 17:45:19 -0000	1.69
> --- binutils/doc/binutils.texi	24 Mar 2005 14:45:58 -0000
> *************** rather than names, for the selected type
> *** 1793,1798 ****
> --- 1793,1805 ----
>   You can list the available values of @var{ABI} and @var{ARCH} using
>   the @option{--help} option.
>   
> + For VAX, you can specify function entry addresses with @option{-M
> + entry:0xf00ba}.  You can use this multiple times to properly
> + disassemble VAX binary files that don't contain symbol tables (like
> + ROM dumps).  In these cases, the function entry mask would otherwise
> + be decoded as VAX instructions, which would probably lead the the rest
> + of the function being wrongly disassembled.
> + 

You took the first patch of the two; there's a typo in the above text,
an excessive "the". That was, however, the only change.


Basically, I of course like the idea of parsing the options only
once.  OTOH, this is one more location that's leaking. Maybe the
time is come to *really* implement an initializer and termination
function for the disassembler backends? But I'm not sure if it's
worth it. Disassembling used to be a slow process, and it's a rare
situation. Even on a VAX, I'd accept that it takes some time[tm]
to disassemble something.  ...and with today's PCs, the overhead
of parsing the options each time /one/ opcode is being
disassembled is irrelevant.

So the pros and cons of your implementation are:

	+ faster
	- leaks memory
	- modifies disassembler_info->disassembler_options

If somebody would implement a init/fini function for the backend, that
would turn out to only being faster with no drawbacks...

Being somewhat undecided, I'd say let's push in your version and start
a new thread about init/fini functions for the disassembler backend.
I'll push it through the compiler in the mean time.

MfG, JBG

-- 
AWEK microdata GmbH -- Am Wellbach 4 -- 33609 Bielefeld

Re: [PATCH-ping] VAX: Forced decoding of function entry masks (eg. for disassembling ROM images)

[Jan-Benedict Glaw]

On Thu, Mar 24, 2005 at 03:11:40PM +0000, Nick Clifton wrote:

> Rather than just complain however, I have produced an alternative 
> version of your patch which addresses most of these issues (attached). 
> What do you think ?  (Also, if you do like it, please could you test it 
> as I have now way of doing so myself).

Compiles cleanly. Please fix the typos (as notes in my previous mail)
and import it. I'll now probably start writing some more files for the
testsuits... Guess that's quite handy to have *before* starting to to
C90'fication or moving around the opcode table...

MfG, JBG

-- 
AWEK microdata GmbH -- Am Wellbach 4 -- 33609 Bielefeld

Re: [PATCH-ping] VAX: Forced decoding of function entry masks (eg. for disassembling ROM images)

[Nick Clifton]

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

Hi Jan-Benedict,

> Basically, I of course like the idea of parsing the options only
> once.  OTOH, this is one more location that's leaking. Maybe the
> time is come to *really* implement an initializer and termination
> function for the disassembler backends? But I'm not sure if it's
> worth it. Disassembling used to be a slow process, and it's a rare
> situation. Even on a VAX, I'd accept that it takes some time[tm]
> to disassemble something.  ...and with today's PCs, the overhead
> of parsing the options each time /one/ opcode is being
> disassembled is irrelevant.
> 
> So the pros and cons of your implementation are:
> 
> 	+ faster
> 	- leaks memory
> 	- modifies disassembler_info->disassembler_options
> 
> If somebody would implement a init/fini function for the backend, that
> would turn out to only being faster with no drawbacks...
> 
> Being somewhat undecided, I'd say let's push in your version and start
> a new thread about init/fini functions for the disassembler backend.
> I'll push it through the compiler in the mean time.

Ok - I have pushed my version, with a few fixes to address all of the 
issues you mentioned, except the memory leak.  (See the attached patch). 
  I think that this leak is OK for now, but we should not forget it. 
One day (soon I hope), I'll write the code to add the necessary hooks, 
so that the leak can be removed.

Cheers
   Nick



[-- Attachment #2: vax.dis.patch --]
[-- Type: text/plain, Size: 5939 bytes --]

Index: opcodes/vax-dis.c
===================================================================
RCS file: /cvs/src/src/opcodes/vax-dis.c,v
retrieving revision 1.9
diff -c -3 -p -r1.9 vax-dis.c
*** opcodes/vax-dis.c	14 Mar 2005 09:35:26 -0000	1.9
--- opcodes/vax-dis.c	29 Mar 2005 16:04:47 -0000
***************
*** 17,22 ****
--- 17,24 ----
     along with this program; if not, write to the Free Software
     Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  */
  
+ #include <setjmp.h>
+ #include <string.h>
  #include "sysdep.h"
  #include "opcode/vax.h"
  #include "dis-asm.h"
*************** static char *entry_mask_bit[] =
*** 77,91 ****
  /* Maximum length of an instruction.  */
  #define MAXLEN 25
  
- #include <setjmp.h>
- 
  struct private
  {
    /* Points to first byte not fetched.  */
!   bfd_byte *max_fetched;
!   bfd_byte the_buffer[MAXLEN];
!   bfd_vma insn_start;
!   jmp_buf bailout;
  };
  
  /* Make sure that bytes from INFO->PRIVATE_DATA->BUFFER (inclusive)
--- 79,91 ----
  /* Maximum length of an instruction.  */
  #define MAXLEN 25
  
  struct private
  {
    /* Points to first byte not fetched.  */
!   bfd_byte * max_fetched;
!   bfd_byte   the_buffer[MAXLEN];
!   bfd_vma    insn_start;
!   jmp_buf    bailout;
  };
  
  /* Make sure that bytes from INFO->PRIVATE_DATA->BUFFER (inclusive)
*************** fetch_data (info, addr)
*** 119,124 ****
--- 119,213 ----
    return 1;
  }
  
+ /* Entry mask handling.  */
+ static unsigned int  entry_addr_occupied_slots = 0;
+ static unsigned int  entry_addr_total_slots = 0;
+ static bfd_vma *     entry_addr = NULL;
+ 
+ /* Parse the VAX specific disassembler options.  These contain function
+    entry addresses, which can be useful to disassemble ROM images, since
+    there's no symbol table.  Returns TRUE upon success, FALSE otherwise.  */
+ 
+ static bfd_boolean
+ parse_disassembler_options (char * options)
+ {
+   const char * entry_switch = "entry:";
+ 
+   while ((options = strstr (options, entry_switch)))
+     {
+       options += strlen (entry_switch);
+ 
+       /* The greater-than part of the test below is paranoia.  */
+       if (entry_addr_occupied_slots >= entry_addr_total_slots)
+ 	{
+ 	  /* A guesstimate of the number of entries we will have to create.  */
+ 	  entry_addr_total_slots +=
+ 	    strlen (options) / (strlen (entry_switch) + 5);
+ 	  
+ 	  entry_addr = realloc (entry_addr, sizeof (bfd_vma)
+ 				* entry_addr_total_slots);
+ 	}
+ 
+       if (entry_addr == NULL)
+ 	return FALSE;
+ 	  
+       entry_addr[entry_addr_occupied_slots] = bfd_scan_vma (options, NULL, 0);
+       entry_addr_occupied_slots ++;
+     }
+ 
+   return TRUE;
+ }
+ 
+ #if 0 /* FIXME:  Ideally the disassembler should have target specific
+ 	 initialisation and termination function pointers.  Then
+ 	 parse_disassembler_options could be the init function and
+ 	 free_entry_array (below) could be the termination routine.
+ 	 Until then there is no way for the disassembler to tell us
+ 	 that it has finished and that we no longer need the entry
+ 	 array, so this routine is suppressed for now.  It does mean
+ 	 that we leak memory, but only to the extent that we do not
+ 	 free it just before the disassembler is about to terminate
+ 	 anyway.  */
+ 
+ /* Free memory allocated to our entry array.  */
+ 
+ static void
+ free_entry_array (void)
+ {
+   if (entry_addr)
+     {
+       free (entry_addr);
+       entry_addr = NULL;
+       entry_addr_occupied_slots = entry_addr_total_slots = 0;
+     }
+ }
+ #endif
+ /* Check if the given address is a known function entry. Either there must
+    be a symbol of function type at this address, or the address must be
+    a forced entry point.  The later helps in disassembling ROM images, because
+    there's no symbol table at all.  Forced entry points can be given by
+    supplying several -M options to objdump: -M entry:0xffbb7730.  */
+ 
+ static bfd_boolean
+ is_function_entry (struct disassemble_info *info, bfd_vma addr)
+ {
+   unsigned int i;
+ 
+   /* Check if there's a BSF_FUNCTION symbol at our address.  */
+   if (info->symbols
+       && info->symbols[0]
+       && (info->symbols[0]->flags & BSF_FUNCTION)
+       && addr == bfd_asymbol_value (info->symbols[0]))
+     return TRUE;
+ 
+   /* Check for forced function entry address.  */
+   for (i = entry_addr_occupied_slots; i--;)
+     if (entry_addr[i] == addr)
+       return TRUE;
+ 
+   return FALSE;
+ }
+ 
  /* Print the vax instruction at address MEMADDR in debugged memory,
     on INFO->STREAM.  Returns length of the instruction, in bytes.  */
  
*************** print_insn_vax (memaddr, info)
*** 127,132 ****
--- 216,222 ----
       bfd_vma memaddr;
       disassemble_info *info;
  {
+   static bfd_boolean parsed_disassembler_options = FALSE;
    const struct vot *votp;
    const char *argp;
    unsigned char *arg;
*************** print_insn_vax (memaddr, info)
*** 137,142 ****
--- 227,241 ----
    priv.max_fetched = priv.the_buffer;
    priv.insn_start = memaddr;
  
+   if (! parsed_disassembler_options
+       && info->disassembler_options != NULL)
+     {
+       parse_disassembler_options (info->disassembler_options);
+ 
+       /* To avoid repeated parsing of these options.  */
+       parsed_disassembler_options = TRUE;
+     }
+ 
    if (setjmp (priv.bailout) != 0)
      {
        /* Error return.  */
*************** print_insn_vax (memaddr, info)
*** 157,166 ****
      }
  
    /* Decode function entry mask.  */
!   if (info->symbols
!       && info->symbols[0]
!       && (info->symbols[0]->flags & BSF_FUNCTION)
!       && memaddr == bfd_asymbol_value (info->symbols[0]))
      {
        int i = 0;
        int register_mask = buffer[1] << 8 | buffer[0];
--- 256,262 ----
      }
  
    /* Decode function entry mask.  */
!   if (is_function_entry (info, memaddr))
      {
        int i = 0;
        int register_mask = buffer[1] << 8 | buffer[0];