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

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

[Luis Machado (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................

[ARM, sim] Fix build error and warnings

Newer GCC's have switched to -fno-common by default, and this breaks the build
for the ARM sim, like this:

binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:65: multiple definition of `DSPsc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:134: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:64: multiple definition of `DSPacc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:133: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:63: multiple definition of `DSPregs'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:132: first defined here

I also noticed a few warnings due to mismatching types, as follows:

binutils-gdb/sim/arm/wrapper.c:870:31: warning: passing argument 1 of ‘sim_target_parse_arg_array’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  sim_target_parse_arg_array (argv);

binutils-gdb/sim/arm/wrapper.c:775:1: note: expected ‘char **’ but argument is of type ‘char * const*’
  sim_target_parse_arg_array (char ** argv)

The following patch fixes both of the above.

sim/arm/ChangeLog:

2019-11-26  Luis Machado  <luis.machado@linaro.org>

	* armemu.c (isize): Move this declaration ...
	* arminit.c (isize): ... here.
	* wrapper.c (DSPregs): Make extern.
	(DSPacc): Likewise.
	(DSPsc): Likewise.
	(sim_create_inferior): Cast variables to proper type.

Signed-off-by: Luis Machado <luis.machado@linaro.org>
Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
---
M sim/arm/armemu.c
M sim/arm/arminit.c
M sim/arm/wrapper.c
3 files changed, 9 insertions(+), 9 deletions(-)



diff --git a/sim/arm/armemu.c b/sim/arm/armemu.c
index 76f398b..3a72277 100644
--- a/sim/arm/armemu.c
+++ b/sim/arm/armemu.c
@@ -1140,10 +1140,6 @@
 
 /* EMULATION of ARM6.  */
 
-/* The PC pipeline value depends on whether ARM
-   or Thumb instructions are being executed.  */
-ARMword isize;
-
 ARMword
 #ifdef MODE32
 ARMul_Emulate32 (ARMul_State * state)
diff --git a/sim/arm/arminit.c b/sim/arm/arminit.c
index 851d356..3a626c8 100644
--- a/sim/arm/arminit.c
+++ b/sim/arm/arminit.c
@@ -40,6 +40,10 @@
 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;
+
 /***************************************************************************\
 *         Call this routine once to set up the emulator's tables.           *
 \***************************************************************************/
diff --git a/sim/arm/wrapper.c b/sim/arm/wrapper.c
index fde5d8c..9f86e08 100644
--- a/sim/arm/wrapper.c
+++ b/sim/arm/wrapper.c
@@ -129,9 +129,9 @@
   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];
+extern union maverick_acc_regs  DSPacc[4];
+extern ARMword DSPsc;
 
 static void
 init (void)
@@ -236,7 +236,7 @@
 {
   int argvlen = 0;
   int mach;
-  char **arg;
+  char * const *arg;
 
   init ();
 
@@ -867,7 +867,7 @@
 
   sim_callback = cb;
 
-  sim_target_parse_arg_array (argv);
+  sim_target_parse_arg_array ((char **) argv);
 
   if (argv[1] != NULL)
     {

-- 
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-MessageType: newchange

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

[Simon Marchi (Code Review)]

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:

How does moving this here help?

There is a declaration of isize in armemu.h, and isize was defined in
armemu.c, so I don't immediately see what's wrong with the existing
code.

| +
|  /***************************************************************************\
|  *         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:

Would it be possible to place these declarations in a header file
shared between all files that use these variables?  Otherwise, there's
always the risk that they get out of sync.

| +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-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 27 Nov 2019 15:36:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

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

[Luis Machado (Code Review)]

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

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

[Simon Marchi (Code Review)]

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

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

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................


Patch Set 1:

To be clear, I can't approve this patch, Andrew is the sim maintainer.


-- 
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:55:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

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

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................


Patch Set 1:

(1 comment)

| --- 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:

Yeah. I'm not too familiar with this code, but it seems a bit messy.

I think this is an artifact of old changes. The comment makes it sound
like we may not include maverick, but Makefile.in includes it
unconditionally.

In this case, i think having the definition in maverick.c and having
it external in wrapper.c makes more sense.

| +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 18:20:08 +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

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

[Andrew Burgess (Code Review)]

Andrew Burgess has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

Sorry for the delay in reviewing.

| --- 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:

I think that it's probably worth trying to clean up this area of code
a little.  Could you add a new header maverick.h and move the extern
declarations as well as the type definitions for 'struct
maverick_regs' and 'union maverick_acc_regs' into the new header.
Delete the duplicate type definitions from maverick.c and then just
include the header into wrapper.c and maverick.c.

This might not be the "best" fix - maybe there's already a header they
could/should go into, but I don't think having a new header will hurt,
and it will be much better than what we have now.

| +extern union maverick_acc_regs  DSPacc[4];
| +extern ARMword DSPsc;
|  
|  static void
|  init (void)
|  {
|    static int done;
|  
|    if (!done)

 ...

| @@ -231,18 +231,18 @@ sim_resume (SIM_DESC sd ATTRIBUTE_UNUSED,
|  SIM_RC
|  sim_create_inferior (SIM_DESC sd ATTRIBUTE_UNUSED,
|  		     struct bfd * abfd,
|  		     char * const *argv,
|  		     char * const *env)
|  {
|    int argvlen = 0;
|    int mach;
| -  char **arg;
| +  char * const *arg;

PS1, Line 239:

I couldn't figure out why this was needed.  I'm not really a fan of
casting away const as you now have to do below, so I'd like to
understand more about the motivation behind this change.

|  
|    init ();
|  
|    if (abfd != NULL)
|      {
|        ARMul_SetPC (state, bfd_get_start_address (abfd));
|        mach = bfd_get_mach (abfd);
|      }
|    else

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

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

[Luis Machado (Code Review)]

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/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:

Honestly it seems this code needs quite a bit of cleanup. There are
multiple coprocessors sharing the same code in a less-than-ideal way.

So your suggestion of cleanup is to have a maverick.h file created
with some more common/duplicate definitions in it?

wrapper.c would still need to have these extern variables:

extern struct maverick_regs     DSPregs[16];
extern union maverick_acc_regs  DSPacc[4];
extern ARMword DSPsc;

And maverick.c would need to have the proper declarations:

struct maverick_regs     DSPregs[16];
union maverick_acc_regs  DSPacc[4];
ARMword DSPsc;

Is this what you have in mind?

| +extern union maverick_acc_regs  DSPacc[4];
| +extern ARMword DSPsc;
|  
|  static void
|  init (void)
|  {
|    static int done;
|  
|    if (!done)

 ...

| @@ -231,18 +231,18 @@ sim_resume (SIM_DESC sd ATTRIBUTE_UNUSED,
|  SIM_RC
|  sim_create_inferior (SIM_DESC sd ATTRIBUTE_UNUSED,
|  		     struct bfd * abfd,
|  		     char * const *argv,
|  		     char * const *env)
|  {
|    int argvlen = 0;
|    int mach;
| -  char **arg;
| +  char * const *arg;

PS1, Line 239:

This fixes one of the 3 -Wdiscarded-qualifiers warnings showing up
when building ARM sim, hence this particular fix. It was the same as
the one in the patch description.

Though not a build failure, i didn't feel like leaving the warning in
there.

|  
|    init ();
|  
|    if (abfd != NULL)
|      {
|        ARMul_SetPC (state, bfd_get_start_address (abfd));
|        mach = bfd_get_mach (abfd);
|      }
|    else

-- 
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: Thu, 28 Nov 2019 12:38:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Luis Machado <luis.machado@linaro.org>
Comment-In-Reply-To: Andrew Burgess <andrew.burgess@embecosm.com>
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment

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

[Luis Machado (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................

[ARM, sim] Fix build error and warnings

Newer GCC's have switched to -fno-common by default, and this breaks the build
for the ARM sim, like this:

binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:65: multiple definition of `DSPsc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:134: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:64: multiple definition of `DSPacc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:133: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:63: multiple definition of `DSPregs'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:132: first defined here

I also noticed a few warnings due to mismatching types, as follows:

binutils-gdb/sim/arm/wrapper.c:870:31: warning: passing argument 1 of ‘sim_target_parse_arg_array’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  sim_target_parse_arg_array (argv);

binutils-gdb/sim/arm/wrapper.c:775:1: note: expected ‘char **’ but argument is of type ‘char * const*’
  sim_target_parse_arg_array (char ** argv)

The following patch fixes both of the above.

sim/arm/ChangeLog:

2019-11-28  Luis Machado  <luis.machado@linaro.org>

	* armemu.c (isize): Move this declaration ...
	* arminit.c (isize): ... here.
	* wrapper.c: Include "maverick.h".
	(<struct maverick_regs>, <union maverick_acc_regs>): Remove.
	(DSPregs): Make extern.
	(DSPacc): Likewise.
	(DSPsc): Likewise.
	(sim_create_inferior): Cast variables to proper type.
	(sim_open): Likewise.
	* maverick.c: Include "maverick.h".
	(<struct maverick_regs>, <union maverick_acc_regs>): Move
	declarations to maverick.h and update comment.
	* maverick.h: New file.

Signed-off-by: Luis Machado <luis.machado@linaro.org>
Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
---
M sim/arm/armemu.c
M sim/arm/arminit.c
M sim/arm/maverick.c
A sim/arm/maverick.h
M sim/arm/wrapper.c
5 files changed, 56 insertions(+), 67 deletions(-)



diff --git a/sim/arm/armemu.c b/sim/arm/armemu.c
index 76f398b..3a72277 100644
--- a/sim/arm/armemu.c
+++ b/sim/arm/armemu.c
@@ -1140,10 +1140,6 @@
 
 /* EMULATION of ARM6.  */
 
-/* The PC pipeline value depends on whether ARM
-   or Thumb instructions are being executed.  */
-ARMword isize;
-
 ARMword
 #ifdef MODE32
 ARMul_Emulate32 (ARMul_State * state)
diff --git a/sim/arm/arminit.c b/sim/arm/arminit.c
index 851d356..3a626c8 100644
--- a/sim/arm/arminit.c
+++ b/sim/arm/arminit.c
@@ -40,6 +40,10 @@
 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;
+
 /***************************************************************************\
 *         Call this routine once to set up the emulator's tables.           *
 \***************************************************************************/
diff --git a/sim/arm/maverick.c b/sim/arm/maverick.c
index c112692..cbcf55b 100644
--- a/sim/arm/maverick.c
+++ b/sim/arm/maverick.c
@@ -19,6 +19,7 @@
 #include "armdefs.h"
 #include "ansidecl.h"
 #include "armemu.h"
+#include "maverick.h"
 
 /*#define CIRRUS_DEBUG 1	*/
 #if CIRRUS_DEBUG
@@ -30,36 +31,9 @@
 #define POS64(i) ( (~(i)) >> 63 )
 #define NEG64(i) ( (i) >> 63 )
 
-/* Define Co-Processor instruction handlers here.  */
-
-/* Here's ARMulator's DSP definition.  A few things to note:
-   1) it has 16 64-bit registers and 4 72-bit accumulators
-   2) you can only access its registers with MCR and MRC.  */
-
-/* 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.  */
-struct maverick_regs
-{
-  union
-  {
-    int i;
-    float f;
-  } upper;
-
-  union
-  {
-    int i;
-    float f;
-  } lower;
-};
-
-union maverick_acc_regs
-{
-  long double ld;		/* Acc registers are 72-bits.  */
-};
-
+/* These variables are defined here and made extern in wrapper.c for now.
+   Eventually the simulator should be made to handle any coprocessor at run
+   time.  */
 struct maverick_regs DSPregs[16];
 union maverick_acc_regs DSPacc[4];
 ARMword DSPsc;
diff --git a/sim/arm/maverick.h b/sim/arm/maverick.h
new file mode 100644
index 0000000..9e081a4
--- /dev/null
+++ b/sim/arm/maverick.h
@@ -0,0 +1,42 @@
+/*  maverick.h -- Cirrus/DSP co-processor interface header
+    Copyright (C) 2003-2019 Free Software Foundation, Inc.
+    Contributed by Aldy Hernandez (aldyh@redhat.com).
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 3 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>. */
+
+/* Define Co-Processor instruction handlers here.  */
+
+/* Here's ARMulator's DSP definition.  A few things to note:
+   1) it has 16 64-bit registers and 4 72-bit accumulators
+   2) you can only access its registers with MCR and MRC.  */
+
+struct maverick_regs
+{
+  union
+  {
+    int i;
+    float f;
+  } upper;
+
+  union
+  {
+    int i;
+    float f;
+  } lower;
+};
+
+union maverick_acc_regs
+{
+  long double ld;		/* Acc registers are 72-bits.  */
+};
diff --git a/sim/arm/wrapper.c b/sim/arm/wrapper.c
index fde5d8c..5c9eb9b9 100644
--- a/sim/arm/wrapper.c
+++ b/sim/arm/wrapper.c
@@ -37,6 +37,7 @@
 #include "gdb/signals.h"
 #include "libiberty.h"
 #include "iwmmxt.h"
+#include "maverick.h"
 
 /* TODO: This should get pulled from the SIM_DESC.  */
 host_callback *sim_callback;
@@ -101,37 +102,9 @@
   fprintf (stderr, " %*s\n", size, opbuf);
 }
 
-/* Cirrus DSP registers.
-
-   We need to define these registers outside of maverick.c because
-   maverick.c might not be linked in unless --target=arm9e-* in which
-   case wrapper.c will not compile because it tries to access Cirrus
-   registers.  This should all go away once we get the Cirrus and ARM
-   Coprocessor to coexist in armcopro.c-- aldyh.  */
-
-struct maverick_regs
-{
-  union
-  {
-    int i;
-    float f;
-  } upper;
-
-  union
-  {
-    int i;
-    float f;
-  } lower;
-};
-
-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];
+extern union maverick_acc_regs  DSPacc[4];
+extern ARMword DSPsc;
 
 static void
 init (void)
@@ -236,7 +209,7 @@
 {
   int argvlen = 0;
   int mach;
-  char **arg;
+  char * const *arg;
 
   init ();
 
@@ -867,7 +840,7 @@
 
   sim_callback = cb;
 
-  sim_target_parse_arg_array (argv);
+  sim_target_parse_arg_array ((char **) argv);
 
   if (argv[1] != NULL)
     {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 2
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-MessageType: newpatchset

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

[Luis Machado (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................

[ARM, sim] Fix build error and warnings

Newer GCC's have switched to -fno-common by default, and this breaks the build
for the ARM sim, like this:

binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:65: multiple definition of `DSPsc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:134: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:64: multiple definition of `DSPacc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:133: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:63: multiple definition of `DSPregs'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:132: first defined here

I also noticed a few warnings due to mismatching types, as follows:

binutils-gdb/sim/arm/wrapper.c:870:31: warning: passing argument 1 of ‘sim_target_parse_arg_array’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  sim_target_parse_arg_array (argv);

binutils-gdb/sim/arm/wrapper.c:775:1: note: expected ‘char **’ but argument is of type ‘char * const*’
  sim_target_parse_arg_array (char ** argv)

The following patch fixes both of the above.

sim/arm/ChangeLog:

2019-11-28  Luis Machado  <luis.machado@linaro.org>

	* armemu.c (isize): Move this declaration ...
	* arminit.c (isize): ... here.
	* wrapper.c: Include "maverick.h".
	(<struct maverick_regs>, <union maverick_acc_regs>): Remove and update
	comment.
	(DSPregs): Make extern.
	(DSPacc): Likewise.
	(DSPsc): Likewise.
	(sim_create_inferior): Cast variables to proper type.
	(sim_open): Likewise.
	* maverick.c: Include "maverick.h".
	(<struct maverick_regs>, <union maverick_acc_regs>): Move
	declarations to maverick.h and update comment.
	* maverick.h: New file.

Signed-off-by: Luis Machado <luis.machado@linaro.org>
Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
---
M sim/arm/armemu.c
M sim/arm/arminit.c
M sim/arm/maverick.c
A sim/arm/maverick.h
M sim/arm/wrapper.c
5 files changed, 58 insertions(+), 67 deletions(-)



diff --git a/sim/arm/armemu.c b/sim/arm/armemu.c
index 76f398b..3a72277 100644
--- a/sim/arm/armemu.c
+++ b/sim/arm/armemu.c
@@ -1140,10 +1140,6 @@
 
 /* EMULATION of ARM6.  */
 
-/* The PC pipeline value depends on whether ARM
-   or Thumb instructions are being executed.  */
-ARMword isize;
-
 ARMword
 #ifdef MODE32
 ARMul_Emulate32 (ARMul_State * state)
diff --git a/sim/arm/arminit.c b/sim/arm/arminit.c
index 851d356..3a626c8 100644
--- a/sim/arm/arminit.c
+++ b/sim/arm/arminit.c
@@ -40,6 +40,10 @@
 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;
+
 /***************************************************************************\
 *         Call this routine once to set up the emulator's tables.           *
 \***************************************************************************/
diff --git a/sim/arm/maverick.c b/sim/arm/maverick.c
index c112692..cbcf55b 100644
--- a/sim/arm/maverick.c
+++ b/sim/arm/maverick.c
@@ -19,6 +19,7 @@
 #include "armdefs.h"
 #include "ansidecl.h"
 #include "armemu.h"
+#include "maverick.h"
 
 /*#define CIRRUS_DEBUG 1	*/
 #if CIRRUS_DEBUG
@@ -30,36 +31,9 @@
 #define POS64(i) ( (~(i)) >> 63 )
 #define NEG64(i) ( (i) >> 63 )
 
-/* Define Co-Processor instruction handlers here.  */
-
-/* Here's ARMulator's DSP definition.  A few things to note:
-   1) it has 16 64-bit registers and 4 72-bit accumulators
-   2) you can only access its registers with MCR and MRC.  */
-
-/* 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.  */
-struct maverick_regs
-{
-  union
-  {
-    int i;
-    float f;
-  } upper;
-
-  union
-  {
-    int i;
-    float f;
-  } lower;
-};
-
-union maverick_acc_regs
-{
-  long double ld;		/* Acc registers are 72-bits.  */
-};
-
+/* These variables are defined here and made extern in wrapper.c for now.
+   Eventually the simulator should be made to handle any coprocessor at run
+   time.  */
 struct maverick_regs DSPregs[16];
 union maverick_acc_regs DSPacc[4];
 ARMword DSPsc;
diff --git a/sim/arm/maverick.h b/sim/arm/maverick.h
new file mode 100644
index 0000000..9e081a4
--- /dev/null
+++ b/sim/arm/maverick.h
@@ -0,0 +1,42 @@
+/*  maverick.h -- Cirrus/DSP co-processor interface header
+    Copyright (C) 2003-2019 Free Software Foundation, Inc.
+    Contributed by Aldy Hernandez (aldyh@redhat.com).
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 3 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>. */
+
+/* Define Co-Processor instruction handlers here.  */
+
+/* Here's ARMulator's DSP definition.  A few things to note:
+   1) it has 16 64-bit registers and 4 72-bit accumulators
+   2) you can only access its registers with MCR and MRC.  */
+
+struct maverick_regs
+{
+  union
+  {
+    int i;
+    float f;
+  } upper;
+
+  union
+  {
+    int i;
+    float f;
+  } lower;
+};
+
+union maverick_acc_regs
+{
+  long double ld;		/* Acc registers are 72-bits.  */
+};
diff --git a/sim/arm/wrapper.c b/sim/arm/wrapper.c
index fde5d8c..5b7bd04 100644
--- a/sim/arm/wrapper.c
+++ b/sim/arm/wrapper.c
@@ -37,6 +37,7 @@
 #include "gdb/signals.h"
 #include "libiberty.h"
 #include "iwmmxt.h"
+#include "maverick.h"
 
 /* TODO: This should get pulled from the SIM_DESC.  */
 host_callback *sim_callback;
@@ -101,37 +102,11 @@
   fprintf (stderr, " %*s\n", size, opbuf);
 }
 
-/* Cirrus DSP registers.
-
-   We need to define these registers outside of maverick.c because
-   maverick.c might not be linked in unless --target=arm9e-* in which
-   case wrapper.c will not compile because it tries to access Cirrus
-   registers.  This should all go away once we get the Cirrus and ARM
-   Coprocessor to coexist in armcopro.c-- aldyh.  */
-
-struct maverick_regs
-{
-  union
-  {
-    int i;
-    float f;
-  } upper;
-
-  union
-  {
-    int i;
-    float f;
-  } lower;
-};
-
-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;
+/* These are defined in maverick.c for now.  Once coprocessor
+   handling is unified, they can go away.  */
+extern struct maverick_regs     DSPregs[16];
+extern union maverick_acc_regs  DSPacc[4];
+extern ARMword DSPsc;
 
 static void
 init (void)
@@ -236,7 +211,7 @@
 {
   int argvlen = 0;
   int mach;
-  char **arg;
+  char * const *arg;
 
   init ();
 
@@ -867,7 +842,7 @@
 
   sim_callback = cb;
 
-  sim_target_parse_arg_array (argv);
+  sim_target_parse_arg_array ((char **) argv);
 
   if (argv[1] != NULL)
     {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 3
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-MessageType: newpatchset

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

[Andrew Burgess (Code Review)]

Andrew Burgess has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................


Patch Set 3:

(3 comments)

| --- 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:

No, I imagined moving the declaration into the header file, so they
are declared once and defined once.  Yes I agree that this code is a
mess.  If you can figure out a better header file to place this into
that's fine, but just creating a new one seemed like the least effort,
but leaves things so we don't have duplicate type definitions and
variable declarations.

| +extern union maverick_acc_regs  DSPacc[4];
| +extern ARMword DSPsc;
|  
|  static void
|  init (void)
|  {
|    static int done;
|  
|    if (!done)

 ...

| @@ -231,18 +231,18 @@ sim_resume (SIM_DESC sd ATTRIBUTE_UNUSED,
|  SIM_RC
|  sim_create_inferior (SIM_DESC sd ATTRIBUTE_UNUSED,
|  		     struct bfd * abfd,
|  		     char * const *argv,
|  		     char * const *env)
|  {
|    int argvlen = 0;
|    int mach;
| -  char **arg;
| +  char * const *arg;

PS1, Line 239:

You're absolutely right, I missread the diff, my problem isn't with
this at all, it's with the next hunk...

|  
|    init ();
|  
|    if (abfd != NULL)
|      {
|        ARMul_SetPC (state, bfd_get_start_address (abfd));
|        mach = bfd_get_mach (abfd);
|      }
|    else

 ...

| @@ -862,18 +862,18 @@ sim_open (SIM_OPEN_KIND kind,
|        CPU_REG_FETCH (cpu) = arm_reg_fetch;
|        CPU_REG_STORE (cpu) = arm_reg_store;
|        CPU_PC_FETCH (cpu) = arm_pc_get;
|        CPU_PC_STORE (cpu) = arm_pc_set;
|      }
|  
|    sim_callback = cb;
|  
| -  sim_target_parse_arg_array (argv);
| +  sim_target_parse_arg_array ((char **) argv);

PS1, Line 870:

So this one I don't like.  I wondered if we couldn't simply push the
const-ness into the functions being called, but then we end up in
sim_target_parse_command_line where the ARGV array is modified.

What I really don't understand right now is why the ARGV array needs
to be modified in sim_target_parse_command_line at all - once we
return from sim_open I don't think that the ARGV array is used any
further.  And reading the comments in gdb/remote-sim.c relating to the
use of ARGVs, I suspect that modifying this would be a bad thing.

I think we need to understand more about how the argv array is being
used in order to solve this issue properly.

|  
|    if (argv[1] != NULL)
|      {
|        int i;
|  
|        /* Scan for memory-size switches.  */
|        for (i = 0; (argv[i] != NULL) && (argv[i][0] != 0); i++)
|  	if (argv[i][0] == '-' && argv[i][1] == 'm')
|  	  {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 3
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: Mon, 02 Dec 2019 22:16:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Luis Machado <luis.machado@linaro.org>
Comment-In-Reply-To: Andrew Burgess <andrew.burgess@embecosm.com>
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment

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

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................


Patch Set 3:

(3 comments)

| --- 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:

Ok. I think i understand now.

| +extern union maverick_acc_regs  DSPacc[4];
| +extern ARMword DSPsc;
|  
|  static void
|  init (void)
|  {
|    static int done;
|  
|    if (!done)

 ...

| @@ -231,18 +231,18 @@ sim_resume (SIM_DESC sd ATTRIBUTE_UNUSED,
|  SIM_RC
|  sim_create_inferior (SIM_DESC sd ATTRIBUTE_UNUSED,
|  		     struct bfd * abfd,
|  		     char * const *argv,
|  		     char * const *env)
|  {
|    int argvlen = 0;
|    int mach;
| -  char **arg;
| +  char * const *arg;

PS1, Line 239:

Done

|  
|    init ();
|  
|    if (abfd != NULL)
|      {
|        ARMul_SetPC (state, bfd_get_start_address (abfd));
|        mach = bfd_get_mach (abfd);
|      }
|    else

 ...

| @@ -862,18 +862,18 @@ sim_open (SIM_OPEN_KIND kind,
|        CPU_REG_FETCH (cpu) = arm_reg_fetch;
|        CPU_REG_STORE (cpu) = arm_reg_store;
|        CPU_PC_FETCH (cpu) = arm_pc_get;
|        CPU_PC_STORE (cpu) = arm_pc_set;
|      }
|  
|    sim_callback = cb;
|  
| -  sim_target_parse_arg_array (argv);
| +  sim_target_parse_arg_array ((char **) argv);

PS1, Line 870:

The code lacks more documentation to make it clear what the intent
was. As is, the code is already doing something it shouldn't, based on
the types passed.

I'm inclined to handle this in a separate patch given the main problem
is a build error.

|  
|    if (argv[1] != NULL)
|      {
|        int i;
|  
|        /* Scan for memory-size switches.  */
|        for (i = 0; (argv[i] != NULL) && (argv[i][0] != 0); i++)
|  	if (argv[i][0] == '-' && argv[i][1] == 'm')
|  	  {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 3
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: Tue, 03 Dec 2019 13:49:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Luis Machado <luis.machado@linaro.org>
Comment-In-Reply-To: Andrew Burgess <andrew.burgess@embecosm.com>
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment

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

[Luis Machado (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................

[ARM, sim] Fix build error and warnings

Newer GCC's have switched to -fno-common by default, and this breaks the build
for the ARM sim, like this:

binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:65: multiple definition of `DSPsc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:134: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:64: multiple definition of `DSPacc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:133: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:63: multiple definition of `DSPregs'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:132: first defined here

I also noticed a few warnings due to mismatching types, as follows:

../../../../repos/binutils-gdb/sim/arm/wrapper.c: In function ‘sim_create_inferior’:
../../../../repos/binutils-gdb/sim/arm/wrapper.c:335:16: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
       for (arg = argv; *arg != NULL; arg++)
                ^
../../../../repos/binutils-gdb/sim/arm/wrapper.c:342:8: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    arg = argv;
        ^
../../../../repos/binutils-gdb/sim/arm/wrapper.c:345:13: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    for (arg = argv; *arg != NULL; arg++)
             ^
The following patch fixes both of the above.

sim/arm/ChangeLog:

2019-12-03  Luis Machado  <luis.machado@linaro.org>

	* armemu.c (isize): Move this declaration ...
	* arminit.c (isize): ... here.
	* maverick.h: New file.
	* wrapper.c: Include "maverick.h".
	(<struct maverick_regs>, <union maverick_acc_regs>): Remove and update
	comment.
	(sim_create_inferior): Cast variables to proper type.
	* maverick.c: Include "maverick.h".
	(<struct maverick_regs>, <union maverick_acc_regs>): Move
	declarations to maverick.h and update comment.
	(DSPsc, DSPacc, DSPregs): Adjust comment.

Signed-off-by: Luis Machado <luis.machado@linaro.org>
Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
---
M sim/arm/armemu.c
M sim/arm/arminit.c
M sim/arm/maverick.c
A sim/arm/maverick.h
M sim/arm/wrapper.c
5 files changed, 57 insertions(+), 67 deletions(-)



diff --git a/sim/arm/armemu.c b/sim/arm/armemu.c
index 76f398b..3a72277 100644
--- a/sim/arm/armemu.c
+++ b/sim/arm/armemu.c
@@ -1140,10 +1140,6 @@
 
 /* EMULATION of ARM6.  */
 
-/* The PC pipeline value depends on whether ARM
-   or Thumb instructions are being executed.  */
-ARMword isize;
-
 ARMword
 #ifdef MODE32
 ARMul_Emulate32 (ARMul_State * state)
diff --git a/sim/arm/arminit.c b/sim/arm/arminit.c
index 851d356..3a626c8 100644
--- a/sim/arm/arminit.c
+++ b/sim/arm/arminit.c
@@ -40,6 +40,10 @@
 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;
+
 /***************************************************************************\
 *         Call this routine once to set up the emulator's tables.           *
 \***************************************************************************/
diff --git a/sim/arm/maverick.c b/sim/arm/maverick.c
index c112692..bae8c47 100644
--- a/sim/arm/maverick.c
+++ b/sim/arm/maverick.c
@@ -19,6 +19,7 @@
 #include "armdefs.h"
 #include "ansidecl.h"
 #include "armemu.h"
+#include "maverick.h"
 
 /*#define CIRRUS_DEBUG 1	*/
 #if CIRRUS_DEBUG
@@ -30,36 +31,10 @@
 #define POS64(i) ( (~(i)) >> 63 )
 #define NEG64(i) ( (i) >> 63 )
 
-/* Define Co-Processor instruction handlers here.  */
-
-/* Here's ARMulator's DSP definition.  A few things to note:
-   1) it has 16 64-bit registers and 4 72-bit accumulators
-   2) you can only access its registers with MCR and MRC.  */
-
-/* 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.  */
-struct maverick_regs
-{
-  union
-  {
-    int i;
-    float f;
-  } upper;
-
-  union
-  {
-    int i;
-    float f;
-  } lower;
-};
-
-union maverick_acc_regs
-{
-  long double ld;		/* Acc registers are 72-bits.  */
-};
-
+/* These variables are defined here and made extern in maverick.h for use
+   in wrapper.c for now.
+   Eventually the simulator should be made to handle any coprocessor at run
+   time.  */
 struct maverick_regs DSPregs[16];
 union maverick_acc_regs DSPacc[4];
 ARMword DSPsc;
diff --git a/sim/arm/maverick.h b/sim/arm/maverick.h
new file mode 100644
index 0000000..2549d21
--- /dev/null
+++ b/sim/arm/maverick.h
@@ -0,0 +1,46 @@
+/*  maverick.h -- Cirrus/DSP co-processor interface header
+    Copyright (C) 2003-2019 Free Software Foundation, Inc.
+    Contributed by Aldy Hernandez (aldyh@redhat.com).
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 3 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>. */
+
+/* Define Co-Processor instruction handlers here.  */
+
+/* Here's ARMulator's DSP definition.  A few things to note:
+   1) it has 16 64-bit registers and 4 72-bit accumulators
+   2) you can only access its registers with MCR and MRC.  */
+
+struct maverick_regs
+{
+  union
+  {
+    int i;
+    float f;
+  } upper;
+
+  union
+  {
+    int i;
+    float f;
+  } lower;
+};
+
+union maverick_acc_regs
+{
+  long double ld;		/* Acc registers are 72-bits.  */
+};
+
+extern struct maverick_regs DSPregs[16];
+extern union maverick_acc_regs DSPacc[4];
+extern ARMword DSPsc;
diff --git a/sim/arm/wrapper.c b/sim/arm/wrapper.c
index fde5d8c..78a9192 100644
--- a/sim/arm/wrapper.c
+++ b/sim/arm/wrapper.c
@@ -37,6 +37,7 @@
 #include "gdb/signals.h"
 #include "libiberty.h"
 #include "iwmmxt.h"
+#include "maverick.h"
 
 /* TODO: This should get pulled from the SIM_DESC.  */
 host_callback *sim_callback;
@@ -101,38 +102,6 @@
   fprintf (stderr, " %*s\n", size, opbuf);
 }
 
-/* Cirrus DSP registers.
-
-   We need to define these registers outside of maverick.c because
-   maverick.c might not be linked in unless --target=arm9e-* in which
-   case wrapper.c will not compile because it tries to access Cirrus
-   registers.  This should all go away once we get the Cirrus and ARM
-   Coprocessor to coexist in armcopro.c-- aldyh.  */
-
-struct maverick_regs
-{
-  union
-  {
-    int i;
-    float f;
-  } upper;
-
-  union
-  {
-    int i;
-    float f;
-  } lower;
-};
-
-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;
-
 static void
 init (void)
 {
@@ -236,7 +205,7 @@
 {
   int argvlen = 0;
   int mach;
-  char **arg;
+  char * const *arg;
 
   init ();
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 4
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-MessageType: newpatchset

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

[Andrew Burgess (Code Review)]

Andrew Burgess has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................


Patch Set 4: Code-Review+2

LGTM.

Thanks for working on this.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 4
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: Fri, 06 Dec 2019 10:34:53 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

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

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................


Patch Set 4:

Thanks Andrew. I'll push this later today.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 4
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: Fri, 06 Dec 2019 13:09:19 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

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

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................


Patch Set 4:

I'm thinking this would be useful for GDB-8.3.2 as well. Would it be ok to backport it?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 4
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: Fri, 06 Dec 2019 13:15:40 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

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

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................


Patch Set 4:

> Patch Set 4:
> 
> I'm thinking this would be useful for GDB-8.3.2 as well. Would it be ok to backport it?

Looking at the schedule, i think that would be GDB 9.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 4
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: Fri, 06 Dec 2019 13:21:04 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

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

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................


Patch Set 4:

> Patch Set 4:
> 
> > Patch Set 4:
> > 
> > I'm thinking this would be useful for GDB-8.3.2 as well. Would it be ok to backport it?
> 
> Looking at the schedule, i think that would be GDB 9.

You don't have to do anything special for it to go in GDB 9 --
the release branch hasn't been made yet.

It's fine if you want to back-port to the 8.3 branch, but I think there
won't be any more official releases from there, so maybe there's not
much reason to do so.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 4
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-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Fri, 06 Dec 2019 14:50:28 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

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

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................

[ARM, sim] Fix build error and warnings

Newer GCC's have switched to -fno-common by default, and this breaks the build
for the ARM sim, like this:

binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:65: multiple definition of `DSPsc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:134: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:64: multiple definition of `DSPacc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:133: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:63: multiple definition of `DSPregs'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:132: first defined here

I also noticed a few warnings due to mismatching types, as follows:

../../../../repos/binutils-gdb/sim/arm/wrapper.c: In function ‘sim_create_inferior’:
../../../../repos/binutils-gdb/sim/arm/wrapper.c:335:16: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
       for (arg = argv; *arg != NULL; arg++)
                ^
../../../../repos/binutils-gdb/sim/arm/wrapper.c:342:8: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    arg = argv;
        ^
../../../../repos/binutils-gdb/sim/arm/wrapper.c:345:13: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    for (arg = argv; *arg != NULL; arg++)
             ^
The following patch fixes both of the above.

sim/arm/ChangeLog:

2019-12-06  Luis Machado  <luis.machado@linaro.org>

	* armemu.c (isize): Move this declaration ...
	* arminit.c (isize): ... here.
	* maverick.h: New file.
	* wrapper.c: Include "maverick.h".
	(<struct maverick_regs>, <union maverick_acc_regs>): Remove and update
	comment.
	(sim_create_inferior): Cast variables to proper type.
	* maverick.c: Include "maverick.h".
	(<struct maverick_regs>, <union maverick_acc_regs>): Move
	declarations to maverick.h and update comment.
	(DSPsc, DSPacc, DSPregs): Adjust comment.

Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
---
M sim/arm/ChangeLog
M sim/arm/armemu.c
M sim/arm/arminit.c
M sim/arm/maverick.c
A sim/arm/maverick.h
M sim/arm/wrapper.c
6 files changed, 71 insertions(+), 67 deletions(-)


diff --git a/sim/arm/ChangeLog b/sim/arm/ChangeLog
index 71097d5..6fb7d7e 100644
--- a/sim/arm/ChangeLog
+++ b/sim/arm/ChangeLog
@@ -1,3 +1,17 @@
+2019-12-06  Luis Machado  <luis.machado@linaro.org>
+
+	* armemu.c (isize): Move this declaration ...
+	* arminit.c (isize): ... here.
+	* maverick.h: New file.
+	* wrapper.c: Include "maverick.h".
+	(<struct maverick_regs>, <union maverick_acc_regs>): Remove and update
+	comment.
+	(sim_create_inferior): Cast variables to proper type.
+	* maverick.c: Include "maverick.h".
+	(<struct maverick_regs>, <union maverick_acc_regs>): Move
+	declarations to maverick.h and update comment.
+	(DSPsc, DSPacc, DSPregs): Adjust comment.
+
 2018-01-02  Nick Clifton  <nickc@redhat.com>
 
 	PR 22663
diff --git a/sim/arm/armemu.c b/sim/arm/armemu.c
index 76f398b..3a72277 100644
--- a/sim/arm/armemu.c
+++ b/sim/arm/armemu.c
@@ -1140,10 +1140,6 @@
 
 /* EMULATION of ARM6.  */
 
-/* The PC pipeline value depends on whether ARM
-   or Thumb instructions are being executed.  */
-ARMword isize;
-
 ARMword
 #ifdef MODE32
 ARMul_Emulate32 (ARMul_State * state)
diff --git a/sim/arm/arminit.c b/sim/arm/arminit.c
index 851d356..3a626c8 100644
--- a/sim/arm/arminit.c
+++ b/sim/arm/arminit.c
@@ -40,6 +40,10 @@
 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;
+
 /***************************************************************************\
 *         Call this routine once to set up the emulator's tables.           *
 \***************************************************************************/
diff --git a/sim/arm/maverick.c b/sim/arm/maverick.c
index c112692..bae8c47 100644
--- a/sim/arm/maverick.c
+++ b/sim/arm/maverick.c
@@ -19,6 +19,7 @@
 #include "armdefs.h"
 #include "ansidecl.h"
 #include "armemu.h"
+#include "maverick.h"
 
 /*#define CIRRUS_DEBUG 1	*/
 #if CIRRUS_DEBUG
@@ -30,36 +31,10 @@
 #define POS64(i) ( (~(i)) >> 63 )
 #define NEG64(i) ( (i) >> 63 )
 
-/* Define Co-Processor instruction handlers here.  */
-
-/* Here's ARMulator's DSP definition.  A few things to note:
-   1) it has 16 64-bit registers and 4 72-bit accumulators
-   2) you can only access its registers with MCR and MRC.  */
-
-/* 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.  */
-struct maverick_regs
-{
-  union
-  {
-    int i;
-    float f;
-  } upper;
-
-  union
-  {
-    int i;
-    float f;
-  } lower;
-};
-
-union maverick_acc_regs
-{
-  long double ld;		/* Acc registers are 72-bits.  */
-};
-
+/* These variables are defined here and made extern in maverick.h for use
+   in wrapper.c for now.
+   Eventually the simulator should be made to handle any coprocessor at run
+   time.  */
 struct maverick_regs DSPregs[16];
 union maverick_acc_regs DSPacc[4];
 ARMword DSPsc;
diff --git a/sim/arm/maverick.h b/sim/arm/maverick.h
new file mode 100644
index 0000000..2549d21
--- /dev/null
+++ b/sim/arm/maverick.h
@@ -0,0 +1,46 @@
+/*  maverick.h -- Cirrus/DSP co-processor interface header
+    Copyright (C) 2003-2019 Free Software Foundation, Inc.
+    Contributed by Aldy Hernandez (aldyh@redhat.com).
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 3 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>. */
+
+/* Define Co-Processor instruction handlers here.  */
+
+/* Here's ARMulator's DSP definition.  A few things to note:
+   1) it has 16 64-bit registers and 4 72-bit accumulators
+   2) you can only access its registers with MCR and MRC.  */
+
+struct maverick_regs
+{
+  union
+  {
+    int i;
+    float f;
+  } upper;
+
+  union
+  {
+    int i;
+    float f;
+  } lower;
+};
+
+union maverick_acc_regs
+{
+  long double ld;		/* Acc registers are 72-bits.  */
+};
+
+extern struct maverick_regs DSPregs[16];
+extern union maverick_acc_regs DSPacc[4];
+extern ARMword DSPsc;
diff --git a/sim/arm/wrapper.c b/sim/arm/wrapper.c
index fde5d8c..78a9192 100644
--- a/sim/arm/wrapper.c
+++ b/sim/arm/wrapper.c
@@ -37,6 +37,7 @@
 #include "gdb/signals.h"
 #include "libiberty.h"
 #include "iwmmxt.h"
+#include "maverick.h"
 
 /* TODO: This should get pulled from the SIM_DESC.  */
 host_callback *sim_callback;
@@ -101,38 +102,6 @@
   fprintf (stderr, " %*s\n", size, opbuf);
 }
 
-/* Cirrus DSP registers.
-
-   We need to define these registers outside of maverick.c because
-   maverick.c might not be linked in unless --target=arm9e-* in which
-   case wrapper.c will not compile because it tries to access Cirrus
-   registers.  This should all go away once we get the Cirrus and ARM
-   Coprocessor to coexist in armcopro.c-- aldyh.  */
-
-struct maverick_regs
-{
-  union
-  {
-    int i;
-    float f;
-  } upper;
-
-  union
-  {
-    int i;
-    float f;
-  } lower;
-};
-
-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;
-
 static void
 init (void)
 {
@@ -236,7 +205,7 @@
 {
   int argvlen = 0;
   int mach;
-  char **arg;
+  char * const *arg;
 
   init ();
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 5
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-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: merged

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

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Luis Machado.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/726
......................................................................

[ARM, sim] Fix build error and warnings

Newer GCC's have switched to -fno-common by default, and this breaks the build
for the ARM sim, like this:

binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:65: multiple definition of `DSPsc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:134: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:64: multiple definition of `DSPacc'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:133: first defined here
binutils-gdb.git~gdb-8.3-release/sim/arm/maverick.c:63: multiple definition of `DSPregs'; libsim.a(wrapper.o):binutils-gdb.git~gdb-8.3-release/sim/arm/wrapper.c:132: first defined here

I also noticed a few warnings due to mismatching types, as follows:

../../../../repos/binutils-gdb/sim/arm/wrapper.c: In function ‘sim_create_inferior’:
../../../../repos/binutils-gdb/sim/arm/wrapper.c:335:16: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
       for (arg = argv; *arg != NULL; arg++)
                ^
../../../../repos/binutils-gdb/sim/arm/wrapper.c:342:8: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    arg = argv;
        ^
../../../../repos/binutils-gdb/sim/arm/wrapper.c:345:13: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    for (arg = argv; *arg != NULL; arg++)
             ^
The following patch fixes both of the above.

sim/arm/ChangeLog:

2019-12-06  Luis Machado  <luis.machado@linaro.org>

	* armemu.c (isize): Move this declaration ...
	* arminit.c (isize): ... here.
	* maverick.h: New file.
	* wrapper.c: Include "maverick.h".
	(<struct maverick_regs>, <union maverick_acc_regs>): Remove and update
	comment.
	(sim_create_inferior): Cast variables to proper type.
	* maverick.c: Include "maverick.h".
	(<struct maverick_regs>, <union maverick_acc_regs>): Move
	declarations to maverick.h and update comment.
	(DSPsc, DSPacc, DSPregs): Adjust comment.

Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
---
M sim/arm/ChangeLog
M sim/arm/armemu.c
M sim/arm/arminit.c
M sim/arm/maverick.c
A sim/arm/maverick.h
M sim/arm/wrapper.c
6 files changed, 71 insertions(+), 67 deletions(-)



diff --git a/sim/arm/ChangeLog b/sim/arm/ChangeLog
index 71097d5..6fb7d7e 100644
--- a/sim/arm/ChangeLog
+++ b/sim/arm/ChangeLog
@@ -1,3 +1,17 @@
+2019-12-06  Luis Machado  <luis.machado@linaro.org>
+
+	* armemu.c (isize): Move this declaration ...
+	* arminit.c (isize): ... here.
+	* maverick.h: New file.
+	* wrapper.c: Include "maverick.h".
+	(<struct maverick_regs>, <union maverick_acc_regs>): Remove and update
+	comment.
+	(sim_create_inferior): Cast variables to proper type.
+	* maverick.c: Include "maverick.h".
+	(<struct maverick_regs>, <union maverick_acc_regs>): Move
+	declarations to maverick.h and update comment.
+	(DSPsc, DSPacc, DSPregs): Adjust comment.
+
 2018-01-02  Nick Clifton  <nickc@redhat.com>
 
 	PR 22663
diff --git a/sim/arm/armemu.c b/sim/arm/armemu.c
index 76f398b..3a72277 100644
--- a/sim/arm/armemu.c
+++ b/sim/arm/armemu.c
@@ -1140,10 +1140,6 @@
 
 /* EMULATION of ARM6.  */
 
-/* The PC pipeline value depends on whether ARM
-   or Thumb instructions are being executed.  */
-ARMword isize;
-
 ARMword
 #ifdef MODE32
 ARMul_Emulate32 (ARMul_State * state)
diff --git a/sim/arm/arminit.c b/sim/arm/arminit.c
index 851d356..3a626c8 100644
--- a/sim/arm/arminit.c
+++ b/sim/arm/arminit.c
@@ -40,6 +40,10 @@
 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;
+
 /***************************************************************************\
 *         Call this routine once to set up the emulator's tables.           *
 \***************************************************************************/
diff --git a/sim/arm/maverick.c b/sim/arm/maverick.c
index c112692..bae8c47 100644
--- a/sim/arm/maverick.c
+++ b/sim/arm/maverick.c
@@ -19,6 +19,7 @@
 #include "armdefs.h"
 #include "ansidecl.h"
 #include "armemu.h"
+#include "maverick.h"
 
 /*#define CIRRUS_DEBUG 1	*/
 #if CIRRUS_DEBUG
@@ -30,36 +31,10 @@
 #define POS64(i) ( (~(i)) >> 63 )
 #define NEG64(i) ( (i) >> 63 )
 
-/* Define Co-Processor instruction handlers here.  */
-
-/* Here's ARMulator's DSP definition.  A few things to note:
-   1) it has 16 64-bit registers and 4 72-bit accumulators
-   2) you can only access its registers with MCR and MRC.  */
-
-/* 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.  */
-struct maverick_regs
-{
-  union
-  {
-    int i;
-    float f;
-  } upper;
-
-  union
-  {
-    int i;
-    float f;
-  } lower;
-};
-
-union maverick_acc_regs
-{
-  long double ld;		/* Acc registers are 72-bits.  */
-};
-
+/* These variables are defined here and made extern in maverick.h for use
+   in wrapper.c for now.
+   Eventually the simulator should be made to handle any coprocessor at run
+   time.  */
 struct maverick_regs DSPregs[16];
 union maverick_acc_regs DSPacc[4];
 ARMword DSPsc;
diff --git a/sim/arm/maverick.h b/sim/arm/maverick.h
new file mode 100644
index 0000000..2549d21
--- /dev/null
+++ b/sim/arm/maverick.h
@@ -0,0 +1,46 @@
+/*  maverick.h -- Cirrus/DSP co-processor interface header
+    Copyright (C) 2003-2019 Free Software Foundation, Inc.
+    Contributed by Aldy Hernandez (aldyh@redhat.com).
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 3 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>. */
+
+/* Define Co-Processor instruction handlers here.  */
+
+/* Here's ARMulator's DSP definition.  A few things to note:
+   1) it has 16 64-bit registers and 4 72-bit accumulators
+   2) you can only access its registers with MCR and MRC.  */
+
+struct maverick_regs
+{
+  union
+  {
+    int i;
+    float f;
+  } upper;
+
+  union
+  {
+    int i;
+    float f;
+  } lower;
+};
+
+union maverick_acc_regs
+{
+  long double ld;		/* Acc registers are 72-bits.  */
+};
+
+extern struct maverick_regs DSPregs[16];
+extern union maverick_acc_regs DSPacc[4];
+extern ARMword DSPsc;
diff --git a/sim/arm/wrapper.c b/sim/arm/wrapper.c
index fde5d8c..78a9192 100644
--- a/sim/arm/wrapper.c
+++ b/sim/arm/wrapper.c
@@ -37,6 +37,7 @@
 #include "gdb/signals.h"
 #include "libiberty.h"
 #include "iwmmxt.h"
+#include "maverick.h"
 
 /* TODO: This should get pulled from the SIM_DESC.  */
 host_callback *sim_callback;
@@ -101,38 +102,6 @@
   fprintf (stderr, " %*s\n", size, opbuf);
 }
 
-/* Cirrus DSP registers.
-
-   We need to define these registers outside of maverick.c because
-   maverick.c might not be linked in unless --target=arm9e-* in which
-   case wrapper.c will not compile because it tries to access Cirrus
-   registers.  This should all go away once we get the Cirrus and ARM
-   Coprocessor to coexist in armcopro.c-- aldyh.  */
-
-struct maverick_regs
-{
-  union
-  {
-    int i;
-    float f;
-  } upper;
-
-  union
-  {
-    int i;
-    float f;
-  } lower;
-};
-
-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;
-
 static void
 init (void)
 {
@@ -236,7 +205,7 @@
 {
   int argvlen = 0;
   int mach;
-  char **arg;
+  char * const *arg;
 
   init ();
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I21db699d3b61b2de8c44053e47be4387285af28f
Gerrit-Change-Number: 726
Gerrit-PatchSet: 5
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-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset