This is the mail archive of the
binutils@sources.redhat.com
mailing list for the binutils project.
Re: [PATCH-ping] VAX: Forced decoding of function entry masks (eg. for disassembling ROM images)
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