Re: RFA: AArch64 sim

[Nicholas Clifton <nickc@redhat.com>]

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

Hi Mike,

   Thanks for the further review.  I am attaching a third version of the 
patch with all of the issues you raised fixed, except for two...

>> +++ 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.


>> +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.

Is this version OK to apply ?

Cheers
   Nick


[-- Attachment #2: aarch64.sim.patch.3.xz --]
[-- Type: application/x-xz, Size: 51300 bytes --]