From patchwork Thu Jun 30 01:33:11 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jim Wilson X-Patchwork-Id: 71214 Delivered-To: patch@linaro.org Received: by 10.140.28.4 with SMTP id 4csp145772qgy; Wed, 29 Jun 2016 18:33:33 -0700 (PDT) X-Received: by 10.98.54.198 with SMTP id d189mr17077678pfa.39.1467250413053; Wed, 29 Jun 2016 18:33:33 -0700 (PDT) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id d22si1415262pfj.151.2016.06.29.18.33.32 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 29 Jun 2016 18:33:33 -0700 (PDT) Received-SPF: pass (google.com: domain of gdb-patches-return-132384-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@sourceware.org; spf=pass (google.com: domain of gdb-patches-return-132384-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gdb-patches-return-132384-patch=linaro.org@sourceware.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; q=dns; s=default; b=pGn/ +MimAFzb2HSmdSlB5SUc6k0+Lp3OiGBh125Ou9tu0DBvyqxtxQkeS2OJm8JSB6py 8SSs2JjqoelNNKV+1k7xc5cD+F7iovhCUAAGEgcK57jTniWcz5cMf/qq26wrVmub qOc+eKCeEzfgPJmAA0iwN+ojf5FoDWN2P3Wp89o= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; s=default; bh=wxiufOhkAw aBseK4qEKkZcMkcL8=; b=G1hKU6YvMvpUbObKN2hucS+KJL3vo/2AU/Bd2Og4G7 SA+59iT6ITTYd+cwRwkiTb/CND7GrbrjrSTIDQaTOS4hG9mNxVBytOkFBYOToWHD vfpD6Kfofdr+0PEw8icV9R34sULUktS4Pw/ZKpRu+GE09tMo+SmTmhH6p6c2feXR w= Received: (qmail 106216 invoked by alias); 30 Jun 2016 01:33:25 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 105700 invoked by uid 89); 30 Jun 2016 01:33:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=IA-64, IA64, ia-64, ia64 X-HELO: mail-qk0-f174.google.com Received: from mail-qk0-f174.google.com (HELO mail-qk0-f174.google.com) (209.85.220.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 30 Jun 2016 01:33:14 +0000 Received: by mail-qk0-f174.google.com with SMTP id a125so120524868qkc.2 for ; Wed, 29 Jun 2016 18:33:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=jTSUtYvNW4Bdwq5dBD+WXEShDbC/ZOHKiYezJcJcQfQ=; b=dX2fpzLtmjKxWhKRQdfvzetHZBsWx4bG6lPbNwj1MQygTDZTmblFXHbQgCC3seuH4Y mj90fhio9sJ2o5jXvdd0728+kl6qAZZQctLT2us+eIKayn5r5WSzibZ1tUTTTCfNwaeI hd7EPWvtYLXlM74OZww1vhwqpIjvl4jB6c04gomaYh6vCBHwMWwbTj04H9OJJ1goB1Gh mLOPhXYMJYQII/XEpSDuOlPoccVU+MLspuqxVMUQ3lFAJ049IsHET/5IN3FmFXqpE/Kv SQEaYtDKuXz96/ISSJoXLWUwHRVx6uNbzFLdOnAnGiSux6ozNsGLW/kXWeo+wpCFbBbF MadQ== X-Gm-Message-State: ALyK8tJ6myDyPfWHZIbZ118QQ9JV7WQiDyQ3c9bHudiut0AjmAAhhgwC/uT0KqafaAnxXwKI/tqt94pNyjqbCQqa X-Received: by 10.13.241.199 with SMTP id a190mr5319845ywf.47.1467250392125; Wed, 29 Jun 2016 18:33:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.129.9.213 with HTTP; Wed, 29 Jun 2016 18:33:11 -0700 (PDT) In-Reply-To: References: From: Jim Wilson Date: Wed, 29 Jun 2016 18:33:11 -0700 Message-ID: Subject: Re: [PATCH] aarch64 sim big-endian support To: Nick Clifton Cc: gdb-patches@sourceware.org On Mon, Jun 13, 2016 at 5:38 AM, Nick Clifton wrote: > I think that I agree with this comment, although I could not find > the raw opcode reading functions to which he was referring, (unless > he meant sim_core_read_buffer), so would you mind trying out this > variation of your patch to see if it works instead ? I finally got back to this. I don't see any raw read function other than sim_core_read_buffer either. A raw read is not quite what I want, as I need a little-endian to host translation, but I can call endian_le2h_4 to do the swap after the raw read. The interface is a little awkward, as sim_core_read_buffer stores into a buffer instead of returning a pointer, so I need to store the instruction, and then read it back out again, swap it, and store it back again. An alternative solution might be to make a copy of sim-n-core.h, call it sim-n-core-le.h, and then change all of the T2H_M/H2T_M calls into LE2H_M/H2LE_M calls, along with a few other minor changes to complete the conversion. We can then call sim_core_read_le_aligned_N instead of sim_core_read_aligned_N for the instruction loads. Note that big-endian aarch64 is not the only target with this problem. big-endian ARMv7-A works the same way, and if we had an IA-64 simulator, it would work the same way too. So there are other potential users of these functions. This is maybe a little overkill though for now, as we don't need the unaligned and misaligned read functions for aarch64/armv7-a/ia-64 instruction loads, and we don't need the write functions either. We only need the aligned read functions. I tried testing this for all four combinations of big/little endian host/target with a hello world program, and discovered that the big-endian host support is broken. The problem is with the GRegisterValue untion. You have typedef union GRegisterValue { int8_t s8; ... int64_t s64; } GRegister; On a little-endian host, the s8 member will match the low-byte of the s64 member, which is what we want. However, on a big-endian host, the s8 member will match the high-byte of the u64 member, and the simulator fails. I can fix this by using an anonymous struct for the big-endian case typedef union GRegisterValue { struct { int64_t :56; int8_t s8; }; ...l sint64_t s64; } GRegister; There are other ways to fix this, but this just seemed to me like the quickest and smallest patch that would make it work. There may also be other issues here, as I only tested an integer hello world program. Fixing the problem this way means that we require either an ISO C 2011 compiler, or a compiler that supports GCC extensions to ISO C 1990 or 1999. Otherwise, you may get an error for the anonymous structs. Or alternatively, it requires using a C++ compiler, as C++ added anonymous structs long before C did. I'm not sure how much of a problem this will be. If this is a serious problem, it could be fixed by giving names to the structs, adding the structs to the little endian side also with the field order switched, and then fixing all users to use the new names for the fields. That will be a bigger patch. With both changes, a hello world program works on all four combinations of big/little host/target. if you aren't happy with the cpustate.h change, it would be nice to get an approval for just the simulator.c change, as that is the part I care more about. We can worry about how to fix the big-endian host cpustate.h support later. Jim 2016-06-29 Jim Wilson sim/aarch64/ * cpustate.h: Include config.h. (union GRegisterValue): Add WORDS_BIGENDIAN check. For big endian code use anonymous structs to align members. * simulator.c (aarch64_step): Use sim_core_read_buffer and endian_le2h_4 to read instruction from pc. diff --git a/sim/aarch64/cpustate.h b/sim/aarch64/cpustate.h index 07446a2..2754f7c 100644 --- a/sim/aarch64/cpustate.h +++ b/sim/aarch64/cpustate.h @@ -22,6 +22,7 @@ #ifndef _CPU_STATE_H #define _CPU_STATE_H +#include "config.h" #include #include #include @@ -133,6 +134,7 @@ typedef enum VReg an explicit extend. */ typedef union GRegisterValue { +#if !WORDS_BIGENDIAN int8_t s8; int16_t s16; int32_t s32; @@ -141,6 +143,16 @@ typedef union GRegisterValue uint16_t u16; uint32_t u32; uint64_t u64; +#else + struct { int64_t :56; int8_t s8; }; + struct { int64_t :48; int16_t s16; }; + struct { int64_t :32; int32_t s32; }; + int64_t s64; + struct { uint64_t :56; uint8_t u8; }; + struct { uint64_t :48; uint16_t u16; }; + struct { uint64_t :32; uint32_t u32; }; + uint64_t u64; +#endif } GRegister; /* Float registers provide for storage of a single, double or quad diff --git a/sim/aarch64/simulator.c b/sim/aarch64/simulator.c index 88cb03d..8eb582a 100644 --- a/sim/aarch64/simulator.c +++ b/sim/aarch64/simulator.c @@ -14083,7 +14083,11 @@ aarch64_step (sim_cpu *cpu) return FALSE; aarch64_set_next_PC (cpu, pc + 4); - aarch64_get_instr (cpu) = aarch64_get_mem_u32 (cpu, pc); + + /* Code is always little-endian. */ + sim_core_read_buffer (CPU_STATE (cpu), cpu, read_map, + &aarch64_get_instr (cpu), pc, 4); + aarch64_get_instr (cpu) = endian_le2h_4 (aarch64_get_instr (cpu)); TRACE_INSN (cpu, " pc = %" PRIx64 " instr = %08x", pc, aarch64_get_instr (cpu));