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

  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(-)
  

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)
  

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)
  

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)
  

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.
  

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)
  

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
  

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
  

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')
|  	  {
  

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')
|  	  {
  

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.
  

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.
  

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?
  

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.
  

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.