From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 64897 invoked by alias); 27 Nov 2019 16:54:37 -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 64889 invoked by uid 89); 27 Nov 2019 16:54:37 -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= 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:54:36 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id 6675E203AC; Wed, 27 Nov 2019 11:54:34 -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 831BB20362; Wed, 27 Nov 2019 11:54:30 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id 673C520AF6; Wed, 27 Nov 2019 11:54:30 -0500 (EST) X-Gerrit-PatchSet: 1 Date: Wed, 27 Nov 2019 16:54:00 -0000 From: "Simon Marchi (Code Review)" To: Luis Machado , gdb-patches@sourceware.org Cc: Andrew Burgess 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:54:29 -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: <20191127165430.673C520AF6@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-11/txt/msg01044.txt.bz2 Simon Marchi 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. Ah ok. I'm surprised that the linker didn't give a "multiple definitions" error. But from what I understand, reading about -fcommon, that's the point: it merges the uninitialized variable symbols from the different objects into one. If the variable had been initialized, I guess we would have had a "multiple definitions" error. So your change looks good to me. It's just a bit inconsistent to have the declaration in armemu.h and definition in arminit.c, but I see that other variables are the same (like ARMul_ImmedTable just above), so I don't think you should worry about it. | + | /***************************************************************************\ | * 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? I don't really know, I was just pointing out the fact that it's a bit dangerous to have some local extern declarations. I think having a local extern declaration is never the *right* solution, although it can help in a pinch. To find the right solution, we would need to better understand which file conceptually owns these variables and which file just borrows it. I find it surprising for example that a variable declared as: struct maverick_regs DSPregs[16]; would be needed when the file `maverick.c` is not included in the build. I don't have a clear picture of the situation (and don't really have the time to dig in), but it might be that the variable really belongs to maverick.c, and that wrapper.c should use it conditionally, like: #ifdef WITH_MAVERICK memcpy (& DSPregs [rn - SIM_ARM_MAVERIC_COP0R0_REGNUM], memory, sizeof (struct maverick_regs)); #endif To be clear, I would be totally fine with the fix you have right now, it helps fixing the build issue while not making the situation any worse than it is right now. | +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:54:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Luis Machado Comment-In-Reply-To: Simon Marchi Gerrit-MessageType: comment