Re: RFA: AArch64 sim

[Mike Frysinger <vapier at gentoo dot org>]

On 17 Jul 2015 15:10, Nicholas Clifton wrote:
> >> +++ include/gdb/sim-aarch64.h
> 
> >> +#ifdef __cplusplus
> >> +extern "C" { // }
> >> +#endif
> >
> > hmm, i see a few arches do this, but most don't.  is there any reason we should
> > keep this ?  or should we scrub all targets to not do this ?
> 
> It is your call.  I saw that other header files in this directory were 
> doing it, so I thought that it would be wise to follow their example. 
> The extra code does not hurt when compiling with C and I presume that it 
> is necessary when compiling with C++.  (I do not know this for sure 
> though - I hate C++).  I am happy to remove the code if you want however.

i've posted a patch to clean up the others.  you should trim it from this
one in the meantime.

> >> +typedef enum
> >> +{
> >> +  STATUS_READY   = 0, /* May continue stepping or running.  */
> >> +  STATUS_RETURN  = 1, /* Via normal return from initial frame.  */
> >> +  STATUS_HALT    = 2, /* Via HALT pseudo-instruction.  */
> >> +  STATUS_BREAK   = 3, /* Via BRK instruction.  */
> >> +  STATUS_CALLOUT = 4, /* Via CALLOUT pseudo-instruction.  */
> >> +  STATUS_ERROR   = 5, /* Simulator detected problem.  */
> >> +  STATUS_MAX     = 6
> >> +} StatusCode;
> >
> > a scan of the code indicates that most of this looks like you're setting state
> > and then acting on it later yourself when you really should be calling
> > sim_engine_halt directly.  any reason for doing it this way ?
> 
> Originally it was simply historical - this is the way the code was 
> written in the smallaarch64sim.  Now it is because it allows better 
> tracing and disassembler output, and cleaner code - the halt and error 
> returns are only handled in one place.

i'm not seeing how this is cleaner.  when you call sim_engine_halt, the
code stops at that point.  there is no returning/etc... afterwards.

plus, when i look at some of these funcs, they only ever return READY.
which leads to a lot of return code paths that are pointless.

+#define STORE_FUNC(TYPE, NAME, N)                                      \
+  StatusCode                                                           \
+  aarch64_set_mem_##NAME (sim_cpu *cpu, uint64_t address, TYPE value)  \
+  {                                                                    \
+    TRACE_MEMORY (cpu,                                                 \
+                 "write of %" PRIx64 " (%d bytes) to %" PRIx64,        \
+                 (uint64_t) value, N, address);                        \
+                                                                       \
+    sim_core_write_unaligned_##N (cpu, 0, write_map, address, value);  \
+    return STATUS_READY;                                               \
+  }
...
+static StatusCode
+stur32 (sim_cpu *cpu, int32_t offset)
+{
+  unsigned rn = uimm (cpu->instr, 9, 5);
+  unsigned rd = uimm (cpu->instr, 4, 0);
+
+  return aarch64_set_mem_u32 (cpu,
+                             aarch64_get_reg_u64 (cpu, rn, SP_OK) + offset,
+                             aarch64_get_reg_u32 (cpu, rd, NO_SP));
+}
...
+static StatusCode
+dexLoadUnscaledImmediate (sim_cpu *cpu)
... i scanned a bunch of these and they look like above ...
+       case 0:  return sturb (cpu, imm);
+       case 1:  return ldurb32 (cpu, imm);
+       case 2:  return ldursb64 (cpu, imm);
+       case 3:  return ldursb32 (cpu, imm);
+       case 4:  return sturh (cpu, imm);
+       case 5:  return ldurh32 (cpu, imm);
+       case 6:  return ldursh64 (cpu, imm);
+       case 7:  return ldursh32 (cpu, imm);
+       case 8:  return stur32 (cpu, imm);
+       case 9:  return ldur32 (cpu, imm);
+       case 10: return ldursw (cpu, imm);
+       case 12: return stur64 (cpu, imm);
+       case 13: return ldur64 (cpu, imm);
...

in this func, the only thing that doesn't return READY are:
+         return_NYI;
+         return_UNALLOC;

which look like:
+#define return_UNALLOC                                                 \
+  do                                                                   \
+    {                                                                  \
+      if (TRACE_INSN_P (cpu))                                          \
+       {                                                               \
+         aarch64_print_insn (CPU_STATE (cpu), aarch64_get_PC (cpu));   \
+         TRACE_INSN (cpu,                                              \
+                     "Unallocated instruction detected at sim line %d,"\
+                     " exe addr %" PRIx64,                             \
+                     __LINE__, aarch64_get_PC (cpu));                  \
+       }                                                               \
+      cpu->errorCode = ERROR_UNALLOC;                                  \
+      return STATUS_ERROR;                                             \
+    }                                                                  \
+  while (1)

so instead of returning, just call sim_engine_halt directly

in fact, i only see the errorCode field being set.  nowhere is it being
read.  you might think "wait but there's aarch64_get_ErrorCode and 
aarch64_get_error_text" ... except nothing calls those functions.

so i can't see how errorCode adds any value, just noise, nor does it have
any impact on debugging/tracing output.

> Is this version OK to apply ?

you should scan the files and trim trailing whitespace while you're at it.
sed -i -r 's:[[:space:]]+$::' *.[ch]
-mike

Attachment: signature.asc
Description: Digital signature