[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