[review] [ARM, sim] Fix build error and warnings

["Luis Machado (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

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 <luis.machado@linaro.org>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 27 Nov 2019 16:20:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment