[rfa/rfc] Revised multi-arch process

[rfa/rfc] Revised multi-arch process

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

Hello,

Attached is a revised process for converting a target to the multi-arch 
framework.  I'm especially interested in comments from people who have 
just done a conversion (how near to reality is the below?).

(It also adds it do the doco, Eli?)
Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 5076 bytes --]

2002-05-15  Andrew Cagney  <ac131313@redhat.com>

	* gdbint.texinfo (Target Architecture Definition): Add section
	``Converting an existing Target Architecture to Multi-arch''.

Index: gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.84
diff -u -r1.84 gdbint.texinfo
--- gdbint.texinfo	13 May 2002 17:20:59 -0000	1.84
+++ gdbint.texinfo	15 May 2002 06:54:38 -0000
@@ -3764,6 +3764,165 @@
 @file{config/tm-@var{os}.h}.
 
 
+@section Converting an existing Target Architecture to Multi-arch
+
+This section describes the current accepted best practice for converting
+an an existing target architecture to the multi-arch framework.
+
+The process consists of generating, testing, posting and committing a
+sequence of patches.  Each patch must contain a single change, for
+instance:
+
+@itemize @bullet
+
+@item
+Directly convert a group of functions into macros (the conversion does
+not change the behavior of any of the functions).
+
+@item
+Replace a non-multi-arch with a multi-arch mechanism (e.g.,
+@code{FRAME_INFO}).
+
+@item
+Enable multi-arch level one
+
+@item
+Delete one or more files
+
+@end itemize
+
+@noindent
+There isn't a size limit on a patch, however, more smaller patches are
+strongly encouraged.
+
+Since each patch is well defined, and since each change has been tested
+and shows no regressions, the patches are considered @emph{fairly}
+obvious.  Such patches do not need approval.  Occasional steps in the
+process may be more complicated and less clear.  The developer is
+expected to use their judgment and is encouraged to seek advice as
+needed.
+
+@subsection Preparation
+
+The first step is to establish control.  Build (with @option{-Werror}
+enabled) and test the target so that there is a baseline against which
+the debugger can be compared.
+
+At no stage can the test results regress or @value{GDBN} stop compiling
+with @option{-Werror}.
+
+@subsection Add the multi-arch initialization code
+
+The objective of this step is to establish the basic multi-arch
+framework.  It involves
+
+@itemize @bullet
+
+@item
+The addition of a @code{@var{arch}_gdbarch_init} function@footnote{The
+above is from the original example and uses K&R C.  @value{GDBN}
+has since converted to ISO C but lets ignore that.} that creates
+the architecture:
+@smallexample
+static struct gdbarch *
+d10v_gdbarch_init (info, arches)
+     struct gdbarch_info info;
+     struct gdbarch_list *arches;
+@{
+  struct gdbarch *gdbarch;
+  /* there is only one d10v architecture */
+  if (arches != NULL)
+    return arches->gdbarch;
+  gdbarch = gdbarch_alloc (&info, NULL);
+  return gdbarch;
+@}
+@end smallexample
+@noindent
+@emph{}
+
+@item
+A per-architecture dump function to print any architecture specific
+information:
+@smallexample
+static void
+mips_dump_tdep (struct gdbarch *current_gdbarch,
+                struct ui_file *file)
+@{
+   @dots{} code to print architecture specific info @dots{}
+@}
+@end smallexample
+
+@item
+A change to @code{_initialize_@var{arch}_tdep} to register this new
+architecture:
+@smallexample
+void
+_initialize_mips_tdep (void)
+@{
+  gdbarch_register (bfd_arch_mips, mips_gdbarch_init,
+                    mips_dump_tdep);
+@end smallexample
+
+@item
+Add the macro @code{GDB_MULTI_ARCH}, defined as 0 (zero), to the file@*
+@file{config/@var{arch}/tm-@var{arch}.h}.
+
+@end itemize
+
+@subsection Update multi-arch incompatible mechanisms
+
+Some mechanisms do not work with multi-arch.  They include:
+
+@table @code
+@item EXTRA_FRAME_INFO
+Delete.
+@item FRAME_FIND_SAVED_REGS
+Replaced with @code{FRAME_INIT_SAVED_REGS}
+@end table
+
+@noindent
+At this stage you could also consider converting the macros into
+functions.
+
+@subsection Prepare for multi-arch level to one
+
+Temporally set @code{GDB_MULTI_ARCH} to GDB_MULTI_ARCH_PARTIAL and then
+build and start @value{GDBN} (the change should not be committed).
+@value{GDBN} may not build, and once built, it may die with an internal
+error listing the architecture methods that must be provided.
+
+Fix any build problems (patch(es)).
+
+Convert all the architecture methods listed, which are only macros, into
+functions (patch(es)).
+
+Update @code{@var{arch}_gdbarch_init} to set all the missing
+architecture methods and wrap the corresponding macros in @code{#if
+!GDB_MULTI_ARCH} (patch(es)).
+
+@subsection Set multi-arch level one
+
+Change the value of @code{GDB_MULTI_ARCH} to GDB_MULTI_ARCH_PARTIAL (a
+single patch).
+
+Any problems with throwing ``the switch'' should have been fixed
+already.
+
+@subsection Convert remaining macros
+
+Suggest converting macros into functions (and setting the corresponding
+architecture method) in small batches.
+
+@subsection Set multi-arch level to two
+
+This should go smoothly.
+
+@subsection Delete the TM file
+
+The @file{tm-@var{arch}.h} can be deleted.  @file{@var{arch}.mt} and
+@file{configure.in} updated.
+
+
 @node Target Vector Definition
 
 @chapter Target Vector Definition

Re: [rfa/rfc] Revised multi-arch process

[Eli Zaretskii]

> Date: Wed, 15 May 2002 02:58:25 -0400
> From: Andrew Cagney <ac131313@cygnus.com>
> 
> (It also adds it do the doco, Eli?)

Thanks for the heads up, I'm hard-pressed for free time lately, so
might miss docs patches sometimes.  Please ping if I do.

I have a few minor comments:

> +@section Converting an existing Target Architecture to Multi-arch

Suggest an index entry here.  Something like

  @cindex converting targets to multi-arch

> +This section describes the current accepted best practice for converting
> +an an existing target architecture to the multi-arch framework.

The ``an'' is doubled here.

> +@item
> +Enable multi-arch level one
> +
> +@item
> +Delete one or more files

These two @item's need to end with a period, since the other two do.

> +There isn't a size limit on a patch, however, more smaller patches are
> +strongly encouraged.

I think ``more'' is redundant.

> +Temporally set @code{GDB_MULTI_ARCH} to GDB_MULTI_ARCH_PARTIAL and then

GDB_MULTI_ARCH_PARTIAL should be in @code.

Otherwise, okay; thanks.

Re: [rfa/rfc] Revised multi-arch process

[Eli Zaretskii]

On Wed, 15 May 2002, I wrote:

> I have a few minor comments:

Oops, missed one:

> +Temporally set @code{GDB_MULTI_ARCH} to GDB_MULTI_ARCH_PARTIAL

That "Temporally" should be "Temporarily", I think.

Re: [rfa/rfc] Revised multi-arch process

[Michael Snyder]

Eli Zaretskii wrote:
> 
> On Wed, 15 May 2002, I wrote:
> 
> > I have a few minor comments:
> 
> Oops, missed one:
> 
> > +Temporally set @code{GDB_MULTI_ARCH} to GDB_MULTI_ARCH_PARTIAL
> 
> That "Temporally" should be "Temporarily", I think.

Unles Andrew is a secret time traveller...   ;-)

Re: [rfa/rfc] Revised multi-arch process

[Eli Zaretskii]

> Date: Thu, 16 May 2002 10:50:37 -0700
> From: Michael Snyder <msnyder@redhat.com>
> > 
> > That "Temporally" should be "Temporarily", I think.
> 
> Unles Andrew is a secret time traveller...   ;-)

That's the reason for the ``I think'' part ;-)

Re: [rfa/rfc] Revised multi-arch process

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 169 bytes --]

> Date: Wed, 15 May 2002 02:58:25 -0400
>> From: Andrew Cagney <ac131313@cygnus.com>
>> 

> I have a few minor comments:

Fixed.  I've checked in the attached.

Andrew


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 5213 bytes --]

2002-06-01  Andrew Cagney  <ac131313@redhat.com>

	* gdbint.texinfo (Target Architecture Definition): Add section
	``Converting an existing Target Architecture to Multi-arch''.

Index: gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.86
diff -u -r1.86 gdbint.texinfo
--- gdbint.texinfo	31 May 2002 01:36:16 -0000	1.86
+++ gdbint.texinfo	1 Jun 2002 20:11:59 -0000
@@ -3885,6 +3885,168 @@
 @file{config/tm-@var{os}.h}.
 
 
+@section Converting an existing Target Architecture to Multi-arch
+@cindex converting targets to multi-arch
+
+This section describes the current accepted best practice for converting
+an existing target architecture to the multi-arch framework.
+
+The process consists of generating, testing, posting and committing a
+sequence of patches.  Each patch must contain a single change, for
+instance:
+
+@itemize @bullet
+
+@item
+Directly convert a group of functions into macros (the conversion does
+not change the behavior of any of the functions).
+
+@item
+Replace a non-multi-arch with a multi-arch mechanism (e.g.,
+@code{FRAME_INFO}).
+
+@item
+Enable multi-arch level one.
+
+@item
+Delete one or more files.
+
+@end itemize
+
+@noindent
+There isn't a size limit on a patch, however, a developer is strongly
+encouraged to keep the patch size down.
+
+Since each patch is well defined, and since each change has been tested
+and shows no regressions, the patches are considered @emph{fairly}
+obvious.  Such patches, when submitted by developers listed in the
+@file{MAINTAINERS} file, do not need approval.  Occasional steps in the
+process may be more complicated and less clear.  The developer is
+expected to use their judgment and is encouraged to seek advice as
+needed.
+
+@subsection Preparation
+
+The first step is to establish control.  Build (with @option{-Werror}
+enabled) and test the target so that there is a baseline against which
+the debugger can be compared.
+
+At no stage can the test results regress or @value{GDBN} stop compiling
+with @option{-Werror}.
+
+@subsection Add the multi-arch initialization code
+
+The objective of this step is to establish the basic multi-arch
+framework.  It involves
+
+@itemize @bullet
+
+@item
+The addition of a @code{@var{arch}_gdbarch_init} function@footnote{The
+above is from the original example and uses K&R C.  @value{GDBN}
+has since converted to ISO C but lets ignore that.} that creates
+the architecture:
+@smallexample
+static struct gdbarch *
+d10v_gdbarch_init (info, arches)
+     struct gdbarch_info info;
+     struct gdbarch_list *arches;
+@{
+  struct gdbarch *gdbarch;
+  /* there is only one d10v architecture */
+  if (arches != NULL)
+    return arches->gdbarch;
+  gdbarch = gdbarch_alloc (&info, NULL);
+  return gdbarch;
+@}
+@end smallexample
+@noindent
+@emph{}
+
+@item
+A per-architecture dump function to print any architecture specific
+information:
+@smallexample
+static void
+mips_dump_tdep (struct gdbarch *current_gdbarch,
+                struct ui_file *file)
+@{
+   @dots{} code to print architecture specific info @dots{}
+@}
+@end smallexample
+
+@item
+A change to @code{_initialize_@var{arch}_tdep} to register this new
+architecture:
+@smallexample
+void
+_initialize_mips_tdep (void)
+@{
+  gdbarch_register (bfd_arch_mips, mips_gdbarch_init,
+                    mips_dump_tdep);
+@end smallexample
+
+@item
+Add the macro @code{GDB_MULTI_ARCH}, defined as 0 (zero), to the file@*
+@file{config/@var{arch}/tm-@var{arch}.h}.
+
+@end itemize
+
+@subsection Update multi-arch incompatible mechanisms
+
+Some mechanisms do not work with multi-arch.  They include:
+
+@table @code
+@item EXTRA_FRAME_INFO
+Delete.
+@item FRAME_FIND_SAVED_REGS
+Replaced with @code{FRAME_INIT_SAVED_REGS}
+@end table
+
+@noindent
+At this stage you could also consider converting the macros into
+functions.
+
+@subsection Prepare for multi-arch level to one
+
+Temporally set @code{GDB_MULTI_ARCH} to @code{GDB_MULTI_ARCH_PARTIAL}
+and then build and start @value{GDBN} (the change should not be
+committed).  @value{GDBN} may not build, and once built, it may die with
+an internal error listing the architecture methods that must be
+provided.
+
+Fix any build problems (patch(es)).
+
+Convert all the architecture methods listed, which are only macros, into
+functions (patch(es)).
+
+Update @code{@var{arch}_gdbarch_init} to set all the missing
+architecture methods and wrap the corresponding macros in @code{#if
+!GDB_MULTI_ARCH} (patch(es)).
+
+@subsection Set multi-arch level one
+
+Change the value of @code{GDB_MULTI_ARCH} to GDB_MULTI_ARCH_PARTIAL (a
+single patch).
+
+Any problems with throwing ``the switch'' should have been fixed
+already.
+
+@subsection Convert remaining macros
+
+Suggest converting macros into functions (and setting the corresponding
+architecture method) in small batches.
+
+@subsection Set multi-arch level to two
+
+This should go smoothly.
+
+@subsection Delete the TM file
+
+The @file{tm-@var{arch}.h} can be deleted.  @file{@var{arch}.mt} and
+@file{configure.in} updated.
+
+
 @node Target Vector Definition
 
 @chapter Target Vector Definition

Re: [rfa/rfc] Revised multi-arch process

[Michael Elizabeth Chastain]

Andrew Cagney writes:
+Since each patch is well defined, and since each change has been tested
+and shows no regressions, the patches are considered @emph{fairly}
+obvious.  Such patches do not need approval.  Occasional steps in the
+process may be more complicated and less clear.  The developer is
+expected to use their judgment and is encouraged to seek advice as
+needed.

Can you add some context that these patches do not need additional
approval for people who are already listed in the MAINTAINERS file.
That is, if Joe Contributor sends us a patch by e-mail, we aren't
automatically going to apply it.

Other than that, this is nice, it makes me want to go multi-arch
something.  :)

Michael C