This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On 15 Jul 2015 17:58, Nick Clifton wrote: it's looking pretty good. comments inline. > +++ include/gdb/sim-aarch64.h > > +/* sim-aarch64.h --- interface between AArch64 simulator and GDB. only one space before "simulator" > +#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 ? > +++ sim/aarch64/cpustate.c > > +#define reg_num(reg) ((reg == R31 && !r31_is_sp) ? 32 : reg) prob doesn't really matter, but it should be (reg) in both places > + if (val != REG (sd)->gr[reg].u64) > + TRACE_REGISTER (STATE_CPU (sd, 0), ideally you'd pass in the cpu instead of hardcoding CPU 0 everywhere via sd. you can get sd from the cpu via CPU_STATE(). > +/* Set the CPSR register as an int. */ > +StatusCode > +aarch64_set_CPSR (SIM_DESC sd, uint32_t new_flags) > +{ > ... > + } should double check trailing whitespace > +++ sim/aarch64/cpustate.h > > +#if __WORDSIZE == 64 > +#define PRINT_64 "l" > +#else > +#define PRINT_64 "ll" > +#endif you want inttypes.h and PRIi64/etc... and then delete PRINT_64 entirely > +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 ? > +#endif /* ifndef DECODE_H */ usually our comments don't include "ifndef" and such, just the macro name (which is _DECODE_H here i think) > +++ sim/aarch64/interp.c > > +static char opbuf[1000]; should you have a sanity check on this to make sure it doesn't overflow ? > +static int > +op_printf (char *buf, char *fmt, ...) should have a printf attribute on it > +static int > +sim_dis_read (bfd_vma memaddr, > + bfd_byte * ptr, > + unsigned int length, > + struct disassemble_info * info) > +{ > + aarch64_get_mem_blk (NULL, memaddr, (char *) ptr, length); > + return 0; > +} shouldn't > +compare_symbols (const void * ap, const void * bp) no space after the *. this comes up a few times in this patch. > +sim_read (SIM_DESC sd, SIM_ADDR mem, unsigned char * buf, int length) > +sim_write (SIM_DESC sd, SIM_ADDR mem, const unsigned char * buf, int length) hmm, do you even need these anymore ? now that you're using the sim core for memory handling, the common ones should just work. > +++ sim/aarch64/Makefile.in > > +INCLUDE = cpustate.h memory.h decode.h pretty sure this isn't needed so you can delete it > +interp.o: cpustate.h memory.h simulator.h sim-main.h > +cpustate.o: cpustate.h memory.h simulator.h sim-main.h > +simulator.o: cpustate.h memory.h simulator.h sim-main.h > +main.o: cpustate.h memory.h simulator.h sim-main.h > +memory.o: cpustate.h memory.h simulator.h sim-main.h you should be able to delete all these rules as they'll get automatically generated now > +++ sim/aarch64/memory.c > > +static bfd * saved_prog = NULL; this whole handling of saved_prog is so tortured. it looks like you should change aarch64_init to accept the bfd directly from the sim func. > +static inline void > +StatusCode > +aarch64_get_mem_blk (SIM_DESC sd, uint64_t address, char * buffer, unsigned length) > +{ > + char * addr = sim_core_trans_addr (sd, STATE_CPU (sd, 0), 0, address); pretty sure map should be set to read_map instead of 0 > + > + if (addr == NULL) > + { > + memset (buffer, 0, length); > + if (sd) > + { > + mem_error (sd, "read of non-existant mem block at", address); > + return aarch64_set_status (sd, STATUS_ERROR, ERROR_EXCEPTION); > + } > + return STATUS_ERROR; > + } > + > + memcpy (buffer, addr, length); > + return STATUS_READY; > +} seems like this whole func should be replaced with sim_core_read_buffer. > +/* Accessor macro to obtain the aarch64 sim registers. */ > +#define REG(sd) STATE_CPU ((sd), 0) would really really prefer the cpu get passed down into funcs instead of hardcoding 0 > --- /dev/null > +++ sim/aarch64/simulator.c > > + return aarch64_set_reg_u64 (sd, rd, NO_SP, aarch64_get_mem_u32 (sd, aarch64_get_PC (sd) + offset * 4)); there's a few lines in this file that need wrapping largely scanned the rest as i'm not familiar with the aarch64 isa and don't have time to learn atm ;) -mike
Attachment:
signature.asc
Description: Digital signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |