Re: [patch] add trace capabilities to arm sim

Re: [patch] add trace capabilities to arm sim

[Richard Earnshaw]

I haven't seen anyone else comment on this, so I'll stick my nose in ;-)

I can't approve or reject this patch, but:


> 2001-02-23  DJ Delorie  <dj@redhat.com>
> 
> 	* armemu.c (ARMul_Emulate): Add hook for tracing.
> 	* wrapper.c (current_bfd): New, remembers which bfd we're
> 	simulating.
> 	(sim_dis_read): New, helper for tracing.
> 	(remove_useless_symbols): New, helper for tracing.  From objdump.
> 	(compare_symbols): Ditto.
> 	(sim_trace_one_arm_insn): New, trace one instruction using
> 	disassembler.
> 	(sim_trace): Make it do something useful now.
> 	(sim_create_inferior): Save current bfd.
> 	(sim_open): Ditto.
> 	(sim_load): Ditto.
> 	(sim_do_command): Handle the `trace' command.
> 
> 	* wrapper.c (ARMul_Debug): Return insn just in case we're called.
> 

> --- armemu.c	2001/02/24 00:24:39
> *************** ARMul_Emulate26 (register ARMul_State * 
> *** 377,382 ****
> --- 377,383 ----
>         if (instr == 0)
>   	abort ();
>   #endif
> +       sim_trace_one_arm_insn (pc, instr);

This will bump up the cost of simulating each instruction by the overhead 
of this call plus any code within that always gets executed.

Since tracing is going to slow the simulation to a crawl, I think it would 
be better to consider this an "exceptional condition"; that is, put the 
code inside the immediately following test:

>   
>         if (state->Exception)
>   	{			/* Any exceptions */

and then bump state->Exception (which behaves pretty much like a semaphore 
count on the number of extra things to go look at) when tracing needs to 
run.

> + void
> + sim_trace_one_arm_insn(pc, insn)
> + 	      int pc;
> + 	      unsigned int insn;
> + {
> +   static int initted = 0;
> +   static asymbol **symtab = 0;
> +   static int symcount = 0;
> +   static int last_sym = -1;
> +   static struct disassemble_info info;
> +   int storage, sym, bestsym, bestaddr;
> +   int min, max, i, pnl;
> +   static ARMword prevregs[16];
> + 
> +   if (insn == 0)
> +     {
> +       state->Emulate = STOP;

why should executing the (legal) instruction

	andeq	r0, r0, r0

cause the simulation to halt (OK, it's not very useful, but it is a valid 
NOP)?

> +       return;
> +     }
> +   if (current_bfd == 0)
> +     return;
> +   if (!do_tracing)
> +     return;

Shouldn't this be the first thing you check?

>   int
>   sim_trace (sd)
>        SIM_DESC sd ATTRIBUTE_UNUSED;

It is used now... (this is also where you might bump/unbump state->
Exception).

> ! {
> !   do_tracing = 1;
> !   sim_resume(sd, 0, 0);
> !   do_tracing = 0;
>     return 1;
>   }
>   

You'd also need to do it here (but a bit more subtly...

>   sim_do_command (sd, cmd)
>        SIM_DESC sd ATTRIBUTE_UNUSED;
>        char *cmd ATTRIBUTE_UNUSED;
> ! {
> !   if (strcmp(cmd, "trace") == 0)
> !     {
> !       do_tracing = !do_tracing;
> !       if (do_tracing)
> ! 	(*sim_callback->printf_filtered) (sim_callback,
> ! 					  "Tracing on.\n");
> !       else
> ! 	(*sim_callback->printf_filtered) (sim_callback,
> ! 					  "Tracing off.\n");
> !     }
> !   else
> ! 	(*sim_callback->printf_filtered) (sim_callback,
> ! 					  "Unknown command, only `trace' allowed.\n");
>   }
>   
>   

R.

Re: [patch] add trace capabilities to arm sim

[DJ Delorie]

> > +       sim_trace_one_arm_insn (pc, instr);
> 
> This will bump up the cost of simulating each instruction by the overhead 
> of this call plus any code within that always gets executed.

OK, I hadn't looked at the state->Exception check as a generic "check things"
but that makes sense.


> > +   if (insn == 0)
> > +     {
> > +       state->Emulate = STOP;
> 
> why should executing the (legal) instruction
> 
> 	andeq	r0, r0, r0
> 
> cause the simulation to halt (OK, it's not very useful, but it is a valid 
> NOP)?

I took that from a commented out debug check in the emulator.  The
idea was to trap when the thread of execution left the code space and
either hit zeros in the data space, or left RAM completely.  I'll take
it out now that I know it's a valid opcode.

> > +   if (!do_tracing)
> > +     return;
> 
> Shouldn't this be the first thing you check?

Either way worked for me, but I can make it first.

Re: [patch] add trace capabilities to arm sim

[Richard Earnshaw]

dj@redhat.com said:
> > +   if (!do_tracing)
> > +     return;
> 
> > Shouldn't this be the first thing you check?
> Either way worked for me, but I can make it first.

I'd go one step further.  Move the check to the caller side.

R.

Re: [patch] add trace capabilities to arm sim

[DJ Delorie]

> I'd go one step further.  Move the check to the caller side.

(1) wouldn't the state->Exceptions check effectively do that?

(2) I'd have to make do_tracing global if I did that.

Re: [patch] add trace capabilities to arm sim

[Richard Earnshaw]

> 
> > I'd go one step further.  Move the check to the caller side.
> 
> (1) wouldn't the state->Exceptions check effectively do that?

Well, it would help, but it is also enabled for other things.

> 
> (2) I'd have to make do_tracing global if I did that.

That brings up another issue.  What if there is more than one ARM model in 
a simulation?  Shouldn't the variable be made part of the "State" anyway?

Re: [patch] add trace capabilities to arm sim

[Andrew Cagney]

The ARM simulator doesn't use sim/common so this is strictly a BTW.

sim/common simulators address performance concerns such as function call
overhead using two techniques:

	o	inline

	o	compile time
		enabling of features

code would we written as:

	do_trace_stuff (....)

and then, when the compiler supported it, ``do_trace_stuff'' might or
might not be inlined.


The second bit of trivia relates to Frank's comment.  Performance
analysis of simulators has shown that some stuff, like tracing, should
be left as separate functions.  The likely reason was that inlining the
code blew out the host CPU cache.

For what it is worth.

	Andrew

Re: [patch] add trace capabilities to arm sim

[Richard Earnshaw]

> 
> Richard Earnshaw <rearnsha@arm.com> writes:
> 
> : I haven't seen anyone else comment on this, so I'll stick my nose in ;-)
> : I can't approve or reject this patch, but:
> 
> Thanks - your input is always appreciated.
> 
> 
> : [...]
> : >         if (state->Exception)
> : >   	{			/* Any exceptions */
> : 
> : and then bump state->Exception (which behaves pretty much like a semaphore 
> : count on the number of extra things to go look at) when tracing needs to 
> : run.
> 
> An interesting technique!  Have you tried using gcc's __builtin_expect
> function to provide branch (un)likelihood hints to the compiler,
> instead of this method?

Well, it would have a marginal benefit on those hosts that support branch 
prediction, but you would still need to do each check.  As long as we 
don't get to the point where state->Exception is more often true than 
false, it is still a benefit to wrap them all up into a single pre-check.

R.