Proposed changes for pdp11 --*magic options

Proposed changes for pdp11 --*magic options

[Stephen Casner]

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

I solicit your feedback regarding a few design choices I've made as
part of implementing two proposed changes related to the ld magic
number options.  These changes affect only the pdp11 emulation and
they are implemented all within an expansion of the customization
files for the pdp11 emulation except for two one-line changes in the
common code as explained below.

And, to allay any concerns about the number of emails I've sent to
this list recently, I don't have any work planned beyond this.

First, to be consistent with the behavior of ld both in Unix v6 at the
beginning of Unix on the PDP11 and in 2.11 BSD at its end, the default
format for the pdp11-aout target should be --omagic, not --nmagic.
That format does not page-align the .data section and generates magic
number 0407 which indicates to the runtime system that the .text
section should not be read-only.  I've changed the default only for
the pdp11 emulation.

Second, I would like to add option --imagic to generate output with
magic number 0411 (defined as IMAGIC in Unix v6 ld.c) that puts both
the .text and .data sections starting at address 0 for loading into
the separate instruction and data spaces of larger PDP11 models.  The
Unix v6 ld selected this format with option -i, but that option
already has another meaning in binutils ld.  In 2.11 BSD the option -z
was added as a synonym for -i, so within the pdp11 emulation I also
implemented that option as a synonym for --imagic.

The expansion of the pdp11 customization consists of adding a pdp11.em
file containing _before_parse() to adjust the default config settings
consistent with --omagic.  It also contains functions _add_options(),
_list_options() and _handle_option() to implement the additional
options including printing pdp11-specific help for all of the --*magic
options.  I also needed a new _get_script() function that compiles in
a sixth linker script where the .data section starts at address 0.
Since genscripts does not produce such a script, when compiling in the
scripts I reuse the script that aligns the data to a page boundary but
edit it to set address 0 instead.

Related to this, I observe that the code in the adjust_*_magic()
functions in bfd/aoutx.h and bfd/pdp11.c that sets the VMA for data is
not effective because the alignment directive in the linker script
overrides it.

I also added a pdp11.sc file instead of using aout.sc so I could
remove some sections that are extraneous for the pdp11 emulation.

The design choices that I want to check with you are as follows:

  - I have used the existing boolean ld_config_type::separate_code to
    indicate when --imagic is seen in the option parsing.  To me its
    meaning seemed consistent with the new way I'm using it, but I
    could add another boolean if that would be preferred.

  - Rather than define another flag bit in bfd_target::object_flags,
    I'm copying the state of the separate_code boolean into a local
    variable in bfd/pdp11.c at _final_link() time for reference later
    in adjust_sizes_and_vmas() as the sections are written.

  - Since the --imagic format intentionally causes the .text and .data
    sections to occupy the same memory addresses, I had to set
    command_line.check_section_addresses = 0.  Would that cause any
    other problems?

  - In my generated epdp11.c, the function name prefix is gldpdp11
    rather than gld_pdp11 since that is how the code was written in
    the emulation from which I borrowed code.  But I also saw the
    underscore included in a different emulation.  Is one of these
    considered correct?

The two one-line changes that I needed to make in common code are as
follows:

  - I needed to add i_magic to enum aout_magic in bfd/libaout.h.

  - In ld/lexsup.c, I needed to set config.text_read_only to TRUE in
    case 'n'.  This is the same as the default value of that boolean
    as initialized in ldmain.c so I assume my change won't affect any
    non-pdp11 emulation.  I need this change so that my change of the
    default value for pdp11 will work.  That boolean is initialized to
    FALSE or TRUE in case 'N' and case OPTION_NO_OMAGIC.

The diff is attached.

                                                        -- Steve

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

diff --git a/bfd/libaout.h b/bfd/libaout.h
index 81ef4cffd5..04efd9b08e 100644
--- a/bfd/libaout.h
+++ b/bfd/libaout.h
@@ -359,7 +359,8 @@ enum aout_magic {
   undecided_magic = 0,
   z_magic,
   o_magic,
-  n_magic
+  n_magic,
+  i_magic
 };
 
 struct aoutdata
diff --git a/bfd/pdp11.c b/bfd/pdp11.c
index 50d006f085..7ee9f94193 100644
--- a/bfd/pdp11.c
+++ b/bfd/pdp11.c
@@ -90,7 +90,8 @@ struct pdp11_external_exec
 #define	A_MAGIC2	NMAGIC
 #define NMAGIC		0410	/* Pure executable.  */
 #define ZMAGIC		0413	/* Demand-paged executable.  */
-#define	A_MAGIC3	0411	/* Separated I&D.  */
+#define	IMAGIC		0411	/* Separated I&D.  */
+#define	A_MAGIC3	IMAGIC
 #define	A_MAGIC4	0405	/* Overlay.  */
 #define	A_MAGIC5	0430	/* Auto-overlay (nonseparate).  */
 #define	A_MAGIC6	0431	/* Auto-overlay (separate).  */
@@ -242,6 +243,10 @@ struct aout_final_link_info
   struct external_nlist *output_syms;
 };
 
+/* Copy of the link_info.separate_code boolean to select the output format with
+   separate instruction and data spaces selected by --imagic */
+static bfd_boolean separate_i_d = FALSE;
+
 reloc_howto_type howto_table_pdp11[] =
 {
   /* type	       rs size bsz  pcrel bitpos ovrf			  sf name     part_inpl readmask  setmask    pcdone */
@@ -988,6 +993,47 @@ adjust_n_magic (bfd *abfd, struct internal_exec *execp)
   N_SET_MAGIC (execp, NMAGIC);
 }
 
+static void
+adjust_i_magic (bfd *abfd, struct internal_exec *execp)
+{
+  file_ptr pos = adata (abfd).exec_bytes_size;
+  bfd_vma vma = 0;
+  int pad;
+  asection *text = obj_textsec (abfd);
+  asection *data = obj_datasec (abfd);
+  asection *bss = obj_bsssec (abfd);
+
+  /* Text.  */
+  text->filepos = pos;
+  if (!text->user_set_vma)
+    text->vma = vma;
+  else
+    vma = text->vma;
+  pos += execp->a_text;
+
+  /* Data.  */
+  data->filepos = pos;
+  if (!data->user_set_vma)
+    data->vma = 0;
+  vma = data->vma;
+
+  /* Since BSS follows data immediately, see if it needs alignment.  */
+  vma += data->size;
+  pad = align_power (vma, bss->alignment_power) - vma;
+  execp->a_data = data->size + pad;
+  pos += execp->a_data;
+
+  /* BSS.  */
+  if (!bss->user_set_vma)
+    bss->vma = vma;
+  else
+    vma = bss->vma;
+
+  /* Fix up exec header.  */
+  execp->a_bss = bss->size;
+  N_SET_MAGIC (execp, IMAGIC);
+}
+
 bfd_boolean
 NAME (aout, adjust_sizes_and_vmas) (bfd *abfd)
 {
@@ -1018,7 +1064,9 @@ NAME (aout, adjust_sizes_and_vmas) (bfd *abfd)
      I understand it better now, but I haven't time to do the cleanup this
      minute.  */
 
-  if (abfd->flags & WP_TEXT)
+  if (separate_i_d)
+    adata (abfd).magic = i_magic;
+  else if (abfd->flags & WP_TEXT)
     adata (abfd).magic = n_magic;
   else
     adata (abfd).magic = o_magic;
@@ -1031,6 +1079,7 @@ NAME (aout, adjust_sizes_and_vmas) (bfd *abfd)
 		{
 		case n_magic: str = "NMAGIC"; break;
 		case o_magic: str = "OMAGIC"; break;
+		case i_magic: str = "IMAGIC"; break;
 		case z_magic: str = "ZMAGIC"; break;
 		default: abort ();
 		}
@@ -1056,6 +1105,9 @@ NAME (aout, adjust_sizes_and_vmas) (bfd *abfd)
     case n_magic:
       adjust_n_magic (abfd, execp);
       break;
+    case i_magic:
+      adjust_i_magic (abfd, execp);
+      break;
     default:
       abort ();
     }
@@ -3624,6 +3676,7 @@ NAME (aout, final_link) (bfd *abfd,
   if (bfd_link_pic (info))
     abfd->flags |= DYNAMIC;
 
+  separate_i_d = info->separate_code;
   aout_info.info = info;
   aout_info.output_bfd = abfd;
   aout_info.contents = NULL;
diff --git a/ld/emulparams/pdp11.sh b/ld/emulparams/pdp11.sh
index 9b6bbbbd25..3f3326d121 100644
--- a/ld/emulparams/pdp11.sh
+++ b/ld/emulparams/pdp11.sh
@@ -1,5 +1,6 @@
-SCRIPT_NAME=aout
+SCRIPT_NAME=pdp11
 OUTPUT_FORMAT="a.out-pdp11"
 TEXT_START_ADDR=0
 TARGET_PAGE_SIZE=8192
+EXTRA_EM_FILE=pdp11
 ARCH=pdp11
diff --git a/ld/emultempl/pdp11.em b/ld/emultempl/pdp11.em
new file mode 100644
index 0000000000..ac1f9805dc
--- /dev/null
+++ b/ld/emultempl/pdp11.em
@@ -0,0 +1,130 @@
+# This shell script emits a C file. -*- C -*-
+#   Copyright (C) 2006-2020 Free Software Foundation, Inc.
+#
+# This file is part of the GNU Binutils.
+#
+# 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, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+
+LDEMUL_BEFORE_PARSE=gldpdp11_before_parse
+
+fragment <<EOF
+
+/* --- \begin{pdp11.em} */
+#include "getopt.h"
+
+static void
+${LDEMUL_BEFORE_PARSE} (void)
+{
+  ldfile_set_output_arch ("`echo ${ARCH}`", bfd_arch_unknown);
+  /* for PDP11 Unix compatibility, default to --omagic */
+  config.magic_demand_paged = FALSE;
+  config.text_read_only = FALSE;
+}
+
+/* PDP11 specific options.  */
+#define OPTION_IMAGIC 301
+
+static void
+gld${EMULATION_NAME}_add_options
+  (int ns ATTRIBUTE_UNUSED,
+   char **shortopts,
+   int nl,
+   struct option **longopts,
+   int nrl ATTRIBUTE_UNUSED,
+   struct option **really_longopts ATTRIBUTE_UNUSED)
+{
+  static const char xtra_short[] = "z";
+  static const struct option xtra_long[] =
+  {
+    {"imagic", no_argument, NULL, OPTION_IMAGIC},
+    {NULL, no_argument, NULL, 0}
+  };
+
+  *shortopts = (char *) xrealloc (*shortopts, ns + sizeof (xtra_short));
+  memcpy (*shortopts + ns, &xtra_short, sizeof (xtra_short));
+  *longopts
+    = xrealloc (*longopts, nl * sizeof (struct option) + sizeof (xtra_long));
+  memcpy (*longopts + nl, &xtra_long, sizeof (xtra_long));
+}
+
+static void
+gld${EMULATION_NAME}_list_options (FILE *file)
+{
+  fprintf (file, _("  -N, --omagic   Do not make text readonly, do not page align data (default)\n"));
+  fprintf (file, _("  -n, --nmagic   Make text readonly, align data to next page\n"));
+  fprintf (file, _("  -z, --imagic   Make text readonly, separate instruction and data spaces\n"));
+  fprintf (file, _("  --no-omagic    Equivalent to --nmagic\n"));
+}
+
+static bfd_boolean
+gld${EMULATION_NAME}_handle_option (int optc)
+{
+  switch (optc)
+    {
+    default:
+      return FALSE;
+
+    case 'z':
+    case OPTION_IMAGIC:
+      link_info.separate_code = 1;
+      command_line.check_section_addresses = 0;
+      break;
+    }
+
+  return TRUE;
+}
+
+/* We need a special case to prepare an additional linker script for option
+ * --imagic where the .data section starts at address 0 rather than directly
+ * following the .text section or being aligned to the next page after the
+ * .text section. */
+static char *
+gld${EMULATION_NAME}_get_script (int *isfile)
+EOF
+# Scripts compiled in.
+# sed commands to quote an ld script as a C string.
+sc="-f stringify.sed"
+
+fragment <<EOF
+{
+  *isfile = 0;
+
+  if (bfd_link_relocatable (&link_info) && config.build_constructors)
+    return
+EOF
+sed $sc ldscripts/${EMULATION_NAME}.xu			>> e${EMULATION_NAME}.c
+echo '  ; else if (bfd_link_relocatable (&link_info)) return' >> e${EMULATION_NAME}.c
+sed $sc ldscripts/${EMULATION_NAME}.xr			>> e${EMULATION_NAME}.c
+echo '  ; else if (link_info.separate_code) return'	>> e${EMULATION_NAME}.c
+sed $sc ldscripts/${EMULATION_NAME}.xn | \
+  sed -e "s/ALIGN($TARGET_PAGE_SIZE)/0/"		>> e${EMULATION_NAME}.c
+echo '  ; else if (!config.text_read_only) return'	>> e${EMULATION_NAME}.c
+sed $sc ldscripts/${EMULATION_NAME}.xbn			>> e${EMULATION_NAME}.c
+echo '  ; else if (!config.magic_demand_paged) return'	>> e${EMULATION_NAME}.c
+sed $sc ldscripts/${EMULATION_NAME}.xn			>> e${EMULATION_NAME}.c
+echo '  ; else return'					>> e${EMULATION_NAME}.c
+sed $sc ldscripts/${EMULATION_NAME}.x			>> e${EMULATION_NAME}.c
+echo '; }'						>> e${EMULATION_NAME}.c
+
+fragment <<EOF
+/* --- \end{pdp11.em} */
+
+EOF
+
+LDEMUL_ADD_OPTIONS=gld"$EMULATION_NAME"_add_options
+LDEMUL_HANDLE_OPTION=gld"$EMULATION_NAME"_handle_option
+LDEMUL_LIST_OPTIONS=gld"$EMULATION_NAME"_list_options
+LDEMUL_GET_SCRIPT=gld"$EMULATION_NAME"_get_script
diff --git a/ld/lexsup.c b/ld/lexsup.c
index 3d15cc491d..d14e822b57 100644
--- a/ld/lexsup.c
+++ b/ld/lexsup.c
@@ -928,6 +928,7 @@ parse_args (unsigned argc, char **argv)
 	     Use --call-shared or -Bdynamic for this.  */
 	  break;
 	case 'n':
+	  config.text_read_only = TRUE;
 	  config.magic_demand_paged = FALSE;
 	  input_flags.dynamic = FALSE;
 	  break;
diff --git a/ld/scripttempl/pdp11.sc b/ld/scripttempl/pdp11.sc
new file mode 100644
index 0000000000..995e5826d0
--- /dev/null
+++ b/ld/scripttempl/pdp11.sc
@@ -0,0 +1,56 @@
+# Copyright (C) 2014-2020 Free Software Foundation, Inc.
+#
+# Copying and distribution of this file, with or without modification,
+# are permitted in any medium without royalty provided the copyright
+# notice and this notice are preserved.
+#
+test -z "${BIG_OUTPUT_FORMAT}" && BIG_OUTPUT_FORMAT=${OUTPUT_FORMAT}
+test -z "${LITTLE_OUTPUT_FORMAT}" && LITTLE_OUTPUT_FORMAT=${OUTPUT_FORMAT}
+test -z "${ALIGNMENT}" && ALIGNMENT="2"
+
+cat <<EOF
+/* Copyright (C) 2014-2020 Free Software Foundation, Inc.
+
+   Copying and distribution of this script, with or without modification,
+   are permitted in any medium without royalty provided the copyright
+   notice and this notice are preserved.  */
+
+OUTPUT_FORMAT("${OUTPUT_FORMAT}", "${BIG_OUTPUT_FORMAT}",
+	      "${LITTLE_OUTPUT_FORMAT}")
+OUTPUT_ARCH(${ARCH})
+
+${RELOCATING+${LIB_SEARCH_DIRS}}
+${STACKZERO+${RELOCATING+${STACKZERO}}}
+${SHLIB_PATH+${RELOCATING+${SHLIB_PATH}}}
+${RELOCATING+${EXECUTABLE_SYMBOLS}}
+${RELOCATING+PROVIDE (__stack = 0);}
+SECTIONS
+{
+  ${RELOCATING+. = ${TEXT_START_ADDR};}
+  .text :
+  {
+    CREATE_OBJECT_SYMBOLS
+    *(.text)
+    ${RELOCATING+_etext = .;}
+    ${RELOCATING+__etext = .;}
+    ${PAD_TEXT+${RELOCATING+. = ${DATA_ALIGNMENT};}}
+  }
+  ${RELOCATING+. = ${DATA_ALIGNMENT};}
+  .data :
+  {
+    *(.data)
+    ${CONSTRUCTING+CONSTRUCTORS}
+    ${RELOCATING+_edata  =  .;}
+    ${RELOCATING+__edata  =  .;}
+  }
+  .bss :
+  {
+   ${RELOCATING+ __bss_start = .};
+   *(.bss)
+   *(COMMON)
+   ${RELOCATING+. = ALIGN(${ALIGNMENT});}
+   ${RELOCATING+_end = . };
+   ${RELOCATING+__end = . };
+  }
+}
+EOF

Re: Proposed changes for pdp11 --*magic options

[Nick Clifton]

Hi Stephen,

  First of all, please accept my apologies for taking such a long
  time to reply to your email.

  Secondly to answer your questions:

>   - I have used the existing boolean ld_config_type::separate_code to
>     indicate when --imagic is seen in the option parsing.  To me its
>     meaning seemed consistent with the new way I'm using it, but I
>     could add another boolean if that would be preferred.

No, reusing the current logic makes sense to me too.

>   - Rather than define another flag bit in bfd_target::object_flags,
>     I'm copying the state of the separate_code boolean into a local
>     variable in bfd/pdp11.c at _final_link() time for reference later
>     in adjust_sizes_and_vmas() as the sections are written.

I don't particularly like this approach, but I think that it is the
best solution without making major changes to the BFD library.  Ideally
the link_info structure should be passed down to adjust_sizes_and_vmas
but this would mean changing target vectors.
 
>   - Since the --imagic format intentionally causes the .text and .data
>     sections to occupy the same memory addresses, I had to set
>     command_line.check_section_addresses = 0.  Would that cause any
>     other problems?

None that I can think of.  I would however recommend adding a comment
where you set check_section_addresses to zero, so that readers will
understand why it is done.

>   - In my generated epdp11.c, the function name prefix is gldpdp11
>     rather than gld_pdp11 since that is how the code was written in
>     the emulation from which I borrowed code.  But I also saw the
>     underscore included in a different emulation.  Is one of these
>     considered correct?

No, although personally I like the presence of the underscore as it
helps to separate the name of the target from the name of the tool.

> The two one-line changes that I needed to make in common code are as
> follows:
> 
>   - I needed to add i_magic to enum aout_magic in bfd/libaout.h.
> 
>   - In ld/lexsup.c, I needed to set config.text_read_only to TRUE in
>     case 'n'.  This is the same as the default value of that boolean
>     as initialized in ldmain.c so I assume my change won't affect any
>     non-pdp11 emulation.  I need this change so that my change of the
>     default value for pdp11 will work.  That boolean is initialized to
>     FALSE or TRUE in case 'N' and case OPTION_NO_OMAGIC.

I have no problem with these changes.

> The diff is attached.

The diff itself is fine.  I do have a few requests however:

  * Please could you add documentation to ld/ld.texi describing
    the new option, what it does, and also mentioning that it is
    only for the PDP11.

  * Please could you add a line to ld/NEWS mentioning the new feature. 

  * It would be nice if you could create a new linker test to check
    the behaviour of the new feature.  I appreciate that there are no
    PDP11 specific tests in the linker testsuite (or aout specific tests
    for that matter).  But there is no reason that you cannot add a 
    new directory and put the test(s) inside it.

  * If/when you do decide to submit the patch for inclusion, please
   could you also provide ChangeLog entries describing what is happening.

One final thing - which really should have been the first item - I do not
think that we have an FSF Copyright Assignment on file for you for 
contributions to the binutils project.  A change of this size/complexity
cannot be considered to be "obvious", so an assignment will be needed.
If you would like to fill out the form here and email it off, that will
start the process rolling:

  http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob_plain;f=doc/Copyright/request-assign.changes;hb=HEAD

Cheers
  Nick

Re: Proposed changes for pdp11 --*magic options

[Stephen Casner]

Nick,

>   First of all, please accept my apologies for taking such a long
>   time to reply to your email.

Thanks for your reply.  After there was no response for a while I went
ahead and filed an enhancement bug with the same questions and you
already added some comments there.

> >   - Rather than define another flag bit in bfd_target::object_flags,
> >     I'm copying the state of the separate_code boolean into a local
> >     variable in bfd/pdp11.c at _final_link() time for reference later
> >     in adjust_sizes_and_vmas() as the sections are written.
>
> I don't particularly like this approach, but I think that it is the
> best solution without making major changes to the BFD library.  Ideally
> the link_info structure should be passed down to adjust_sizes_and_vmas
> but this would mean changing target vectors.

I wonder now whether there would be some other operations in the BFD
menagerie that would need a new flag bit in bfd_target::object_flags
in order to implement the right behavior.  For example, an email in
another forum today made me realize that pdp11-aout-objdump -h does
not show the data at address 0 when the binary was made with my new
--imagic option, but for that instance the problem must be not
recognizing the different magic number.  (I have not investigated
yet.)

> The diff itself is fine.  I do have a few requests however:
>
>   * Please could you add documentation to ld/ld.texi describing
>     the new option, what it does, and also mentioning that it is
>     only for the PDP11.

What I was planning to do is add a new section 2.1.7 Options specific
to PDP-11 target.

>   * It would be nice if you could create a new linker test to check
>     the behaviour of the new feature.  I appreciate that there are no
>     PDP11 specific tests in the linker testsuite (or aout specific tests
>     for that matter).  But there is no reason that you cannot add a
>     new directory and put the test(s) inside it.

I'm willing to give this a shot.  You may not have noticed a related
question in my bug comment:  Would it be reasonable to introduce
changes to some of the existing tests to make them compatible with a
16-bit address space and reduce the number of failures for pdp11?
Those are addresses and section sizes for the "MEMORY" and "MEMORY
with symbols" tests that could be reduced to be less than 0x10000.
Also there are some script tests with a souce file having a .long
variable.  If that was .word instead, then it would work for pdp11 as
well.

I have not explored yet to see if there is some global notion of word
size that could be used by the test code or that should be used by nm
so it does not show 32-bit addresses for pdp11.

>   * Please could you add a line to ld/NEWS mentioning the new feature.
>
>   * If/when you do decide to submit the patch for inclusion, please
>    could you also provide ChangeLog entries describing what is happening.

For both of these, do you just want the patch to include edits to
these files?  It seems that the final form of the entries depends upon
the changes being merged into the official repository and committed to
a release.

Is there a way that I should provide the patch other than just a diff
attached to the bug?  I am familiar with forking and pull requests in
github, but I don't know if there is some git-specific method for
feeding changes back to binutils.

> One final thing - which really should have been the first item - I do not
> think that we have an FSF Copyright Assignment on file for you for
> contributions to the binutils project.  A change of this size/complexity
> cannot be considered to be "obvious", so an assignment will be needed.
> If you would like to fill out the form here and email it off, that will
> start the process rolling:

I did that already as soon as I saw your similar request in the bug
comment.  That was on 27 Mar 2020 with subject line "Stephen Lewis
Casner" as requested.  There will be additional files touched beyond
those listed in that email in order to update the documentation and
testsuite and possibly for objdump and nm as mentioned earlier.

Another new question: ld help says:

pdp11-aout-ld: supported targets: a.out-pdp11 srec symbolsrec verilog tekhex binary ihex plugin
pdp11-aout-ld: supported emulations: pdp11

I'm not sure that the list of supported targets is true nor where the
list is specified.  Trying "--oformat srec" gives error file format
not recognized, but so does "--oformat a.out-pdp11".

                                                        -- Steve

Re: Proposed changes for pdp11 --*magic options

[Stephen Casner]

Correcting myself:

> Another new question: ld help says:
>
> pdp11-aout-ld: supported targets: a.out-pdp11 srec symbolsrec verilog tekhex binary ihex plugin
> pdp11-aout-ld: supported emulations: pdp11
>
> I'm not sure that the list of supported targets is true nor where the
> list is specified.  Trying "--oformat srec" gives error file format
> not recognized, but so does "--oformat a.out-pdp11".

I was mininterpreting the error messages.  The file format ld was
complaining about was and incorrent input file.  I guess ld can
produce srec format, but again an email in another forum points out
that the relocation is not correct when offset with -Ttext for the
srec output.  I guess I'm not surprised since it was probably not
tested in the past.  Clearly not all of those supported targets make
sense, so that list should be reduced.

                                                        -- Steve

Re: Proposed changes for pdp11 --*magic options

[Stephen Casner]

Nick,

You detected that my patch for PR 25677 exposed an additional
testsuite failure beyond those that were seen previously.  Is the set
of expected failures recorded somewhere, or is that just a comparison
that you make locally with before-and-after builds?  Shouldn't those
tests be marked with xfail: pdp11-*-* instead?  Or perhaps you've left
them as unexpected errors as a TODO to resolve why they occur?

After finding a fix for the new failure, I've been examining the
others.  Some could be easily fixed:

Running ../../ld/testsuite/ld-misc/defsym.exp ...
ERROR: /Users/casner/epos/binutils/binutils-gdb/ld/testsuite/ld-misc/start.s: assembly failed
UNRESOLVED: Build start.o

The assembler error is "Can not represent BFD_RELOC_32 relocation in
this object file format".  That occurs on the source line:

	.long foo

where foo is an address label that needs to be relocated.  This error
can be easily avoided and allow the test to pass by replacing .long
with .word or .dc.a so the relocation fits the 16-bit address for
pdp11.  Would that cause a problem for any other targets?


FAIL: ld-scripts/default-script1
FAIL: ld-scripts/default-script2
FAIL: ld-scripts/default-script3
FAIL: ld-scripts/default-script4

These can easily be fixed by changing the test symbol values from
0x8000000 and 0x9000000 to 0x8000 and 0x9000 so they fit as 16-bit
symbol values.  From what I see, the choice of values is arbitrary.


FAIL: ld-scripts/empty-address-1
FAIL: ld-scripts/empty-address-2a
FAIL: ld-scripts/empty-address-2b

These tests need 0x2000000 changed to 0x2000 and .long changed to
.word as mentioned for other tests, but here there may be a real bug
that needs to be fixed.  The tests are checking whether symbols are
defined various ways in a linker script have the correct value.  In
the pdp11 target, the symbols are undefined which suggests some
problem with inserting symbols from a linker script into the output
symbol table.  I don't know the code well enough to have an idea where
that problem might be.


FAIL: ld-scripts/pr14962
FAIL: ld-scripts/pr14962-2
FAIL: ld-scripts/pr22267

Additional instances of symbols in a linker script showing as
undefined in the output.


FAIL: ld-scripts/pr18963

This one is more complicated.  Starting with data.o consisting on only
a header and zero-length text, data and bss the linker script creates
sections that are each 0x10000 long with symbols corresponding to the
section addresses just to test whether addition is commutative using
those symbols.  The output generated by that linker script is much
larger than indicated by the a.out header (and several times larger
than the 16-bit address space) making the format invalid, so nm gives
an error.  Unless the test could be implemented with sections created
in the object file and just have the symbols defined in the linker
script based on those sections, this test is not supported for
pdp11-aout.


FAIL: MEMORY
FAIL: MEMORY with symbols

These scripts fail for two reasons:

1) The sizes of the text and data sections are too large to fit
together in a 16-bit address space, so the resulting symbol values
masked to 16 bits don't match the expected result.  I believe these
sizes could be reduced without affecting the test.

2) Symbol values with the 0x8000 bit set get sign-extended to 64 bits
in the output of nm, so again they don't match the expected output.
This sign extension is caused by the following statement in
bfd/pdp11.c function NAME (aout, translate_symbol_table):

      in->symbol.value = GET_SWORD (abfd,  ext->e_value);

That line is as written when the PDP11 target was added in 2001.
Since addresses are inherently unsigned, why is sign-extension
appropriate?  There may be offsets that require the result to wrap
around, but all arithmetic on addresses should be masked to the
number of address bits after the arithmetic operation.

Both of these causes could be avoided simply by reducing the numbers
in the test so that the results are all less than 0x8000, but I belive
the sign extension is a separate bug.

                                                        -- Steve

Re: Proposed changes for pdp11 --*magic options

[Nick Clifton]

Hi Stephen,

> You detected that my patch for PR 25677 exposed an additional
> testsuite failure beyond those that were seen previously.  Is the set
> of expected failures recorded somewhere, or is that just a comparison
> that you make locally with before-and-after builds?

Sorry - it is just a local comparison.

>  Shouldn't those
> tests be marked with xfail: pdp11-*-* instead?  Or perhaps you've left
> them as unexpected errors as a TODO to resolve why they occur?

Exactly.  I am fairly confident that most of them can be XFAILed,
but I prefer to add a comment explaining why the XFAIL is there,
and for that I need to understand the reason for the failure.
(Actually, since I am not a PDP11 expert, I am hoping that somebody
else will do this for me...)

 
> ERROR: /Users/casner/epos/binutils/binutils-gdb/ld/testsuite/ld-misc/start.s: assembly failed

> The assembler error is "Can not represent BFD_RELOC_32 relocation in
> this object file format".  That occurs on the source line:
> 
> 	.long foo
> 
> where foo is an address label that needs to be relocated.  This error
> can be easily avoided and allow the test to pass by replacing .long
> with .word or .dc.a so the relocation fits the 16-bit address for
> pdp11.  Would that cause a problem for any other targets?

Using .dc.a should work for all targets, so that is what I would recommend.

> FAIL: ld-scripts/default-script1
> FAIL: ld-scripts/default-script2
> FAIL: ld-scripts/default-script3
> FAIL: ld-scripts/default-script4
> 
> These can easily be fixed by changing the test symbol values from
> 0x8000000 and 0x9000000 to 0x8000 and 0x9000 so they fit as 16-bit
> symbol values.  From what I see, the choice of values is arbitrary.

Agreed.

> FAIL: ld-scripts/empty-address-1
> FAIL: ld-scripts/empty-address-2a
> FAIL: ld-scripts/empty-address-2b
> 
> These tests need 0x2000000 changed to 0x2000 and .long changed to
> .word as mentioned for other tests, but here there may be a real bug
> that needs to be fixed.  The tests are checking whether symbols are
> defined various ways in a linker script have the correct value.  In
> the pdp11 target, the symbols are undefined which suggests some
> problem with inserting symbols from a linker script into the output
> symbol table.  I don't know the code well enough to have an idea where
> that problem might be.

OK, then these should be left as FAIL results for now.


> FAIL: ld-scripts/pr18963
> 
> This one is more complicated.  Starting with data.o consisting on only
> a header and zero-length text, data and bss the linker script creates
> sections that are each 0x10000 long with symbols corresponding to the
> section addresses just to test whether addition is commutative using
> those symbols.  The output generated by that linker script is much
> larger than indicated by the a.out header (and several times larger
> than the 16-bit address space) making the format invalid, so nm gives
> an error.  Unless the test could be implemented with sections created
> in the object file and just have the symbols defined in the linker
> script based on those sections, this test is not supported for
> pdp11-aout.

This sounds like a reasonable case for an xfail, with a comment along
the lines of "this test creates a binary that is too big for 16-bit architectures".


> 2) Symbol values with the 0x8000 bit set get sign-extended to 64 bits
> in the output of nm, so again they don't match the expected output.
> This sign extension is caused by the following statement in
> bfd/pdp11.c function NAME (aout, translate_symbol_table):
> 
>       in->symbol.value = GET_SWORD (abfd,  ext->e_value);
> 
> That line is as written when the PDP11 target was added in 2001.
> Since addresses are inherently unsigned, why is sign-extension
> appropriate?  There may be offsets that require the result to wrap
> around, but all arithmetic on addresses should be masked to the
> number of address bits after the arithmetic operation.

I would be inclined to say that this is a bug and change the 
macro to GET_WORD.  There are symbols whose value are not addresses
but their signed-ness is not known to the linker, so an unsigned
fetch should be safe.


> Both of these causes could be avoided simply by reducing the numbers
> in the test so that the results are all less than 0x8000, but I belive
> the sign extension is a separate bug.

I would be happy to review a patch containing the changes you have suggested above...

Cheers
  Nick

Re: Proposed changes for pdp11 --*magic options

[Paul Koning]

> On Apr 7, 2020, at 6:06 AM, Nick Clifton via Binutils <binutils@sourceware.org> wrote:
> 
> ...
>> FAIL: ld-scripts/pr18963
>> 
>> This one is more complicated.  Starting with data.o consisting on only
>> a header and zero-length text, data and bss the linker script creates
>> sections that are each 0x10000 long with symbols corresponding to the
>> section addresses just to test whether addition is commutative using
>> those symbols.  The output generated by that linker script is much
>> larger than indicated by the a.out header (and several times larger
>> than the 16-bit address space) making the format invalid, so nm gives
>> an error.  Unless the test could be implemented with sections created
>> in the object file and just have the symbols defined in the linker
>> script based on those sections, this test is not supported for
>> pdp11-aout.
> 
> This sounds like a reasonable case for an xfail, with a comment along
> the lines of "this test creates a binary that is too big for 16-bit architectures".

The GCC test machinery has notation that lets you mark a test case as not supported on 16 bit machines, so it is simply skilled there.  Does binutils have that same mechanism, or something like it?

	paul

Re: Proposed changes for pdp11 --*magic options

[Stephen Casner]

On Thu, 9 Apr 2020, Paul Koning wrote:
> > On Apr 7, 2020, at 6:06 AM, Nick Clifton via Binutils <binutils@sourceware.org> wrote
> > ...
> >> FAIL: ld-scripts/pr18963
> >>
> >> This one is more complicated.  Starting with data.o consisting on only
> >> a header and zero-length text, data and bss the linker script creates
> >> sections that are each 0x10000 long with symbols corresponding to the
> >> section addresses just to test whether addition is commutative using
> >> those symbols.  The output generated by that linker script is much
> >> larger than indicated by the a.out header (and several times larger
> >> than the 16-bit address space) making the format invalid, so nm gives
> >> an error.  Unless the test could be implemented with sections created
> >> in the object file and just have the symbols defined in the linker
> >> script based on those sections, this test is not supported for
> >> pdp11-aout.
> >
> > This sounds like a reasonable case for an xfail, with a comment along
> > the lines of "this test creates a binary that is too big for 16-bit architectures".
>
> The GCC test machinery has notation that lets you mark a test case
> as not supported on 16 bit machines, so it is simply skilled there.
> Does binutils have that same mechanism, or something like it?

I've seen instances of testing for particular targets to exclude a
test as unsupported or as xfail as mentioned above.  I'm not sure if
there is a generic test against the architecture address or word
width.

But for this particular test, I have implemented a proposed
replacement test script that can be supported by the a.out output
format and a 16-bit address space.  I've posted that as a comment on
PR 18963 in bugzilla.

                                                        -- Steve

Re: Proposed changes for pdp11 --*magic options

[Nick Clifton]

Hi Paul,

> The GCC test machinery has notation that lets you mark a test case as not supported on 16 bit machines, so it is simply skilled there.  Does binutils have that same mechanism, or something like it?

No, but it could.

Take a look at the is_elf64 proc in binutils/testsuite/lib/binutils-common.exp
This is used to check for 64-bit ELF targets, but it would not be too difficult
to create a similar function to look for 16-bit aout or coff targets.

Cheers
  Nick