From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 98749 invoked by alias); 10 Nov 2015 07:32:50 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 98735 invoked by uid 89); 10 Nov 2015 07:32:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: smtp.gentoo.org Received: from smtp.gentoo.org (HELO smtp.gentoo.org) (140.211.166.183) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 10 Nov 2015 07:32:47 +0000 Received: from vapier.lan (localhost [127.0.0.1]) by smtp.gentoo.org (Postfix) with SMTP id B714C33E01A; Tue, 10 Nov 2015 07:32:45 +0000 (UTC) Date: Tue, 10 Nov 2015 07:32:00 -0000 From: Mike Frysinger To: Nicholas Clifton Cc: gdb-patches@sourceware.org Subject: Re: RFA: AArch64 sim Message-ID: <20151110073245.GN5154@vapier.lan> Mail-Followup-To: Nicholas Clifton , gdb-patches@sourceware.org References: <87vbe8hvfo.fsf@redhat.com> <87y4ih8ij4.fsf@redhat.com> <20150716151902.GE5641@vapier> <55A90CBA.40007@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Iovxle6z3WGVZjcj" Content-Disposition: inline In-Reply-To: <55A90CBA.40007@redhat.com> X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg00245.txt.bz2 --Iovxle6z3WGVZjcj Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 5533 On 17 Jul 2015 15:10, Nicholas Clifton wrote: > >> +++ include/gdb/sim-aarch64.h >=20 > >> +#ifdef __cplusplus > >> +extern "C" { // } > >> +#endif > > > > hmm, i see a few arches do this, but most don't. is there any reason w= e should > > keep this ? or should we scrub all targets to not do this ? >=20 > It is your call. I saw that other header files in this directory were=20 > doing it, so I thought that it would be wise to follow their example.=20 > The extra code does not hurt when compiling with C and I presume that it= =20 > is necessary when compiling with C++. (I do not know this for sure=20 > 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 =3D 0, /* May continue stepping or running. */ > >> + STATUS_RETURN =3D 1, /* Via normal return from initial frame. */ > >> + STATUS_HALT =3D 2, /* Via HALT pseudo-instruction. */ > >> + STATUS_BREAK =3D 3, /* Via BRK instruction. */ > >> + STATUS_CALLOUT =3D 4, /* Via CALLOUT pseudo-instruction. */ > >> + STATUS_ERROR =3D 5, /* Simulator detected problem. */ > >> + STATUS_MAX =3D 6 > >> +} StatusCode; > > > > a scan of the code indicates that most of this looks like you're settin= g 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 ? >=20 > Originally it was simply historical - this is the way the code was=20 > written in the smallaarch64sim. Now it is because it allows better=20 > tracing and disassembler output, and cleaner code - the halt and error=20 > 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 =3D uimm (cpu->instr, 9, 5); + unsigned rd =3D 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 =3D 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=20 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 --Iovxle6z3WGVZjcj Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature Content-length: 819 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJWQZ2dAAoJEEFjO5/oN/WBtycP/RHJpG/nkK81BkJxAodwUb+1 AFLW5gG4DtdMXyok7CYJ2PiE5I/PuS1h/YAkO8UbXBoM6TCNCux9R8jBNp8FRiYm Xk9O8fWyQg8oFO3w+G5ze3Huqj5m5OXC+1AQWm1HURpjvKbnpS47Bj/du3p7XPu3 D3udx3XYVRyABOaNBULM98PPBZ2Z5HaNKPo/GSatdndY1EkSUOqJoyBe0FAIrIyN EnHu84c2p2/g3lPB0E4USgrR+v8fip/b6gOFnLXTQ3od8eCt+kRRwirf7fIqDGg9 AnJatPyY7/vYRtcc+en7gCw1xyjvN7n1lUHlm3sczt8lJvw9i7HWY74CL/Hr3h/U dDNz8EXdPmqgx2ciBr821ruwA6WTTeYzSly8kqCO6lgezi+gcanhAdWU+a/5N28u tCPUBEa/cBZy9/cy92smSLM726Q0+lIEd0+v+6MeNLyBmsHMTyu2LqDWKmVl5rIy k7XdOdvpZOCHfjNgwdMc00KT5YunwZcUdXozDkhOCe9X88c6N7XsMy2V1eB4GWyx qUo28BIoxAuZur/jpirFjcA89tKrujzhA+3Wwm5yi63PvlGTc30xRwZpucYMf1AQ ZStPN+wk99L5Yb26MNHG3F13PXTlSWp2oKk660oDKzPXr49rJO8CgE9njEu0WDhI z4EUwG0po6BQ03YXKEKT =T7yA -----END PGP SIGNATURE----- --Iovxle6z3WGVZjcj--