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

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

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 <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:54:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Luis Machado <luis.machado@linaro.org>
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment