[PATCH 0/6] PE/COFF linking adjustments

[PATCH 0/6] PE/COFF linking adjustments

[Jan Beulich]

As reported in [1] and [2] the 2.36 release has caused issues with
(at least) Xen Project's linking of EFI binaries from ELF object
files. This series, while consisting of largely independent changes,
tries to address aspects of these issues as well as further ones
(see e.g. [3]) found in the course of making things work (again and,
for some aspects, actually in the first place).

In particular the last two patches adding diagnostics will want to
be considered partly RFC, as I haven't checked them yet for fallout
on a sufficiently large set of targets. But before doing so I first
wanted to see whether the changes are deemed generally acceptable
at all.

Of course I'll happily take suggestions for all of these changes as
to addressing the issues at had in a better way.

1: ld: don't generate base relocations in PE output for absolute symbols
2: bfd: prune COFF/PE section flags setting
3: bfd: refine handling of relocations between debugging sections
4: ld: adjust ld-scripts/map-address.*
5: bfd: don't silently wrap or truncate PE image section RVAs
6: bfd: strip symbols not representable in COFF/PE symbol table

Jan

[1] https://sourceware.org/pipermail/binutils/2021-February/115220.html
[2] https://sourceware.org/pipermail/binutils/2021-February/115430.html
[3] https://sourceware.org/pipermail/binutils/2021-February/115433.html

[PATCH 1/6] ld: don't generate base relocations in PE output for absolute symbols

[Jan Beulich]

It is the very nature of absolute symbols that they don't change even
if the loader decides to put the image at other than its link-time base
address. Of the linker-defined (and PE-specific) symbols __image_base__
(and its alias) needs special casing, as it'll still appear to be
absolute at this point.

A new inquiry function in ldexp.c is needed because PE base relocations
get generated before ldexp_finalize_syms() runs, yet whether a
relocation is needed depends on the ultimate property of a symbol.

ld/
2021-02-XX  Jan Beulich  <jbeulich@suse.com>

	* ldexp.c (ldexp_is_final_sym_absolute): New.
	* ldexp.h (ldexp_is_final_sym_absolute): Declare.
	* pe-dll.c (generate_reloc): Skip absolute symbols.

--- a/ld/ldexp.c
+++ b/ld/ldexp.c
@@ -1696,6 +1696,28 @@ ldexp_finalize_syms (void)
   bfd_hash_traverse (&definedness_table, set_sym_sections, NULL);
 }
 
+/* Determine whether a symbol is going to remain absolute even after
+   ldexp_finalize_syms() has run.  */
+
+bfd_boolean
+ldexp_is_final_sym_absolute (const struct bfd_link_hash_entry *h)
+{
+  if (h->type == bfd_link_hash_defined
+      && h->u.def.section == bfd_abs_section_ptr)
+    {
+      const struct definedness_hash_entry *def;
+
+      if (!h->ldscript_def)
+	return TRUE;
+
+      def = symbol_defined (h->root.string);
+      if (def != NULL)
+	return def->final_sec == bfd_abs_section_ptr;
+    }
+
+  return FALSE;
+}
+
 void
 ldexp_finish (void)
 {
--- a/ld/ldexp.h
+++ b/ld/ldexp.h
@@ -239,6 +239,7 @@ bfd_vma exp_get_abs_int
   (etree_type *, int, char *);
 void ldexp_init (void);
 void ldexp_finalize_syms (void);
+bfd_boolean ldexp_is_final_sym_absolute (const struct bfd_link_hash_entry *);
 void ldexp_finish (void);
 
 #endif
--- a/ld/pe-dll.c
+++ b/ld/pe-dll.c
@@ -1579,13 +1579,13 @@ generate_reloc (bfd *abfd, struct bfd_li
 		  && relocs[i]->howto->type != pe_details->imagebase_reloc)
 		{
 		  struct bfd_symbol *sym = *relocs[i]->sym_ptr_ptr;
+		  const struct bfd_link_hash_entry *blhe
+		    = bfd_wrapped_link_hash_lookup (abfd, info, sym->name,
+						    FALSE, FALSE, FALSE);
 
 		  /* Don't create relocs for undefined weak symbols.  */
 		  if (sym->flags == BSF_WEAK)
 		    {
-		      struct bfd_link_hash_entry *blhe
-			= bfd_wrapped_link_hash_lookup (abfd, info, sym->name,
-						FALSE, FALSE, FALSE);
 		      if (blhe && blhe->type == bfd_link_hash_undefweak)
 			{
 			  /* Check aux sym and see if it is defined or not. */
@@ -1617,6 +1617,12 @@ generate_reloc (bfd *abfd, struct bfd_li
 		      if (!strcmp (s->name, ".eh_frame"))
 			continue;
 		    }
+		  /* Nor for absolute symbols.  */
+		  else if (blhe && ldexp_is_final_sym_absolute (blhe)
+			   && (!blhe->linker_def
+			       || (strcmp(sym->name, "__image_base__")
+				   && strcmp(sym->name, U ("__ImageBase")))))
+		    continue;
 
 		  reloc_data[total_relocs].vma = sec_vma + relocs[i]->address;
 		  reloc_data[total_relocs].idx = total_relocs;

[PATCH 2/6] bfd: prune COFF/PE section flags setting

[Jan Beulich]

It is my understanding that IMAGE_SCN_LNK_* are supposed to communicate
information to the (static) linker, and become at best meaningless in PE
images. I wouldn't call loaders wrong which would refuse to process
sections with any of these bits set. While there's no replacement for
IMAGE_SCN_LNK_COMDAT, use IMAGE_SCN_MEM_DISCARDABLE in place of
IMAGE_SCN_LNK_REMOVE in this case.

bfd/
2021-02-XX  Jan Beulich  <jbeulich@suse.com>

	* coffcode.h (sec_to_styp_flags): Don't set IMAGE_SCN_LNK_* in
	final PE images.

--- a/bfd/coffcode.h
+++ b/bfd/coffcode.h
@@ -672,22 +672,31 @@ sec_to_styp_flags (const char *sec_name,
   /* skip ROM */
   /* skip constRUCTOR */
   /* skip CONTENTS */
+#ifndef COFF_IMAGE_WITH_PE
+  /* I don't think any of the IMAGE_SCN_LNK_* flags set below should be set
+     when the output is PE. Only object files should have them, for the linker
+     to consume.  */
   if ((sec_flags & SEC_IS_COMMON) != 0)
     styp_flags |= IMAGE_SCN_LNK_COMDAT;
+#endif
   if ((sec_flags & SEC_DEBUGGING) != 0)
     styp_flags |= IMAGE_SCN_MEM_DISCARDABLE;
-  if ((sec_flags & SEC_EXCLUDE) != 0 && !is_dbg)
-    styp_flags |= IMAGE_SCN_LNK_REMOVE;
-  if ((sec_flags & SEC_NEVER_LOAD) != 0 && !is_dbg)
+  if ((sec_flags & (SEC_EXCLUDE | SEC_NEVER_LOAD)) != 0 && !is_dbg)
+#ifdef COFF_IMAGE_WITH_PE
+    styp_flags |= IMAGE_SCN_MEM_DISCARDABLE;
+#else
     styp_flags |= IMAGE_SCN_LNK_REMOVE;
+#endif
   /* skip IN_MEMORY */
   /* skip SORT */
+#ifndef COFF_IMAGE_WITH_PE
   if (sec_flags & SEC_LINK_ONCE)
     styp_flags |= IMAGE_SCN_LNK_COMDAT;
   if ((sec_flags
        & (SEC_LINK_DUPLICATES_DISCARD | SEC_LINK_DUPLICATES_SAME_CONTENTS
 	  | SEC_LINK_DUPLICATES_SAME_SIZE)) != 0)
     styp_flags |= IMAGE_SCN_LNK_COMDAT;
+#endif
 
   /* skip LINKER_CREATED */
 

[PATCH 3/6] bfd: refine handling of relocations between debugging sections

[Jan Beulich]

Preliminary remark: While relevant for Dwarf in particular, I would
assume other debug info formats have similar implications. If not, I've
no idea how to correctly deal with the Dwarf case.

Dwarf wants references between the various .debug_* sections to be
section relative. ELF, however, has section relative relocations on only
very few architectures. Hence normal 32-bit / 64-bit data relocations
get used (the ones with correspond to BFD_RELOC_{32,64}). For ELF output
this is not a problem by default as all these sections get placed at VA
zero. For PE output using VA 0 is not an option, as that would place the
section below the image base (see also "bfd: don't silently wrap or
truncate PE image section RVAs"). And even for ELF output this can be a
problem if these sections get assigned real VAs, e.g. when a program or
library wants to be able to access its own debug info.

For 32-bit relocations, relocation overflows would be reported if the
image base isn't small enough, while for 64-bit relocations bad output
(not a section relative value) would silently be generated.

Therefore the section VMA may not be used when determining the output
base for such relocations. Since this is a heuristic, quite a bit of
extra checking is being applied to make sure only the very few affected
relocation types get processed this way.

bfd/
2021-02-XX  Jan Beulich  <jbeulich@suse.com>

	* reloc.c (bfd_perform_relocation): Force output base to zero
	for relocations between debugging sections.

--- a/bfd/reloc.c
+++ b/bfd/reloc.c
@@ -749,6 +749,30 @@ bfd_perform_relocation (bfd *abfd,
   else
     output_base = reloc_target_output_section->vma;
 
+  /* Most architectures have no section relative ELF relocations.  They use
+     ordinary ones instead for representing section relative references between
+     debugging sections, which works fine as long as the section VMA gets set
+     to zero.  While this is the default for ELF output (albeit not a
+     requirement), in particular PE doesn't even allow zero VMAs for any of the
+     sections.  */
+  if(output_base && !howto->pc_relative
+     && bfd_get_flavour (abfd) == bfd_target_elf_flavour
+     && (reloc_target_output_section->flags
+	 & input_section->flags & SEC_DEBUGGING))
+    {
+      /* Since this is a heuristic, apply further checks in an attempt to
+	 exclude relocation types other than simple base ones.  */
+      unsigned int size = bfd_get_reloc_size (howto);
+
+      if (size && !(size & (size - 1))
+          && !(howto->bitsize & (howto->bitsize - 1))
+          && !howto->bitpos && !howto->rightshift
+          && !howto->negate && !howto->partial_inplace
+          && !(howto->src_mask & (howto->src_mask + 1))
+          && !(howto->dst_mask & (howto->dst_mask + 1)))
+	output_base = 0;
+    }
+
   output_base += symbol->section->output_offset;
 
   /* If symbol addresses are in octets, convert to bytes.  */

[PATCH 4/6] ld: adjust ld-scripts/map-address.*

[Jan Beulich]

Without setting an image base address and without naming at least .text,
this test produces entirely bogus PE output. To be honest, even the ELF
output looks odd: .text gets placed at 0x10204, and both foo and bar get
associated with .text despite living below its start address.

Since neither image base nor .text placement are the subject of this
test, specify .text placement explicitly and in the PE case force the
image base to zero.

ld/
2021-02-XX  Jan Beulich  <jbeulich@suse.com>

	* testsuite/ld-scripts/map-address.exp: Set image base to zero
	for PE/COFF.
	* testsuite/ld-scripts/map-address.t: Place .text.

--- a/ld/testsuite/ld-scripts/map-address.exp
+++ b/ld/testsuite/ld-scripts/map-address.exp
@@ -26,9 +26,15 @@ if {![ld_assemble $as $srcdir/$subdir/si
     return
 }
 
+if { [is_pecoff_format] } then {
+    set IMAGE_BASE "--image-base 0"
+} else {
+    set IMAGE_BASE ""
+}
+
 if {![ld_link $ld tmpdir/map-address \
 	 "$LDFLAGS -T $srcdir/$subdir/map-address.t \
-	  tmpdir/map-address.o \
+	  $IMAGE_BASE tmpdir/map-address.o \
 	  -Map tmpdir/map-address.map"]} {
     fail $testname
     return
@@ -51,7 +57,7 @@ set testname "map to directory"
 
 if {![ld_link $ld tmpdir/map-address \
 	 "$LDFLAGS -T $srcdir/$subdir/map-address.t \
-	  tmpdir/map-address.o \
+	  $IMAGE_BASE tmpdir/map-address.o \
 	  -Map tmpdir --output fred"]} {
     fail $testname
     return
@@ -74,7 +80,7 @@ set testname "map to % directory"
 
 if {![ld_link $ld tmpdir/map-address \
 	 "$LDFLAGS -T $srcdir/$subdir/map-address.t \
-	  tmpdir/map-address.o \
+	  $IMAGE_BASE tmpdir/map-address.o \
 	  -Map=tmpdir/% --output fred"]} {
     fail $testname
     return
@@ -97,7 +103,7 @@ set testname "map to %.foo directory"
 
 if {![ld_link $ld tmpdir/map-address \
 	 "$LDFLAGS -T $srcdir/$subdir/map-address.t \
-	  tmpdir/map-address.o \
+	  $IMAGE_BASE tmpdir/map-address.o \
 	  -Map=tmpdir/%.foo --output fred"]} {
     fail $testname
     return
--- a/ld/testsuite/ld-scripts/map-address.t
+++ b/ld/testsuite/ld-scripts/map-address.t
@@ -8,4 +8,7 @@ SECTIONS
   bar = .;
   . = ALIGN (4);
   frob = .;
+
+  . = 0x10000;
+  .text : { *(.text) }
 }

[PATCH 5/6] bfd: don't silently wrap or truncate PE image section RVAs

[Jan Beulich]

In PE images section addresses get expressed as addresses relative to
the image base. Therefore the VA of a section must be no less than the
image base, and after subtraction of the image base the resulting value
should fit in 32 bits. (The issue is particularly obvious to notice when
sections, perhaps because of ELF assumptions, get placed at VA 0 by
default. Debugging info sections as well as .comment, when input files
are ELF, are a good example. All such sections need proper mentioning in
the linker script to avoid this warning.)

There are a number of test cases which previously produced bogus images,
yet still declared the test a success. Like done for other tests
already, force a zero image base for these. This then also allows (and
requires) dropping again xfail-s which 39a7b38fac0e ("Fix linker tests
to work with 16-bit targets") had added to ld-scripts/default-script*.d
(originally as skip-s). This also depends on similar adjustments to
testsuite/ld-scripts/map-address.* made by an earlier patch.

For ld-scripts/print-memory-usage.* I suppose xcoff could be dropped
from the exclusion list by suppressing garbage collection, just like
already done in e.g. (as seen in the diff here) ld-scripts/data.*, but I
didn't want to make unrelated adjustments.

bfd/
2021-02-XX  Jan Beulich  <jbeulich@suse.com>

	* peXXigen.c (_bfd_XXi_swap_scnhdr_out): Diagnose out of range RVA.

ld/
2021-02-XX  Jan Beulich  <jbeulich@suse.com>

	* testsuite/ld-scripts/alignof.exp,
	testsuite/ld-scripts/data.exp,
	testsuite/ld-scripts/default-script.exp,
	testsuite/ld-scripts/log2.exp,
	testsuite/ld-scripts/print-memory-usage.exp,
	testsuite/ld-scripts/sizeof.exp,
	testsuite/ld-undefined/weak-undef.exp: Set image base to zero
	for PE/COFF.
	* testsuite/ld-scripts/default-script1.d,
	testsuite/ld-scripts/default-script2.d,
	testsuite/ld-scripts/default-script3.d,
	testsuite/ld-scripts/default-script4.d: Drop xfail and comment.
---
RFC: There are likely more (target specific) testsuite changes needed,
     as I've only run Cygwin and MingW target tests so far.

--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -933,11 +933,12 @@ _bfd_XXi_swap_scnhdr_out (bfd * abfd, vo
 
   memcpy (scnhdr_ext->s_name, scnhdr_int->s_name, sizeof (scnhdr_int->s_name));
 
-  PUT_SCNHDR_VADDR (abfd,
-		    ((scnhdr_int->s_vaddr
-		      - pe_data (abfd)->pe_opthdr.ImageBase)
-		     & 0xffffffff),
-		    scnhdr_ext->s_vaddr);
+  ss = scnhdr_int->s_vaddr - pe_data (abfd)->pe_opthdr.ImageBase;
+  if (scnhdr_int->s_vaddr < pe_data (abfd)->pe_opthdr.ImageBase)
+    _bfd_error_handler("%pB:%.8s: section below image base", abfd, scnhdr_int->s_name);
+  else if(ss != (ss & 0xffffffff))
+    _bfd_error_handler("%pB:%.8s: RVA truncated", abfd, scnhdr_int->s_name);
+  PUT_SCNHDR_VADDR (abfd, ss & 0xffffffff, scnhdr_ext->s_vaddr);
 
   /* NT wants the size data to be rounded up to the next
      NT_FILE_ALIGNMENT, but zero if it has no content (as in .bss,
--- a/ld/testsuite/ld-scripts/alignof.exp
+++ b/ld/testsuite/ld-scripts/alignof.exp
@@ -32,7 +32,14 @@ if ![ld_assemble $as $srcdir/$subdir/ali
     return
 }
 
-if ![ld_link $ld tmpdir/alignof "-T $srcdir/$subdir/alignof.t tmpdir/alignof.o"] {
+if { [is_pecoff_format] } {
+    set IMAGE_BASE "--image-base 0"
+} else {
+    set IMAGE_BASE ""
+}
+
+if ![ld_link $ld tmpdir/alignof "-T $srcdir/$subdir/alignof.t \
+	$IMAGE_BASE tmpdir/alignof.o"] {
     fail $testname
     return
 }
--- a/ld/testsuite/ld-scripts/data.exp
+++ b/ld/testsuite/ld-scripts/data.exp
@@ -20,7 +20,9 @@
 # MA 02110-1301, USA.
 
 set old_LDFLAGS $LDFLAGS
-if { [is_xcoff_format] } then {
+if { [is_pecoff_format] } then {
+    set LDFLAGS "$LDFLAGS --image-base 0"
+} elseif { [is_xcoff_format] } then {
     set LDFLAGS "$LDFLAGS -bnogc"
 }
 
--- a/ld/testsuite/ld-scripts/default-script.exp
+++ b/ld/testsuite/ld-scripts/default-script.exp
@@ -21,6 +21,8 @@
 set old_ldflags $LDFLAGS
 if { [istarget spu*-*-*] } {
     set LDFLAGS "$LDFLAGS --local-store 0:0"
+} elseif { [is_pecoff_format] } {
+    set LDFLAGS "$LDFLAGS --image-base 0"
 } elseif { [is_xcoff_format] } {
     set LDFLAGS "$LDFLAGS -bnogc"
 }
--- a/ld/testsuite/ld-scripts/default-script1.d
+++ b/ld/testsuite/ld-scripts/default-script1.d
@@ -1,8 +1,6 @@
 #source: default-script.s
 #ld: -defsym _START=0x800 -T default-script.t
 #nm: -n
-#xfail: {[is_pecoff_format x86_64-*]}
-# Skipped on Mingw64 and Cygwin because the image base defaults to 0x100000000
 
 #...
 0*800 . _START
--- a/ld/testsuite/ld-scripts/default-script2.d
+++ b/ld/testsuite/ld-scripts/default-script2.d
@@ -1,8 +1,6 @@
 #source: default-script.s
 #ld: -T default-script.t -defsym _START=0x800
 #nm: -n
-#xfail: {[is_pecoff_format x86_64-*]}
-# Skipped on Mingw64 and Cygwin because the image base defaults to 0x100000000
 
 #...
 0*800 . _START
--- a/ld/testsuite/ld-scripts/default-script3.d
+++ b/ld/testsuite/ld-scripts/default-script3.d
@@ -1,8 +1,6 @@
 #source: default-script.s
 #ld: -defsym _START=0x800 -dT default-script.t
 #nm: -n
-#xfail: {[is_pecoff_format x86_64-*]}
-# Skipped on Mingw64 and Cygwin because the image base defaults to 0x100000000
 
 #...
 0*800 . _START
--- a/ld/testsuite/ld-scripts/default-script4.d
+++ b/ld/testsuite/ld-scripts/default-script4.d
@@ -1,8 +1,6 @@
 #source: default-script.s
 #ld: --default-script default-script.t -defsym _START=0x800
 #nm: -n
-#xfail: {[is_pecoff_format x86_64-*]}
-# Skipped on Mingw64 and Cygwin because the image base defaults to 0x100000000
 
 #...
 0*800 . _START
--- a/ld/testsuite/ld-scripts/log2.exp
+++ b/ld/testsuite/ld-scripts/log2.exp
@@ -26,7 +26,14 @@ if {![ld_assemble $as $srcdir/$subdir/lo
     return
 }
 
-if {![ld_link $ld tmpdir/log2 "$LDFLAGS -T $srcdir/$subdir/log2.t tmpdir/log2.o"]} {
+if { [is_pecoff_format] } {
+    set IMAGE_BASE "--image-base 0"
+} else {
+    set IMAGE_BASE ""
+}
+
+if {![ld_link $ld tmpdir/log2 "$LDFLAGS -T $srcdir/$subdir/log2.t \
+	$IMAGE_BASE tmpdir/log2.o"]} {
     fail $testname
 } else {
     pass $testname
--- a/ld/testsuite/ld-scripts/print-memory-usage.exp
+++ b/ld/testsuite/ld-scripts/print-memory-usage.exp
@@ -33,6 +33,11 @@ if { [istarget mips*-*-*]
     return
 }
 
+set old_LDFLAGS $LDFLAGS
+if { [is_pecoff_format] } {
+    set LDFLAGS "$LDFLAGS --image-base 0"
+}
+
 run_ld_link_tests {
     {
 	"print-memory-usage-1"
@@ -66,3 +71,4 @@ run_ld_link_tests {
 
 }
 
+set LDFLAGS $old_LDFLAGS
--- a/ld/testsuite/ld-scripts/sizeof.exp
+++ b/ld/testsuite/ld-scripts/sizeof.exp
@@ -27,7 +27,14 @@ if ![ld_assemble $as $srcdir/$subdir/siz
     return
 }
 
-if ![ld_link $ld tmpdir/sizeof "$LDFLAGS -T $srcdir/$subdir/sizeof.t tmpdir/sizeof.o"] {
+if { [is_pecoff_format] } {
+    set IMAGE_BASE "--image-base 0"
+} else {
+    set IMAGE_BASE ""
+}
+
+if ![ld_link $ld tmpdir/sizeof "$LDFLAGS -T $srcdir/$subdir/sizeof.t \
+	$IMAGE_BASE tmpdir/sizeof.o"] {
     fail $testname
     return
 }
--- a/ld/testsuite/ld-undefined/weak-undef.exp
+++ b/ld/testsuite/ld-undefined/weak-undef.exp
@@ -23,14 +23,20 @@
 # some a.out targets too.
 set testname "weak undefined data symbols"
 
+if { [is_pecoff_format] } then {
+    set IMAGE_BASE "--image-base 0"
+} else {
+    set IMAGE_BASE ""
+}
+
 if { ![is_elf_format] && ![is_pecoff_format] } then {
     unsupported $testname
 } elseif {![ld_assemble $as $srcdir/$subdir/weak-undef.s \
 	    tmpdir/weak-undef.o]} then {
     # It's OK if .weak doesn't work on this target.
     unsupported $testname
-} elseif {![ld_link $ld tmpdir/weak-undef \
-		"tmpdir/weak-undef.o -T $srcdir/$subdir/weak-undef.t"]} then {
+} elseif {![ld_link $ld tmpdir/weak-undef "tmpdir/weak-undef.o \
+		-T $srcdir/$subdir/weak-undef.t $IMAGE_BASE"]} then {
     # Weak symbols are broken for non-i386 PE targets.
     if {! [istarget i?86-*-*]} {
 	setup_xfail *-*-pe*

[PATCH 6/6] bfd: strip symbols not representable in COFF/PE symbol table

[Jan Beulich]

The offset-within-section field in the symbol table entry is only 32
bits wide, so rather than emitting bogus entries omit them, and issue
a diagnostic identifying the issue.

This requires adjusting the PR/22267 test to no longer produce symbols
with out of range values on 64-bit BFD. This also depends on
adjustments to testsuite/ld-scripts/map-address.* made by an earlier
patch. The purpose of the test can very well be achieved nevertheless.

bfd/
2021-02-XX  Jan Beulich  <jbeulich@suse.com>

	* cofflink.c (_bfd_coff_write_global_sym): Range-check symbol
	offset.

ld/
2021-02-XX  Jan Beulich  <jbeulich@suse.com>

	* testsuite/ld-scripts/pr22267.t: Avoid symbol value with more
	than 32 set bits.
	* testsuite/ld-scripts/pr22267.d: Adjust expectation and drop
	comment.
---
RFC: There are likely more (target specific) testsuite changes needed,
     as I've only run Cygwin and MingW target tests so far.

--- a/bfd/cofflink.c
+++ b/bfd/cofflink.c
@@ -2602,6 +2602,16 @@ _bfd_coff_write_global_sym (struct bfd_h
 			+ h->root.u.def.section->output_offset);
 	if (! obj_pe (flaginfo->output_bfd))
 	  isym.n_value += sec->vma;
+#ifdef BFD64
+	if (isym.n_value > (bfd_vma)0xffffffff)
+	  {
+	    if (! h->root.linker_def)
+	      _bfd_error_handler
+	        (_("%pB: stripping non-representable symbol '%s' (value %"BFD_VMA_FMT"x)"),
+	         output_bfd, h->root.root.string, isym.n_value);
+	    return TRUE;
+	  }
+#endif
       }
       break;
 
--- a/ld/testsuite/ld-scripts/pr22267.d
+++ b/ld/testsuite/ld-scripts/pr22267.d
@@ -2,7 +2,6 @@
 #nm: -n
 #xfail: bfin-*-linux* frv-*-linux*
 
-# Some targets may zero-extend 32-bit address to 64 bits.
 #...
-0*f+00 A foo
+0*ff A foo
 #pass
--- a/ld/testsuite/ld-scripts/pr22267.t
+++ b/ld/testsuite/ld-scripts/pr22267.t
@@ -1,4 +1,4 @@
 SECTIONS
 {
-  foo = ~0xFF;
+  foo = ~~0xFF;
 }

Re: [PATCH 1/6] ld: don't generate base relocations in PE output for absolute symbols

[Jan Beulich]

On 02.03.2021 10:47, Jan Beulich via Binutils wrote:
> It is the very nature of absolute symbols that they don't change even
> if the loader decides to put the image at other than its link-time base
> address. Of the linker-defined (and PE-specific) symbols __image_base__
> (and its alias) needs special casing, as it'll still appear to be
> absolute at this point.
> 
> A new inquiry function in ldexp.c is needed because PE base relocations
> get generated before ldexp_finalize_syms() runs, yet whether a
> relocation is needed depends on the ultimate property of a symbol.
> 
> ld/
> 2021-02-XX  Jan Beulich  <jbeulich@suse.com>
> 
> 	* ldexp.c (ldexp_is_final_sym_absolute): New.
> 	* ldexp.h (ldexp_is_final_sym_absolute): Declare.
> 	* pe-dll.c (generate_reloc): Skip absolute symbols.

Below the testcase I'm going to add to this change.

Jan

--- a/ld/testsuite/ld-pe/pe.exp
+++ b/ld/testsuite/ld-pe/pe.exp
@@ -76,6 +76,8 @@ run_dump_test "longsecn-5"
 run_dump_test "orphan"
 run_dump_test "orphan_nu"
 
+run_dump_test "reloc"
+
 run_dump_test "weakdef-1"
 
 run_dump_test "pr19803"
--- /dev/null
+++ b/ld/testsuite/ld-pe/reloc.d
@@ -0,0 +1,14 @@
+#name: PE base relocations
+#ld: --enable-reloc-section
+#objdump: -p
+
+.*:     file format .*
+
+#...
+PE File Base Relocations.*
+Virtual Address: .* Number of fixups 4
+[ 	]*reloc    0 offset    0 .* (LOW|HIGHLOW|DIR64)
+[ 	]*reloc    1 offset    [248] .* (LOW|HIGHLOW|DIR64)
+[ 	]*reloc    2 offset   [124]0 .* (LOW|HIGHLOW|DIR64)
+[ 	]*reloc    3 offset    0 .* ABSOLUTE
+#pass
--- /dev/null
+++ b/ld/testsuite/ld-pe/reloc.s
@@ -0,0 +1,13 @@
+	.data
+	.p2align 4
+start:
+	.dc.a	__image_base__
+	.dc.a	start
+	.dc.a	__section_alignment__
+	.dc.a	__file_alignment__
+	.dc.a	__major_os_version__
+	.dc.a	__minor_os_version__
+	.dc.a	__major_subsystem_version__
+	.dc.a	__minor_subsystem_version__
+	.dc.a	end
+end:

Re: [PATCH 1/6] ld: don't generate base relocations in PE output for absolute symbols

[Alan Modra]

On Tue, Mar 02, 2021 at 10:47:03AM +0100, Jan Beulich via Binutils wrote:
> It is the very nature of absolute symbols that they don't change even
> if the loader decides to put the image at other than its link-time base
> address. Of the linker-defined (and PE-specific) symbols __image_base__
> (and its alias) needs special casing, as it'll still appear to be
> absolute at this point.
> 
> A new inquiry function in ldexp.c is needed because PE base relocations
> get generated before ldexp_finalize_syms() runs, yet whether a
> relocation is needed depends on the ultimate property of a symbol.
> 
> ld/
> 2021-02-XX  Jan Beulich  <jbeulich@suse.com>
> 
> 	* ldexp.c (ldexp_is_final_sym_absolute): New.
> 	* ldexp.h (ldexp_is_final_sym_absolute): Declare.
> 	* pe-dll.c (generate_reloc): Skip absolute symbols.

OK.

> +		  /* Nor for absolute symbols.  */
> +		  else if (blhe && ldexp_is_final_sym_absolute (blhe)
> +			   && (!blhe->linker_def
> +			       || (strcmp(sym->name, "__image_base__")
> +				   && strcmp(sym->name, U ("__ImageBase")))))

Space after strcmp.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 2/6] bfd: prune COFF/PE section flags setting

[Alan Modra]

On Tue, Mar 02, 2021 at 10:47:44AM +0100, Jan Beulich via Binutils wrote:
> It is my understanding that IMAGE_SCN_LNK_* are supposed to communicate
> information to the (static) linker, and become at best meaningless in PE
> images. I wouldn't call loaders wrong which would refuse to process
> sections with any of these bits set. While there's no replacement for
> IMAGE_SCN_LNK_COMDAT, use IMAGE_SCN_MEM_DISCARDABLE in place of
> IMAGE_SCN_LNK_REMOVE in this case.
> 
> bfd/
> 2021-02-XX  Jan Beulich  <jbeulich@suse.com>
> 
> 	* coffcode.h (sec_to_styp_flags): Don't set IMAGE_SCN_LNK_* in
> 	final PE images.

OK.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 3/6] bfd: refine handling of relocations between debugging sections

[Alan Modra]

On Tue, Mar 02, 2021 at 10:48:30AM +0100, Jan Beulich via Binutils wrote:
> Preliminary remark: While relevant for Dwarf in particular, I would
> assume other debug info formats have similar implications. If not, I've
> no idea how to correctly deal with the Dwarf case.
> 
> Dwarf wants references between the various .debug_* sections to be
> section relative. ELF, however, has section relative relocations on only
> very few architectures. Hence normal 32-bit / 64-bit data relocations
> get used (the ones with correspond to BFD_RELOC_{32,64}). For ELF output
> this is not a problem by default as all these sections get placed at VA
> zero. For PE output using VA 0 is not an option, as that would place the
> section below the image base (see also "bfd: don't silently wrap or
> truncate PE image section RVAs"). And even for ELF output this can be a
> problem if these sections get assigned real VAs, e.g. when a program or
> library wants to be able to access its own debug info.

So this is for linking ELF with debug info into PE output?

> For 32-bit relocations, relocation overflows would be reported if the
> image base isn't small enough, while for 64-bit relocations bad output
> (not a section relative value) would silently be generated.
> 
> Therefore the section VMA may not be used when determining the output
> base for such relocations. Since this is a heuristic, quite a bit of
> extra checking is being applied to make sure only the very few affected
> relocation types get processed this way.
> 
> bfd/
> 2021-02-XX  Jan Beulich  <jbeulich@suse.com>
> 
> 	* reloc.c (bfd_perform_relocation): Force output base to zero
> 	for relocations between debugging sections.
> 
> --- a/bfd/reloc.c
> +++ b/bfd/reloc.c
> @@ -749,6 +749,30 @@ bfd_perform_relocation (bfd *abfd,
>    else
>      output_base = reloc_target_output_section->vma;
>  
> +  /* Most architectures have no section relative ELF relocations.  They use
> +     ordinary ones instead for representing section relative references between
> +     debugging sections, which works fine as long as the section VMA gets set
> +     to zero.  While this is the default for ELF output (albeit not a
> +     requirement), in particular PE doesn't even allow zero VMAs for any of the
> +     sections.  */
> +  if(output_base && !howto->pc_relative
> +     && bfd_get_flavour (abfd) == bfd_target_elf_flavour
> +     && (reloc_target_output_section->flags
> +	 & input_section->flags & SEC_DEBUGGING))
> +    {
> +      /* Since this is a heuristic, apply further checks in an attempt to
> +	 exclude relocation types other than simple base ones.  */
> +      unsigned int size = bfd_get_reloc_size (howto);
> +
> +      if (size && !(size & (size - 1))
> +          && !(howto->bitsize & (howto->bitsize - 1))
> +          && !howto->bitpos && !howto->rightshift
> +          && !howto->negate && !howto->partial_inplace
> +          && !(howto->src_mask & (howto->src_mask + 1))
> +          && !(howto->dst_mask & (howto->dst_mask + 1)))
> +	output_base = 0;
> +    }
> +
>    output_base += symbol->section->output_offset;
>  
>    /* If symbol addresses are in octets, convert to bytes.  */

When we need this sort of horrible hack, it's time to redesign.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 4/6] ld: adjust ld-scripts/map-address.*

[Alan Modra]

On Tue, Mar 02, 2021 at 10:49:02AM +0100, Jan Beulich via Binutils wrote:
> Without setting an image base address and without naming at least .text,
> this test produces entirely bogus PE output. To be honest, even the ELF
> output looks odd: .text gets placed at 0x10204, and both foo and bar get
> associated with .text despite living below its start address.
> 
> Since neither image base nor .text placement are the subject of this
> test, specify .text placement explicitly and in the PE case force the
> image base to zero.
> 
> ld/
> 2021-02-XX  Jan Beulich  <jbeulich@suse.com>
> 
> 	* testsuite/ld-scripts/map-address.exp: Set image base to zero
> 	for PE/COFF.
> 	* testsuite/ld-scripts/map-address.t: Place .text.

OK.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 5/6] bfd: don't silently wrap or truncate PE image section RVAs

[Alan Modra]

On Tue, Mar 02, 2021 at 10:49:54AM +0100, Jan Beulich via Binutils wrote:
> In PE images section addresses get expressed as addresses relative to
> the image base. Therefore the VA of a section must be no less than the
> image base, and after subtraction of the image base the resulting value
> should fit in 32 bits. (The issue is particularly obvious to notice when
> sections, perhaps because of ELF assumptions, get placed at VA 0 by
> default. Debugging info sections as well as .comment, when input files
> are ELF, are a good example. All such sections need proper mentioning in
> the linker script to avoid this warning.)
> 
> There are a number of test cases which previously produced bogus images,
> yet still declared the test a success. Like done for other tests
> already, force a zero image base for these. This then also allows (and
> requires) dropping again xfail-s which 39a7b38fac0e ("Fix linker tests
> to work with 16-bit targets") had added to ld-scripts/default-script*.d
> (originally as skip-s). This also depends on similar adjustments to
> testsuite/ld-scripts/map-address.* made by an earlier patch.
> 
> For ld-scripts/print-memory-usage.* I suppose xcoff could be dropped
> from the exclusion list by suppressing garbage collection, just like
> already done in e.g. (as seen in the diff here) ld-scripts/data.*, but I
> didn't want to make unrelated adjustments.
> 
> bfd/
> 2021-02-XX  Jan Beulich  <jbeulich@suse.com>
> 
> 	* peXXigen.c (_bfd_XXi_swap_scnhdr_out): Diagnose out of range RVA.
> 
> ld/
> 2021-02-XX  Jan Beulich  <jbeulich@suse.com>
> 
> 	* testsuite/ld-scripts/alignof.exp,
> 	testsuite/ld-scripts/data.exp,
> 	testsuite/ld-scripts/default-script.exp,
> 	testsuite/ld-scripts/log2.exp,
> 	testsuite/ld-scripts/print-memory-usage.exp,
> 	testsuite/ld-scripts/sizeof.exp,
> 	testsuite/ld-undefined/weak-undef.exp: Set image base to zero
> 	for PE/COFF.
> 	* testsuite/ld-scripts/default-script1.d,
> 	testsuite/ld-scripts/default-script2.d,
> 	testsuite/ld-scripts/default-script3.d,
> 	testsuite/ld-scripts/default-script4.d: Drop xfail and comment.

OK, but again you need to add some spaces before function call open
parentheses.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 6/6] bfd: strip symbols not representable in COFF/PE symbol table

[Alan Modra]

On Tue, Mar 02, 2021 at 10:50:59AM +0100, Jan Beulich via Binutils wrote:
> The offset-within-section field in the symbol table entry is only 32
> bits wide, so rather than emitting bogus entries omit them, and issue
> a diagnostic identifying the issue.
> 
> This requires adjusting the PR/22267 test to no longer produce symbols
> with out of range values on 64-bit BFD. This also depends on
> adjustments to testsuite/ld-scripts/map-address.* made by an earlier
> patch. The purpose of the test can very well be achieved nevertheless.
> 
> bfd/
> 2021-02-XX  Jan Beulich  <jbeulich@suse.com>
> 
> 	* cofflink.c (_bfd_coff_write_global_sym): Range-check symbol
> 	offset.
> 
> ld/
> 2021-02-XX  Jan Beulich  <jbeulich@suse.com>
> 
> 	* testsuite/ld-scripts/pr22267.t: Avoid symbol value with more
> 	than 32 set bits.
> 	* testsuite/ld-scripts/pr22267.d: Adjust expectation and drop
> 	comment.

OK.

> --- a/bfd/cofflink.c
> +++ b/bfd/cofflink.c
> @@ -2602,6 +2602,16 @@ _bfd_coff_write_global_sym (struct bfd_h
>  			+ h->root.u.def.section->output_offset);
>  	if (! obj_pe (flaginfo->output_bfd))
>  	  isym.n_value += sec->vma;
> +#ifdef BFD64
> +	if (isym.n_value > (bfd_vma)0xffffffff)

space here after cast.

> +	  {
> +	    if (! h->root.linker_def)
> +	      _bfd_error_handler
> +	        (_("%pB: stripping non-representable symbol '%s' (value %"BFD_VMA_FMT"x)"),

overlong line.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 1/6] ld: don't generate base relocations in PE output for absolute symbols

[Jan Beulich]

On 04.03.2021 05:46, Alan Modra wrote:
> On Tue, Mar 02, 2021 at 10:47:03AM +0100, Jan Beulich via Binutils wrote:
>> It is the very nature of absolute symbols that they don't change even
>> if the loader decides to put the image at other than its link-time base
>> address. Of the linker-defined (and PE-specific) symbols __image_base__
>> (and its alias) needs special casing, as it'll still appear to be
>> absolute at this point.
>>
>> A new inquiry function in ldexp.c is needed because PE base relocations
>> get generated before ldexp_finalize_syms() runs, yet whether a
>> relocation is needed depends on the ultimate property of a symbol.
>>
>> ld/
>> 2021-02-XX  Jan Beulich  <jbeulich@suse.com>
>>
>> 	* ldexp.c (ldexp_is_final_sym_absolute): New.
>> 	* ldexp.h (ldexp_is_final_sym_absolute): Declare.
>> 	* pe-dll.c (generate_reloc): Skip absolute symbols.
> 
> OK.

Just to confirm - subsequently I did send a test case to be added
here, for which I have these additional entries:

	* testsuite/ld-pe/reloc.s, testsuite/ld-pe/reloc.d: New.
	* testsuite/ld-pe/pe.exp: Run new test.

Is that okay to commit as a whole, or should I re-submit (or
submit the test case addition as a separate change)?

>> +		  /* Nor for absolute symbols.  */
>> +		  else if (blhe && ldexp_is_final_sym_absolute (blhe)
>> +			   && (!blhe->linker_def
>> +			       || (strcmp(sym->name, "__image_base__")
>> +				   && strcmp(sym->name, U ("__ImageBase")))))
> 
> Space after strcmp.

Oh, yes, sorry.

Jan

Re: [PATCH 3/6] bfd: refine handling of relocations between debugging sections

[Jan Beulich]

On 04.03.2021 07:10, Alan Modra wrote:
> On Tue, Mar 02, 2021 at 10:48:30AM +0100, Jan Beulich via Binutils wrote:
>> Preliminary remark: While relevant for Dwarf in particular, I would
>> assume other debug info formats have similar implications. If not, I've
>> no idea how to correctly deal with the Dwarf case.
>>
>> Dwarf wants references between the various .debug_* sections to be
>> section relative. ELF, however, has section relative relocations on only
>> very few architectures. Hence normal 32-bit / 64-bit data relocations
>> get used (the ones with correspond to BFD_RELOC_{32,64}). For ELF output
>> this is not a problem by default as all these sections get placed at VA
>> zero. For PE output using VA 0 is not an option, as that would place the
>> section below the image base (see also "bfd: don't silently wrap or
>> truncate PE image section RVAs"). And even for ELF output this can be a
>> problem if these sections get assigned real VAs, e.g. when a program or
>> library wants to be able to access its own debug info.
> 
> So this is for linking ELF with debug info into PE output?

It's unavoidable for this case, yes, but as said even for ELF it can
turn out to be necessary (when the .debug_* sections get assigned a
non-zero [and large enough] VA).

>> --- a/bfd/reloc.c
>> +++ b/bfd/reloc.c
>> @@ -749,6 +749,30 @@ bfd_perform_relocation (bfd *abfd,
>>    else
>>      output_base = reloc_target_output_section->vma;
>>  
>> +  /* Most architectures have no section relative ELF relocations.  They use
>> +     ordinary ones instead for representing section relative references between
>> +     debugging sections, which works fine as long as the section VMA gets set
>> +     to zero.  While this is the default for ELF output (albeit not a
>> +     requirement), in particular PE doesn't even allow zero VMAs for any of the
>> +     sections.  */
>> +  if(output_base && !howto->pc_relative
>> +     && bfd_get_flavour (abfd) == bfd_target_elf_flavour
>> +     && (reloc_target_output_section->flags
>> +	 & input_section->flags & SEC_DEBUGGING))
>> +    {
>> +      /* Since this is a heuristic, apply further checks in an attempt to
>> +	 exclude relocation types other than simple base ones.  */
>> +      unsigned int size = bfd_get_reloc_size (howto);
>> +
>> +      if (size && !(size & (size - 1))
>> +          && !(howto->bitsize & (howto->bitsize - 1))
>> +          && !howto->bitpos && !howto->rightshift
>> +          && !howto->negate && !howto->partial_inplace
>> +          && !(howto->src_mask & (howto->src_mask + 1))
>> +          && !(howto->dst_mask & (howto->dst_mask + 1)))
>> +	output_base = 0;
>> +    }
>> +
>>    output_base += symbol->section->output_offset;
>>  
>>    /* If symbol addresses are in octets, convert to bytes.  */
> 
> When we need this sort of horrible hack, it's time to redesign.

Well, I was fearing that, but I have to admit I have no idea what
would need doing where. Surely the "horrible" aspects could be
reduced by limiting the number of extra checks - as said in
comment and description, I've added them to reduce risk of
mistakenly zapping output_base. But I definitely agree that no
matter how much massaging would be done, it's probably going to
remain ugly without finding an entirely different approach.

This said, I had to admit that I was actually inspired by this
code a few lines up from where I did the addition:

  /* Convert input-section-relative symbol value to absolute.  */
  if ((output_bfd && ! howto->partial_inplace)
      || reloc_target_output_section == NULL)
    output_base = 0;

I may not fully understand the reasons behind it, but it as well
did look like a hack to me.

Jan

Re: [PATCH 6/6] bfd: strip symbols not representable in COFF/PE symbol table

[Jan Beulich]

On 04.03.2021 07:15, Alan Modra wrote:
> On Tue, Mar 02, 2021 at 10:50:59AM +0100, Jan Beulich via Binutils wrote:
>> --- a/bfd/cofflink.c
>> +++ b/bfd/cofflink.c
>> @@ -2602,6 +2602,16 @@ _bfd_coff_write_global_sym (struct bfd_h
>>  			+ h->root.u.def.section->output_offset);
>>  	if (! obj_pe (flaginfo->output_bfd))
>>  	  isym.n_value += sec->vma;
>> +#ifdef BFD64
>> +	if (isym.n_value > (bfd_vma)0xffffffff)
> 
> space here after cast.

Oh, I see.

>> +	  {
>> +	    if (! h->root.linker_def)
>> +	      _bfd_error_handler
>> +	        (_("%pB: stripping non-representable symbol '%s' (value %"BFD_VMA_FMT"x)"),
> 
> overlong line.

What am I to do in this case? From other projects I'm used to the
rule that format strings, to remain grep-able, are allowed to
run past the normal line length limit. For now I'll split before
the opening parentheses, but I'll be waiting with committing this
anyway until I've tested this against a more complete set of
targets (like I'll also do for patch 5).

Jan

Re: [PATCH 1/6] ld: don't generate base relocations in PE output for absolute symbols

[Alan Modra]

On Thu, Mar 04, 2021 at 09:52:06AM +0100, Jan Beulich wrote:
> On 04.03.2021 05:46, Alan Modra wrote:
> > On Tue, Mar 02, 2021 at 10:47:03AM +0100, Jan Beulich via Binutils wrote:
> >> It is the very nature of absolute symbols that they don't change even
> >> if the loader decides to put the image at other than its link-time base
> >> address. Of the linker-defined (and PE-specific) symbols __image_base__
> >> (and its alias) needs special casing, as it'll still appear to be
> >> absolute at this point.
> >>
> >> A new inquiry function in ldexp.c is needed because PE base relocations
> >> get generated before ldexp_finalize_syms() runs, yet whether a
> >> relocation is needed depends on the ultimate property of a symbol.
> >>
> >> ld/
> >> 2021-02-XX  Jan Beulich  <jbeulich@suse.com>
> >>
> >> 	* ldexp.c (ldexp_is_final_sym_absolute): New.
> >> 	* ldexp.h (ldexp_is_final_sym_absolute): Declare.
> >> 	* pe-dll.c (generate_reloc): Skip absolute symbols.
> > 
> > OK.
> 
> Just to confirm - subsequently I did send a test case to be added
> here, for which I have these additional entries:

Yes, that's fine too.

> 	* testsuite/ld-pe/reloc.s, testsuite/ld-pe/reloc.d: New.
> 	* testsuite/ld-pe/pe.exp: Run new test.
> 
> Is that okay to commit as a whole, or should I re-submit (or
> submit the test case addition as a separate change)?
> 
> >> +		  /* Nor for absolute symbols.  */
> >> +		  else if (blhe && ldexp_is_final_sym_absolute (blhe)
> >> +			   && (!blhe->linker_def
> >> +			       || (strcmp(sym->name, "__image_base__")
> >> +				   && strcmp(sym->name, U ("__ImageBase")))))
> > 
> > Space after strcmp.
> 
> Oh, yes, sorry.
> 
> Jan

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 6/6] bfd: strip symbols not representable in COFF/PE symbol table

[Alan Modra]

On Thu, Mar 04, 2021 at 10:06:27AM +0100, Jan Beulich wrote:
> On 04.03.2021 07:15, Alan Modra wrote:
> > On Tue, Mar 02, 2021 at 10:50:59AM +0100, Jan Beulich via Binutils wrote:
> >> +	        (_("%pB: stripping non-representable symbol '%s' (value %"BFD_VMA_FMT"x)"),
> > 
> > overlong line.
> 
> What am I to do in this case? From other projects I'm used to the
> rule that format strings, to remain grep-able, are allowed to
> run past the normal line length limit.

You're already using string literal concatenation there.  I'd split
(with spaces around BFD_VMA_FMT) like this:

	(_("%pB: stripping non-representable symbol '%s' (value "
	   "%" BFD_VMA_FMT "x)"),

or if you prefer, with the "%" left on the first line.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 3/6] bfd: refine handling of relocations between debugging sections

[Alan Modra]

On Thu, Mar 04, 2021 at 10:00:58AM +0100, Jan Beulich wrote:
> On 04.03.2021 07:10, Alan Modra wrote:
> > So this is for linking ELF with debug info into PE output?
> 
> It's unavoidable for this case, yes, but as said even for ELF it can
> turn out to be necessary (when the .debug_* sections get assigned a
> non-zero [and large enough] VA).

Most ELF targets won't use bfd_perform_relocation when linking native
objects, so any hacks here won't help.

> >> --- a/bfd/reloc.c
> >> +++ b/bfd/reloc.c
> >> @@ -749,6 +749,30 @@ bfd_perform_relocation (bfd *abfd,
> >>    else
> >>      output_base = reloc_target_output_section->vma;
> >>  
> >> +  /* Most architectures have no section relative ELF relocations.  They use
> >> +     ordinary ones instead for representing section relative references between
> >> +     debugging sections, which works fine as long as the section VMA gets set
> >> +     to zero.  While this is the default for ELF output (albeit not a
> >> +     requirement), in particular PE doesn't even allow zero VMAs for any of the
> >> +     sections.  */
> >> +  if(output_base && !howto->pc_relative
> >> +     && bfd_get_flavour (abfd) == bfd_target_elf_flavour
> >> +     && (reloc_target_output_section->flags
> >> +	 & input_section->flags & SEC_DEBUGGING))
> >> +    {
> >> +      /* Since this is a heuristic, apply further checks in an attempt to
> >> +	 exclude relocation types other than simple base ones.  */
> >> +      unsigned int size = bfd_get_reloc_size (howto);
> >> +
> >> +      if (size && !(size & (size - 1))
> >> +          && !(howto->bitsize & (howto->bitsize - 1))
> >> +          && !howto->bitpos && !howto->rightshift
> >> +          && !howto->negate && !howto->partial_inplace
> >> +          && !(howto->src_mask & (howto->src_mask + 1))
> >> +          && !(howto->dst_mask & (howto->dst_mask + 1)))
> >> +	output_base = 0;
> >> +    }
> >> +
> >>    output_base += symbol->section->output_offset;
> >>  
> >>    /* If symbol addresses are in octets, convert to bytes.  */
> > 
> > When we need this sort of horrible hack, it's time to redesign.
> 
> Well, I was fearing that, but I have to admit I have no idea what
> would need doing where. Surely the "horrible" aspects could be
> reduced by limiting the number of extra checks - as said in
> comment and description, I've added them to reduce risk of
> mistakenly zapping output_base. But I definitely agree that no
> matter how much massaging would be done, it's probably going to
> remain ugly without finding an entirely different approach.

Yes, it's true all the extra conditions don't inspire confidence.  If
it's OK to do at all then it should be OK with just

  if (output_bfd == NULL
      && bfd_get_flavour (abfd) == bfd_target_elf_flavour
      && (input_section->flags & SEC_DEBUGGING) != 0
      && (symbol->section->flags & SEC_DEBUGGING) != 0)
    output_base = 0;

with a comment saying why, of course.  I can't say I like it though,
and I'm inclined to say the right approach is to fix the bad DWARF.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 1/6] ld: don't generate base relocations in PE output for absolute symbols

[Alan Modra]

On Thu, Mar 04, 2021 at 11:46:09PM +1030, Alan Modra wrote:
> On Thu, Mar 04, 2021 at 09:52:06AM +0100, Jan Beulich wrote:
> > Just to confirm - subsequently I did send a test case to be added
> > here, for which I have these additional entries:
> 
> Yes, that's fine too.

mcore-pe  +FAIL: PE base relocations
sh-pe  +FAIL: PE base relocations

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 1/6] ld: don't generate base relocations in PE output for absolute symbols

[Jan Beulich]

On 05.03.2021 13:49, Alan Modra wrote:
> On Thu, Mar 04, 2021 at 11:46:09PM +1030, Alan Modra wrote:
>> On Thu, Mar 04, 2021 at 09:52:06AM +0100, Jan Beulich wrote:
>>> Just to confirm - subsequently I did send a test case to be added
>>> here, for which I have these additional entries:
>>
>> Yes, that's fine too.
> 
> mcore-pe  +FAIL: PE base relocations
> sh-pe  +FAIL: PE base relocations

Oh, so I should have waited even with this one until I get around
to produce and look at the wider set of targets' results. I hope
getting to this some time next week will be enough - if not,
please tell me and I'll revert.

I'm sorry for the fallout,
Jan

Re: [PATCH 1/6] ld: don't generate base relocations in PE output for absolute symbols

[Alan Modra]

On Fri, Mar 05, 2021 at 02:38:40PM +0100, Jan Beulich wrote:
> On 05.03.2021 13:49, Alan Modra wrote:
> > On Thu, Mar 04, 2021 at 11:46:09PM +1030, Alan Modra wrote:
> >> On Thu, Mar 04, 2021 at 09:52:06AM +0100, Jan Beulich wrote:
> >>> Just to confirm - subsequently I did send a test case to be added
> >>> here, for which I have these additional entries:
> >>
> >> Yes, that's fine too.
> > 
> > mcore-pe  +FAIL: PE base relocations
> > sh-pe  +FAIL: PE base relocations
> 
> Oh, so I should have waited even with this one until I get around
> to produce and look at the wider set of targets' results. I hope
> getting to this some time next week will be enough - if not,
> please tell me and I'll revert.
> 
> I'm sorry for the fallout,

There is no hurry.  It should be easy to fix.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 3/6] bfd: refine handling of relocations between debugging sections

[Jan Beulich]

On 05.03.2021 13:00, Alan Modra wrote:
> On Thu, Mar 04, 2021 at 10:00:58AM +0100, Jan Beulich wrote:
>> On 04.03.2021 07:10, Alan Modra wrote:
>>> So this is for linking ELF with debug info into PE output?
>>
>> It's unavoidable for this case, yes, but as said even for ELF it can
>> turn out to be necessary (when the .debug_* sections get assigned a
>> non-zero [and large enough] VA).
> 
> Most ELF targets won't use bfd_perform_relocation when linking native
> objects, so any hacks here won't help.

I've extended the patch description accordingly. Thanks for pointing
this out to me. Looks like to get ELF working this may then require
in some cases a per-target fix. Afaics a change might be possible in
a somewhat centralized manner in _bfd_elf_rela_local_sym(), but
targets using _bfd_elf_rel_local_sym() would need dealing with
individually. I've not looked into what it would take to deal with
relocations targeting global symbols, both because for debug info
this shouldn't matter and because I'm not after fixing ELF output
anyway.

>>>> --- a/bfd/reloc.c
>>>> +++ b/bfd/reloc.c
>>>> @@ -749,6 +749,30 @@ bfd_perform_relocation (bfd *abfd,
>>>>    else
>>>>      output_base = reloc_target_output_section->vma;
>>>>  
>>>> +  /* Most architectures have no section relative ELF relocations.  They use
>>>> +     ordinary ones instead for representing section relative references between
>>>> +     debugging sections, which works fine as long as the section VMA gets set
>>>> +     to zero.  While this is the default for ELF output (albeit not a
>>>> +     requirement), in particular PE doesn't even allow zero VMAs for any of the
>>>> +     sections.  */
>>>> +  if(output_base && !howto->pc_relative
>>>> +     && bfd_get_flavour (abfd) == bfd_target_elf_flavour
>>>> +     && (reloc_target_output_section->flags
>>>> +	 & input_section->flags & SEC_DEBUGGING))
>>>> +    {
>>>> +      /* Since this is a heuristic, apply further checks in an attempt to
>>>> +	 exclude relocation types other than simple base ones.  */
>>>> +      unsigned int size = bfd_get_reloc_size (howto);
>>>> +
>>>> +      if (size && !(size & (size - 1))
>>>> +          && !(howto->bitsize & (howto->bitsize - 1))
>>>> +          && !howto->bitpos && !howto->rightshift
>>>> +          && !howto->negate && !howto->partial_inplace
>>>> +          && !(howto->src_mask & (howto->src_mask + 1))
>>>> +          && !(howto->dst_mask & (howto->dst_mask + 1)))
>>>> +	output_base = 0;
>>>> +    }
>>>> +
>>>>    output_base += symbol->section->output_offset;
>>>>  
>>>>    /* If symbol addresses are in octets, convert to bytes.  */
>>>
>>> When we need this sort of horrible hack, it's time to redesign.
>>
>> Well, I was fearing that, but I have to admit I have no idea what
>> would need doing where. Surely the "horrible" aspects could be
>> reduced by limiting the number of extra checks - as said in
>> comment and description, I've added them to reduce risk of
>> mistakenly zapping output_base. But I definitely agree that no
>> matter how much massaging would be done, it's probably going to
>> remain ugly without finding an entirely different approach.
> 
> Yes, it's true all the extra conditions don't inspire confidence.  If
> it's OK to do at all then it should be OK with just
> 
>   if (output_bfd == NULL
>       && bfd_get_flavour (abfd) == bfd_target_elf_flavour
>       && (input_section->flags & SEC_DEBUGGING) != 0
>       && (symbol->section->flags & SEC_DEBUGGING) != 0)
>     output_base = 0;
> 
> with a comment saying why, of course.

While, as said, I agree the extra conditions don't look reassuring,
dropping them adds to what we imply towards relocations between
debugging sections. I understand you're okay with that (as far as
"okay" can go with a hack like this)?

With the code snippet above I observe a few more modifications
than just to drop the extra conditions. You use symbol->section
when I used reloc_target_output_section. Is that for a reason?

You've also split my

     && (reloc_target_output_section->flags
	 & input_section->flags & SEC_DEBUGGING))

which was probably just a style change (including the addition of
() != 0 around them). While I don't really see the reason for
this transformation, if that's what binutils style requires I can
surely change. However, meanwhile I've realized that
input_section's flags shouldn't really matter: If there indeed
was a relocation in a non-debugging section targeting a debugging
one, I would suppose output_base should be similarly ignored.
Thoughts? (Such relocations don't seem to make a lot of sense, so
perhaps this aspect is largely academical anyway.)

>  I can't say I like it though,
> and I'm inclined to say the right approach is to fix the bad DWARF.

I wonder what you mean by "fix the bad Dwarf": It's the lack of
suitable relocations in most targets' ELF ABIs which makes this
hack necessary. Are you suggesting all targets supporting Dwarf
should gain new section-relative relocation types?

I ask because - as indicated before - I'd be more than happy to
use a less hacky approach. Going the new-reloc-types route would
be a rather long process though, whereas the desire to not need
to discard debug info when linking EFI executables is a present
one.

Jan

Re: [PATCH 6/6] bfd: strip symbols not representable in COFF/PE symbol table

[Jan Beulich]

On 04.03.2021 10:06, Jan Beulich via Binutils wrote:
> ..., but I'll be waiting with committing this
> anyway until I've tested this against a more complete set of
> targets (like I'll also do for patch 5).

This change is producing a large amount of fallout on mcore. My
understanding is that this is due to ld/scripttempl/mcorepe.sc
having

  .stack 0x80000 :
  {
    _stack = .;
    *(.stack)
  }

This use of a fixed address makes it quite likely that _stack will
end up ahead of all sections (default image base being 0x400000),
and hence have a negative (i.e. apparently huge positive) value. I
have to admit I have a hard time seeing how placing the stack at a
fixed address can do anything good. Besides possibly wrapping the
assignment of _stack in a PROVIDE() (not tested yet) an option
might also be to replace the fixed address by ALIGN(0x80000).

According to the list of maintainers, there's no specific
maintainer for mcore, and the introduction of the above construct
is so old that I can't figure an original author. So it's rather
hard to find a suitable solution here without risking breakage.

Jan

Re: [PATCH 6/6] bfd: strip symbols not representable in COFF/PE symbol table

[Jan Beulich]

On 08.03.2021 16:05, Jan Beulich via Binutils wrote:
> On 04.03.2021 10:06, Jan Beulich via Binutils wrote:
>> ..., but I'll be waiting with committing this
>> anyway until I've tested this against a more complete set of
>> targets (like I'll also do for patch 5).
> 
> This change is producing a large amount of fallout on mcore. My
> understanding is that this is due to ld/scripttempl/mcorepe.sc
> having
> 
>   .stack 0x80000 :
>   {
>     _stack = .;
>     *(.stack)
>   }
> 
> This use of a fixed address makes it quite likely that _stack will
> end up ahead of all sections (default image base being 0x400000),
> and hence have a negative (i.e. apparently huge positive) value. I
> have to admit I have a hard time seeing how placing the stack at a
> fixed address can do anything good. Besides possibly wrapping the
> assignment of _stack in a PROVIDE() (not tested yet) an option
> might also be to replace the fixed address by ALIGN(0x80000).

PROVIDE() looks to fix all the issues, so I guess that'll be the
way to go then.

Jan

Re: [PATCH 3/6] bfd: refine handling of relocations between debugging sections

[Alan Modra]

On Mon, Mar 08, 2021 at 03:17:54PM +0100, Jan Beulich wrote:
> On 05.03.2021 13:00, Alan Modra wrote:
> > On Thu, Mar 04, 2021 at 10:00:58AM +0100, Jan Beulich wrote:
> >> On 04.03.2021 07:10, Alan Modra wrote:
> >>> So this is for linking ELF with debug info into PE output?
> >>
> >> It's unavoidable for this case, yes, but as said even for ELF it can
> >> turn out to be necessary (when the .debug_* sections get assigned a
> >> non-zero [and large enough] VA).
> > 
> > Most ELF targets won't use bfd_perform_relocation when linking native
> > objects, so any hacks here won't help.
> 
> I've extended the patch description accordingly. Thanks for pointing
> this out to me. Looks like to get ELF working this may then require
> in some cases a per-target fix. Afaics a change might be possible in
> a somewhat centralized manner in _bfd_elf_rela_local_sym(), but
> targets using _bfd_elf_rel_local_sym() would need dealing with
> individually. I've not looked into what it would take to deal with
> relocations targeting global symbols, both because for debug info
> this shouldn't matter and because I'm not after fixing ELF output
> anyway.
> 
> >>>> --- a/bfd/reloc.c
> >>>> +++ b/bfd/reloc.c
> >>>> @@ -749,6 +749,30 @@ bfd_perform_relocation (bfd *abfd,
> >>>>    else
> >>>>      output_base = reloc_target_output_section->vma;
> >>>>  
> >>>> +  /* Most architectures have no section relative ELF relocations.  They use
> >>>> +     ordinary ones instead for representing section relative references between
> >>>> +     debugging sections, which works fine as long as the section VMA gets set
> >>>> +     to zero.  While this is the default for ELF output (albeit not a
> >>>> +     requirement), in particular PE doesn't even allow zero VMAs for any of the
> >>>> +     sections.  */
> >>>> +  if(output_base && !howto->pc_relative
> >>>> +     && bfd_get_flavour (abfd) == bfd_target_elf_flavour
> >>>> +     && (reloc_target_output_section->flags
> >>>> +	 & input_section->flags & SEC_DEBUGGING))
> >>>> +    {
> >>>> +      /* Since this is a heuristic, apply further checks in an attempt to
> >>>> +	 exclude relocation types other than simple base ones.  */
> >>>> +      unsigned int size = bfd_get_reloc_size (howto);
> >>>> +
> >>>> +      if (size && !(size & (size - 1))
> >>>> +          && !(howto->bitsize & (howto->bitsize - 1))
> >>>> +          && !howto->bitpos && !howto->rightshift
> >>>> +          && !howto->negate && !howto->partial_inplace
> >>>> +          && !(howto->src_mask & (howto->src_mask + 1))
> >>>> +          && !(howto->dst_mask & (howto->dst_mask + 1)))
> >>>> +	output_base = 0;
> >>>> +    }
> >>>> +
> >>>>    output_base += symbol->section->output_offset;
> >>>>  
> >>>>    /* If symbol addresses are in octets, convert to bytes.  */
> >>>
> >>> When we need this sort of horrible hack, it's time to redesign.
> >>
> >> Well, I was fearing that, but I have to admit I have no idea what
> >> would need doing where. Surely the "horrible" aspects could be
> >> reduced by limiting the number of extra checks - as said in
> >> comment and description, I've added them to reduce risk of
> >> mistakenly zapping output_base. But I definitely agree that no
> >> matter how much massaging would be done, it's probably going to
> >> remain ugly without finding an entirely different approach.
> > 
> > Yes, it's true all the extra conditions don't inspire confidence.  If
> > it's OK to do at all then it should be OK with just
> > 
> >   if (output_bfd == NULL
> >       && bfd_get_flavour (abfd) == bfd_target_elf_flavour
> >       && (input_section->flags & SEC_DEBUGGING) != 0
> >       && (symbol->section->flags & SEC_DEBUGGING) != 0)
> >     output_base = 0;
> > 
> > with a comment saying why, of course.
> 
> While, as said, I agree the extra conditions don't look reassuring,
> dropping them adds to what we imply towards relocations between
> debugging sections. I understand you're okay with that (as far as
> "okay" can go with a hack like this)?
> 
> With the code snippet above I observe a few more modifications
> than just to drop the extra conditions. You use symbol->section
> when I used reloc_target_output_section. Is that for a reason?
> 
> You've also split my
> 
>      && (reloc_target_output_section->flags
> 	 & input_section->flags & SEC_DEBUGGING))
> 
> which was probably just a style change (including the addition of
> () != 0 around them). While I don't really see the reason for
> this transformation, if that's what binutils style requires I can
> surely change.

Yes, I deliberatey changed the test of what is effectively
  symbol->section->output_section->flags
to
  symbol->section->flags

You care about what is in the input section, not the output section.
The output section may even be .data, for example.

(As far as the style issue goes, I wasn't unhappy with the way you
wrote the expression.  The change was mostly to avoid a line-wrap in a
place you wouldn't normally wrap if allowed very long lines.)

> However, meanwhile I've realized that
> input_section's flags shouldn't really matter: If there indeed
> was a relocation in a non-debugging section targeting a debugging
> one, I would suppose output_base should be similarly ignored.
> Thoughts? (Such relocations don't seem to make a lot of sense, so
> perhaps this aspect is largely academical anyway.)

Yes, it likely is academic, but you put forward the idea of loaded
DWARF data accessed by an application.  In that situation you want
relocations from non-DWARF to DWARF to behave normally: The DWARF is
just a chunk of data.

> >  I can't say I like it though,
> > and I'm inclined to say the right approach is to fix the bad DWARF.
> 
> I wonder what you mean by "fix the bad Dwarf": It's the lack of
> suitable relocations in most targets' ELF ABIs which makes this
> hack necessary. Are you suggesting all targets supporting Dwarf
> should gain new section-relative relocation types?

It certainly would be nice if the correct relocations were available,
and used.  That would be taking a very hard line though.  I was
wondering if we can correct the DWARF relocations in the linker,
before we get to bfd_perform_relocation.  None of the ideas I had work
very well though, for example trying to do something in
elf_slurp_reloc_table_from_section to set a howto flag asking for
section-relative values does minimize changes in
bfd_perform_relocation, but you'd need all the howtos to be writable
or be switching howtos in elf_slurp_reloc_table_from_section.
Besides, adding a flag requires lots of editing for little real
benefit over accepting another hack in bfd_perform_relocation.


> I ask because - as indicated before - I'd be more than happy to
> use a less hacky approach. Going the new-reloc-types route would
> be a rather long process though, whereas the desire to not need
> to discard debug info when linking EFI executables is a present
> one.
> 
> Jan

Does this work for you?

	* elf.c (bfd_elf_generic_reloc): Make references between debug
	sections use section relative values.

diff --git a/bfd/elf.c b/bfd/elf.c
index 553fa65a11..5331a7064a 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -1302,8 +1302,7 @@ const char *const bfd_elf_section_type_names[] =
    change anything about the way the reloc is handled, since it will
    all be done at final link time.  Rather than put special case code
    into bfd_perform_relocation, all the reloc types use this howto
-   function.  It just short circuits the reloc if producing
-   relocatable output against an external symbol.  */
+   function, or should call this function for relocatable output.  */
 
 bfd_reloc_status_type
 bfd_elf_generic_reloc (bfd *abfd ATTRIBUTE_UNUSED,
@@ -1323,6 +1322,19 @@ bfd_elf_generic_reloc (bfd *abfd ATTRIBUTE_UNUSED,
       return bfd_reloc_ok;
     }
 
+  /* In some cases the relocation should be treated as input section
+     relative, as when linking ELF DWARF into PE COFF.  Many ELF
+     targets lack section relative relocations and instead use
+     ordinary absolute relocations for references between DWARF
+     sections.  That is arguably a bug in those targets but it happens
+     to work for the usual case of linking to non-loaded ELF debug
+     sections with VMAs forced to zero.  PE COFF on the other hand
+     doesn't allow a section VMA of zero.  */
+  if (output_bfd == NULL
+      && (symbol->section->flags & SEC_DEBUGGING) != 0
+      && (input_section->flags & SEC_DEBUGGING) != 0)
+    reloc_entry->addend -= symbol->section->output_section->vma;
+
   return bfd_reloc_continue;
 }
 \f


-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 3/6] bfd: refine handling of relocations between debugging sections

[Jan Beulich]

On 09.03.2021 03:24, Alan Modra wrote:
> On Mon, Mar 08, 2021 at 03:17:54PM +0100, Jan Beulich wrote:
>> On 05.03.2021 13:00, Alan Modra wrote:
>>> Yes, it's true all the extra conditions don't inspire confidence.  If
>>> it's OK to do at all then it should be OK with just
>>>
>>>   if (output_bfd == NULL
>>>       && bfd_get_flavour (abfd) == bfd_target_elf_flavour
>>>       && (input_section->flags & SEC_DEBUGGING) != 0
>>>       && (symbol->section->flags & SEC_DEBUGGING) != 0)
>>>     output_base = 0;
>>>
>>> with a comment saying why, of course.
>>
>> While, as said, I agree the extra conditions don't look reassuring,
>> dropping them adds to what we imply towards relocations between
>> debugging sections. I understand you're okay with that (as far as
>> "okay" can go with a hack like this)?
>>
>> With the code snippet above I observe a few more modifications
>> than just to drop the extra conditions. You use symbol->section
>> when I used reloc_target_output_section. Is that for a reason?
>>
>> You've also split my
>>
>>      && (reloc_target_output_section->flags
>> 	 & input_section->flags & SEC_DEBUGGING))
>>
>> which was probably just a style change (including the addition of
>> () != 0 around them). While I don't really see the reason for
>> this transformation, if that's what binutils style requires I can
>> surely change.
> 
> Yes, I deliberatey changed the test of what is effectively
>   symbol->section->output_section->flags
> to
>   symbol->section->flags
> 
> You care about what is in the input section, not the output section.
> The output section may even be .data, for example.

Oh, yes, good point.

> (As far as the style issue goes, I wasn't unhappy with the way you
> wrote the expression.  The change was mostly to avoid a line-wrap in a
> place you wouldn't normally wrap if allowed very long lines.)

I see. With the change to use symbol->section the line wouldn't
need wrapping anymore, though.

>> However, meanwhile I've realized that
>> input_section's flags shouldn't really matter: If there indeed
>> was a relocation in a non-debugging section targeting a debugging
>> one, I would suppose output_base should be similarly ignored.
>> Thoughts? (Such relocations don't seem to make a lot of sense, so
>> perhaps this aspect is largely academical anyway.)
> 
> Yes, it likely is academic, but you put forward the idea of loaded
> DWARF data accessed by an application.  In that situation you want
> relocations from non-DWARF to DWARF to behave normally: The DWARF is
> just a chunk of data.

I guess this can really be viewed both ways: When relocations
between debugging sections are specified to be section relative,
relocations targeting a debugging sections may also be assumed to
be so - consuming code needs to add in section base addresses
anyway. But yes, perhaps checking both sides produces the more
"natural" result.

> Does this work for you?

I'll give this a try, perhaps later today. However, ...

> @@ -1323,6 +1322,19 @@ bfd_elf_generic_reloc (bfd *abfd ATTRIBUTE_UNUSED,
>        return bfd_reloc_ok;
>      }
>  
> +  /* In some cases the relocation should be treated as input section
> +     relative, as when linking ELF DWARF into PE COFF.  Many ELF
> +     targets lack section relative relocations and instead use
> +     ordinary absolute relocations for references between DWARF
> +     sections.  That is arguably a bug in those targets but it happens
> +     to work for the usual case of linking to non-loaded ELF debug
> +     sections with VMAs forced to zero.  PE COFF on the other hand
> +     doesn't allow a section VMA of zero.  */
> +  if (output_bfd == NULL
> +      && (symbol->section->flags & SEC_DEBUGGING) != 0
> +      && (input_section->flags & SEC_DEBUGGING) != 0)
> +    reloc_entry->addend -= symbol->section->output_section->vma;

... already in your reduced replacement suggestion to my change
to bfd_perform_relocation() you didn't only drop the "just to
be on the safe side" checks, but also the pc-relative one. Are
you sure there aren't any cases where such relocations might be
used, particularly when the relocation points back to the same
section?

Jan

Re: [PATCH 3/6] bfd: refine handling of relocations between debugging sections

[Alan Modra]

On Tue, Mar 09, 2021 at 08:46:08AM +0100, Jan Beulich wrote:
> On 09.03.2021 03:24, Alan Modra wrote:
> > @@ -1323,6 +1322,19 @@ bfd_elf_generic_reloc (bfd *abfd ATTRIBUTE_UNUSED,
> >        return bfd_reloc_ok;
> >      }
> >  
> > +  /* In some cases the relocation should be treated as input section
> > +     relative, as when linking ELF DWARF into PE COFF.  Many ELF
> > +     targets lack section relative relocations and instead use
> > +     ordinary absolute relocations for references between DWARF
> > +     sections.  That is arguably a bug in those targets but it happens
> > +     to work for the usual case of linking to non-loaded ELF debug
> > +     sections with VMAs forced to zero.  PE COFF on the other hand
> > +     doesn't allow a section VMA of zero.  */
> > +  if (output_bfd == NULL
> > +      && (symbol->section->flags & SEC_DEBUGGING) != 0
> > +      && (input_section->flags & SEC_DEBUGGING) != 0)
> > +    reloc_entry->addend -= symbol->section->output_section->vma;
> 
> ... already in your reduced replacement suggestion to my change
> to bfd_perform_relocation() you didn't only drop the "just to
> be on the safe side" checks, but also the pc-relative one. Are
> you sure there aren't any cases where such relocations might be
> used, particularly when the relocation points back to the same
> section?

I'm aware of DW_OP_skip, DW_OP_bra, DW_OP_call[24] that use relative
offsets to the same section.  I figured these ought to resolve at
assembly time and thus not have relocations.  But I suppose some
targets might emit needless relocs, so yes, I'll put the pc-relative
test back.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 3/6] bfd: refine handling of relocations between debugging sections

[Jan Beulich]

On 09.03.2021 03:24, Alan Modra wrote:
> Does this work for you?
> 
> 	* elf.c (bfd_elf_generic_reloc): Make references between debug
> 	sections use section relative values.
> 
> diff --git a/bfd/elf.c b/bfd/elf.c
> index 553fa65a11..5331a7064a 100644
> --- a/bfd/elf.c
> +++ b/bfd/elf.c
> @@ -1302,8 +1302,7 @@ const char *const bfd_elf_section_type_names[] =
>     change anything about the way the reloc is handled, since it will
>     all be done at final link time.  Rather than put special case code
>     into bfd_perform_relocation, all the reloc types use this howto
> -   function.  It just short circuits the reloc if producing
> -   relocatable output against an external symbol.  */
> +   function, or should call this function for relocatable output.  */
>  
>  bfd_reloc_status_type
>  bfd_elf_generic_reloc (bfd *abfd ATTRIBUTE_UNUSED,
> @@ -1323,6 +1322,19 @@ bfd_elf_generic_reloc (bfd *abfd ATTRIBUTE_UNUSED,
>        return bfd_reloc_ok;
>      }
>  
> +  /* In some cases the relocation should be treated as input section
> +     relative, as when linking ELF DWARF into PE COFF.  Many ELF
> +     targets lack section relative relocations and instead use
> +     ordinary absolute relocations for references between DWARF
> +     sections.  That is arguably a bug in those targets but it happens
> +     to work for the usual case of linking to non-loaded ELF debug
> +     sections with VMAs forced to zero.  PE COFF on the other hand
> +     doesn't allow a section VMA of zero.  */
> +  if (output_bfd == NULL
> +      && (symbol->section->flags & SEC_DEBUGGING) != 0
> +      && (input_section->flags & SEC_DEBUGGING) != 0)
> +    reloc_entry->addend -= symbol->section->output_section->vma;
> +
>    return bfd_reloc_continue;
>  }
>  \f

Looks to be working fine. Just a question on the comment text:
Don't you mean "output section relative" in the first sentence?

Jan