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 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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |