From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38783 invoked by alias); 27 Nov 2019 16:20:44 -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 38772 invoked by uid 89); 27 Nov 2019 16:20:44 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=Though, solves X-HELO: mx1.osci.io Received: from polly.osci.io (HELO mx1.osci.io) (8.43.85.229) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 27 Nov 2019 16:20:41 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id ACFFC203C1; Wed, 27 Nov 2019 11:20:39 -0500 (EST) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [IPv6:2620:52:3:1:5054:ff:fe06:16ca]) by mx1.osci.io (Postfix) with ESMTP id 4F2DD2018B; Wed, 27 Nov 2019 11:20:37 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id 4DD7020AF6; Wed, 27 Nov 2019 11:20:36 -0500 (EST) X-Gerrit-PatchSet: 1 Date: Wed, 27 Nov 2019 16:20:00 -0000 From: "Luis Machado (Code Review)" To: gdb-patches@sourceware.org Cc: Andrew Burgess , Simon Marchi Auto-Submitted: auto-generated X-Gerrit-MessageType: comment Subject: [review] [ARM, sim] Fix build error and warnings X-Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f X-Gerrit-Change-Number: 726 X-Gerrit-ChangeURL: X-Gerrit-Commit: 2650f4cf5fdcdad48033763425070cfdcec43e7a In-Reply-To: References: X-Gerrit-Comment-Date: Wed, 27 Nov 2019 11:20:35 -0500 Reply-To: gnutoolchain-gerrit@osci.io MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-79-g83ff7f88f1 Content-Type: text/plain; charset=UTF-8 Message-Id: <20191127162036.4DD7020AF6@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-11/txt/msg01043.txt.bz2 Luis Machado has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726 ...................................................................... Patch Set 1: (2 comments) | --- sim/arm/arminit.c | +++ sim/arm/arminit.c | @@ -36,15 +36,19 @@ void ARMul_Abort (ARMul_State * state, ARMword address); | unsigned ARMul_MultTable[32] = | { 1, 2, 2, 3, 3, 4, 4, 5, 5, 6, 6, 7, 7, 8, 8, 9, 9, | 10, 10, 11, 11, 12, 12, 13, 13, 14, 14, 15, 15, 16, 16, 16 | }; | ARMword ARMul_ImmedTable[4096]; /* immediate DP LHS values */ | char ARMul_BitList[256]; /* number of bits in a byte table */ | | +/* The PC pipeline value depends on whether ARM | + or Thumb instructions are being executed. */ | +ARMword isize; PS1, Line 45: I should've given some more background, i apologize. The existing code looks sane, but the way it is built makes it not okay for -fno-common. >From armemu.c we build armemu26.o and armemu32.o, and those get linked together into libsim.a. Since both armemu26.o and armemu32.o came out of the same file, we get multiple definitions of isize. Moving such a definition to arminit.c (that generates arminit.o) solves the issue. I think one could argue that there are multiple ways to solve this. | + | /***************************************************************************\ | * Call this routine once to set up the emulator's tables. * | \***************************************************************************/ | | void | ARMul_EmulateInit (void) | { | unsigned long i, j; | --- sim/arm/wrapper.c | +++ sim/arm/wrapper.c | @@ -126,16 +126,16 @@ }; | | union maverick_acc_regs | { | long double ld; /* Acc registers are 72-bits. */ | }; | | -struct maverick_regs DSPregs[16]; | -union maverick_acc_regs DSPacc[4]; | -ARMword DSPsc; | +extern struct maverick_regs DSPregs[16]; PS1, Line 132: It is all a bit confusing. For example, based on the comment from sim/arm/maverick.c: /* We can't define these in here because this file might not be linked unless the target is arm9e-*. They are defined in wrapper.c. Eventually the simulator should be made to handle any coprocessor at run time. */ But in fact we're defining those in maverick.c and in wrapper.c. Though when both maverick.o and wrapper.o are included in the final linking, we get multiple definitions of these: struct maverick_regs DSPregs[16]; union maverick_acc_regs DSPacc[4]; ARMword DSPsc; I could put the definitions in a header, but the comment seems to indicate we shouldn't do that by default. Maybe the right solution here is to make the maverick.c definitions extern and the wrapper.c ones non-extern? | +extern union maverick_acc_regs DSPacc[4]; | +extern ARMword DSPsc; | | static void | init (void) | { | static int done; | | if (!done) -- Gerrit-Project: binutils-gdb Gerrit-Branch: master Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f Gerrit-Change-Number: 726 Gerrit-PatchSet: 1 Gerrit-Owner: Luis Machado Gerrit-Reviewer: Andrew Burgess Gerrit-Reviewer: Luis Machado Gerrit-CC: Simon Marchi Gerrit-Comment-Date: Wed, 27 Nov 2019 16:20:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Simon Marchi Gerrit-MessageType: comment