[PATCH] x86: Propery check PC16 reloc overflow in 16-bit mode instructions

public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed

* [PATCH] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
@ 2021-05-24 23:42 H.J. Lu
  2021-05-25  6:40 ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-05-24 23:42 UTC (permalink / raw)
  To: binutils

commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
Author: Jan Beulich <jbeulich@suse.com>
Date:   Mon Apr 26 10:41:35 2021 +0200

    x86: correct overflow checking for 16-bit PC-relative relocs

caused linker failure when building 16-bit program in a 32-bit ELF
container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with

 #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)

as in

https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA

to indicate that 16-bit mode instructions are used in the object to
allow linker to properly perform relocation overflow check for 16-bit
PC-relative relocations in 16-bit mode instructions.

1. Update x86 assembler to always generate the GNU property note with
GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.

bfd/

	PR ld/27905
	* elf32-i386.c: Include "libiberty.h".
	(elf_howto_table): Add 16-bit R_386_PC16 entry.
	(elf_i386_rtype_to_howto): Add a BFD argument.  Use 16-bit
	R_386_PC16 if input has 16-bit mode instructions.
	(elf_i386_info_to_howto_rel): Update elf_i386_rtype_to_howto
	call.
	(elf_i386_tls_transition): Likewise.
	(elf_i386_relocate_section): Likewise.
	* elf64-x86-64.c (x86_64_elf_howto_table): Add 16-bit
	R_X86_64_PC16 entry.
	(elf_x86_64_rtype_to_howto): Use 16-bit R_X86_64_PC16 if input
	has 16-bit mode instructions.
	* elfxx-x86.c (_bfd_x86_elf_parse_gnu_properties): Set
	elf_x86_has_code16 if relocatable input is marked with
	GNU_PROPERTY_X86_FEATURE_2_CODE16.
	* elfxx-x86.h (elf_x86_obj_tdata): Add has_code16.
	(elf_x86_has_code16): New.

binutils/

	PR ld/27905
	* readelf.c (decode_x86_feature_2): Support
	GNU_PROPERTY_X86_FEATURE_2_CODE16.

gas/

	PR ld/27905
	* config/tc-i386.c (set_code_flag): Update x86_feature_2_used
	with GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF
	object.
	(set_16bit_gcc_code_flag): Likewise.
	(x86_cleanup): Always generate the GNU property note if
	x86_feature_2_used isn't 0.
	* testsuite/gas/i386/code16-2.d: New file.
	* testsuite/gas/i386/code16-2.s: Likewise.
	* testsuite/gas/i386/x86-64-code16-2.d: Likewise.
	* testsuite/gas/i386/i386.exp: Run code16-2 and x86-64-code16-2.

include/

	PR ld/27905
	* elf/common.h (GNU_PROPERTY_X86_FEATURE_2_CODE16): New.

ld/

	PR ld/27905
	* testsuite/ld-i386/code16.d: New file.
	* testsuite/ld-i386/code16.t: Likewise.
	* testsuite/ld-x86-64/code16.d: Likewise.
	* testsuite/ld-x86-64/code16.t: Likewise.
	* testsuite/ld-i386/i386.exp: Run code16.
	* testsuite/ld-x86-64/x86-64.exp: Likewise.
---
 bfd/elf32-i386.c                         | 23 ++++++++---
 bfd/elf64-x86-64.c                       | 12 ++++++
 bfd/elfxx-x86.c                          |  9 ++++-
 bfd/elfxx-x86.h                          |  6 +++
 binutils/readelf.c                       |  3 ++
 gas/config/tc-i386.c                     | 50 +++++++++++++++---------
 gas/testsuite/gas/i386/code16-2.d        |  8 ++++
 gas/testsuite/gas/i386/code16-2.s        | 10 +++++
 gas/testsuite/gas/i386/i386.exp          |  2 +
 gas/testsuite/gas/i386/x86-64-code16-2.d |  9 +++++
 include/elf/common.h                     |  1 +
 ld/testsuite/ld-i386/code16.d            | 19 +++++++++
 ld/testsuite/ld-i386/code16.t            |  7 ++++
 ld/testsuite/ld-i386/i386.exp            |  1 +
 ld/testsuite/ld-x86-64/code16.d          | 19 +++++++++
 ld/testsuite/ld-x86-64/code16.t          |  7 ++++
 ld/testsuite/ld-x86-64/x86-64.exp        |  1 +
 17 files changed, 162 insertions(+), 25 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/code16-2.d
 create mode 100644 gas/testsuite/gas/i386/code16-2.s
 create mode 100644 gas/testsuite/gas/i386/x86-64-code16-2.d
 create mode 100644 ld/testsuite/ld-i386/code16.d
 create mode 100644 ld/testsuite/ld-i386/code16.t
 create mode 100644 ld/testsuite/ld-x86-64/code16.d
 create mode 100644 ld/testsuite/ld-x86-64/code16.t

diff --git a/bfd/elf32-i386.c b/bfd/elf32-i386.c
index 4451f9b451c..c68741af02c 100644
--- a/bfd/elf32-i386.c
+++ b/bfd/elf32-i386.c
@@ -22,6 +22,7 @@
 #include "elf-vxworks.h"
 #include "dwarf2.h"
 #include "opcode/i386.h"
+#include "libiberty.h"
 
 /* 386 uses REL relocations instead of RELA.  */
 #define USE_REL	1
@@ -175,10 +176,14 @@ static reloc_howto_type elf_howto_table[]=
 	 false,			/* partial_inplace */
 	 0,			/* src_mask */
 	 0,			/* dst_mask */
-	 false)			/* pcrel_offset */
+	 false),		/* pcrel_offset */
 
 #define R_386_vt (R_386_GNU_VTENTRY + 1 - R_386_vt_offset)
 
+/* Use complain_overflow_bitfield on R_386_PC16 for code16.  */
+  HOWTO(R_386_PC16, 0, 1, 16, true, 0, complain_overflow_bitfield,
+	bfd_elf_generic_reloc, "R_386_PC16",
+	true, 0xffff, 0xffff, true)
 };
 
 #define X86_PCREL_TYPE_P(TYPE) ((TYPE) == R_386_PC32)
@@ -369,7 +374,7 @@ elf_i386_reloc_name_lookup (bfd *abfd ATTRIBUTE_UNUSED,
 }
 
 static reloc_howto_type *
-elf_i386_rtype_to_howto (unsigned r_type)
+elf_i386_rtype_to_howto (bfd *abfd, unsigned r_type)
 {
   unsigned int indx;
 
@@ -384,6 +389,11 @@ elf_i386_rtype_to_howto (unsigned r_type)
   /* PR 17512: file: 0f67f69d.  */
   if (elf_howto_table [indx].type != r_type)
     return NULL;
+
+  /* Use complain_overflow_bitfield on R_386_PC16 for code16.  */
+  if (r_type == (unsigned int) R_386_PC16 && elf_x86_has_code16 (abfd))
+    indx = ARRAY_SIZE (elf_howto_table) - 1;
+
   return &elf_howto_table[indx];
 }
 
@@ -394,7 +404,8 @@ elf_i386_info_to_howto_rel (bfd *abfd,
 {
   unsigned int r_type = ELF32_R_TYPE (dst->r_info);
 
-  if ((cache_ptr->howto = elf_i386_rtype_to_howto (r_type)) == NULL)
+  if ((cache_ptr->howto = elf_i386_rtype_to_howto (abfd, r_type))
+      == NULL)
     {
       /* xgettext:c-format */
       _bfd_error_handler (_("%pB: unsupported relocation type %#x"),
@@ -1142,8 +1153,8 @@ elf_i386_tls_transition (struct bfd_link_info *info, bfd *abfd,
       reloc_howto_type *from, *to;
       const char *name;
 
-      from = elf_i386_rtype_to_howto (from_type);
-      to = elf_i386_rtype_to_howto (to_type);
+      from = elf_i386_rtype_to_howto (abfd, from_type);
+      to = elf_i386_rtype_to_howto (abfd, to_type);
 
       if (h)
 	name = h->root.root.string;
@@ -2074,7 +2085,7 @@ elf_i386_relocate_section (bfd *output_bfd,
 	  continue;
 	}
 
-      howto = elf_i386_rtype_to_howto (r_type);
+      howto = elf_i386_rtype_to_howto (input_bfd, r_type);
       if (howto == NULL)
 	return _bfd_unrecognized_reloc (input_bfd, input_section, r_type);
 
diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
index d0c994e570a..b6fefc968c0 100644
--- a/bfd/elf64-x86-64.c
+++ b/bfd/elf64-x86-64.c
@@ -187,6 +187,10 @@ static reloc_howto_type x86_64_elf_howto_table[] =
 	 _bfd_elf_rel_vtable_reloc_fn, "R_X86_64_GNU_VTENTRY", false, 0, 0,
 	 false),
 
+/* Use complain_overflow_bitfield on R_X86_64_PC16 for code16.  */
+  HOWTO(R_X86_64_PC16, 0, 1, 16, true, 0, complain_overflow_bitfield,
+	bfd_elf_generic_reloc, "R_X86_64_PC16", false, 0, 0xffff, true),
+
 /* Use complain_overflow_bitfield on R_X86_64_32 for x32.  */
   HOWTO(R_X86_64_32, 0, 2, 32, false, 0, complain_overflow_bitfield,
 	bfd_elf_generic_reloc, "R_X86_64_32", false, 0, 0xffffffff,
@@ -270,6 +274,14 @@ elf_x86_64_rtype_to_howto (bfd *abfd, unsigned r_type)
       else
 	i = ARRAY_SIZE (x86_64_elf_howto_table) - 1;
     }
+  if (r_type == (unsigned int) R_X86_64_PC16)
+    {
+      /* Use complain_overflow_bitfield on R_X86_64_PC16 for code16.  */
+      if (elf_x86_has_code16 (abfd))
+	i = ARRAY_SIZE (x86_64_elf_howto_table) - 2;
+      else
+	i = r_type;
+    }
   else if (r_type < (unsigned int) R_X86_64_GNU_VTINHERIT
 	   || r_type >= (unsigned int) R_X86_64_max)
     {
diff --git a/bfd/elfxx-x86.c b/bfd/elfxx-x86.c
index 62d516aab8d..29dc7f04b4d 100644
--- a/bfd/elfxx-x86.c
+++ b/bfd/elfxx-x86.c
@@ -2353,6 +2353,7 @@ _bfd_x86_elf_parse_gnu_properties (bfd *abfd, unsigned int type,
       || (type >= GNU_PROPERTY_X86_UINT32_OR_AND_LO
 	  && type <= GNU_PROPERTY_X86_UINT32_OR_AND_HI))
     {
+      unsigned int number;
       if (datasz != 4)
 	{
 	  _bfd_error_handler
@@ -2361,7 +2362,13 @@ _bfd_x86_elf_parse_gnu_properties (bfd *abfd, unsigned int type,
 	  return property_corrupt;
 	}
       prop = _bfd_elf_get_property (abfd, type, datasz);
-      prop->u.number |= bfd_h_get_32 (abfd, ptr);
+      number = bfd_h_get_32 (abfd, ptr);
+      if ((abfd->flags
+	   & (DYNAMIC | BFD_LINKER_CREATED | BFD_PLUGIN)) == 0
+	  && type == GNU_PROPERTY_X86_FEATURE_2_USED
+	  && (number & GNU_PROPERTY_X86_FEATURE_2_CODE16) != 0)
+	elf_x86_has_code16 (abfd) = 1;
+      prop->u.number |= number;
       prop->pr_kind = property_number;
       return property_number;
     }
diff --git a/bfd/elfxx-x86.h b/bfd/elfxx-x86.h
index db11327e96f..e8344305492 100644
--- a/bfd/elfxx-x86.h
+++ b/bfd/elfxx-x86.h
@@ -549,6 +549,9 @@ struct elf_x86_obj_tdata
 
   /* GOTPLT entries for TLS descriptors.  */
   bfd_vma *local_tlsdesc_gotent;
+
+  /* Set if the objec file has 16-bit code.  */
+  unsigned int has_code16 : 1;
 };
 
 enum elf_x86_plt_type
@@ -584,6 +587,9 @@ struct elf_x86_plt
 #define elf_x86_local_tlsdesc_gotent(abfd) \
   (elf_x86_tdata (abfd)->local_tlsdesc_gotent)
 
+#define elf_x86_has_code16(abfd) \
+  (elf_x86_tdata (abfd)->has_code16)
+
 #define elf_x86_compute_jump_table_size(htab) \
   ((htab)->elf.srelplt->reloc_count * (htab)->got_entry_size)
 
diff --git a/binutils/readelf.c b/binutils/readelf.c
index 2989adf31b6..8f24cc4071b 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -19140,6 +19140,9 @@ decode_x86_feature_2 (unsigned int bitmask)
 	case GNU_PROPERTY_X86_FEATURE_2_XSAVEC:
 	  printf ("XSAVEC");
 	  break;
+	case GNU_PROPERTY_X86_FEATURE_2_CODE16:
+	  printf ("CODE16");
+	  break;
 	default:
 	  printf (_("<unknown: %x>"), bit);
 	  break;
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index d3441988e34..5a55d5478e6 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2695,6 +2695,10 @@ static void
 set_code_flag (int value)
 {
   update_code_flag (value, 0);
+#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
+  if (IS_ELF && value == CODE_16BIT)
+    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
+#endif
 }
 
 static void
@@ -2706,6 +2710,10 @@ set_16bit_gcc_code_flag (int new_code_flag)
   cpu_arch_flags.bitfield.cpu64 = 0;
   cpu_arch_flags.bitfield.cpuno64 = 1;
   stackop_size = LONG_MNEM_SUFFIX;
+#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
+  if (IS_ELF && new_code_flag == CODE_16BIT)
+    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
+#endif
 }
 
 static void
@@ -9032,7 +9040,7 @@ x86_cleanup (void)
   unsigned int isa_1_descsz_raw, feature_2_descsz_raw;
   unsigned int padding;
 
-  if (!IS_ELF || !x86_used_note)
+  if (!IS_ELF || (!x86_used_note && !x86_feature_2_used))
     return;
 
   x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_X86;
@@ -9072,15 +9080,18 @@ x86_cleanup (void)
   bfd_set_section_alignment (sec, alignment);
   elf_section_type (sec) = SHT_NOTE;
 
-  /* GNU_PROPERTY_X86_ISA_1_USED: 4-byte type + 4-byte data size
-				  + 4-byte data  */
-  isa_1_descsz_raw = 4 + 4 + 4;
-  /* Align GNU_PROPERTY_X86_ISA_1_USED.  */
-  isa_1_descsz = (isa_1_descsz_raw + align_size_1) & ~align_size_1;
+  if (x86_used_note)
+    {
+      /* GNU_PROPERTY_X86_ISA_1_USED: 4-byte type + 4-byte data size
+	 + 4-byte data  */
+      isa_1_descsz_raw = 4 + 4 + 4;
+      /* Align GNU_PROPERTY_X86_ISA_1_USED.  */
+      isa_1_descsz = (isa_1_descsz_raw + align_size_1) & ~align_size_1;
+    }
 
   feature_2_descsz_raw = isa_1_descsz;
   /* GNU_PROPERTY_X86_FEATURE_2_USED: 4-byte type + 4-byte data size
-				      + 4-byte data  */
+     + 4-byte data  */
   feature_2_descsz_raw += 4 + 4 + 4;
   /* Align GNU_PROPERTY_X86_FEATURE_2_USED.  */
   feature_2_descsz = ((feature_2_descsz_raw + align_size_1)
@@ -9102,20 +9113,23 @@ x86_cleanup (void)
   /* Write n_name.  */
   memcpy (p + 4 * 3, "GNU", 4);
 
-  /* Write 4-byte type.  */
-  md_number_to_chars (p + 4 * 4,
-		      (valueT) GNU_PROPERTY_X86_ISA_1_USED, 4);
+  if (x86_used_note)
+    {
+      /* Write 4-byte type.  */
+      md_number_to_chars (p + 4 * 4,
+			  (valueT) GNU_PROPERTY_X86_ISA_1_USED, 4);
 
-  /* Write 4-byte data size.  */
-  md_number_to_chars (p + 4 * 5, (valueT) 4, 4);
+      /* Write 4-byte data size.  */
+      md_number_to_chars (p + 4 * 5, (valueT) 4, 4);
 
-  /* Write 4-byte data.  */
-  md_number_to_chars (p + 4 * 6, (valueT) x86_isa_1_used, 4);
+      /* Write 4-byte data.  */
+      md_number_to_chars (p + 4 * 6, (valueT) x86_isa_1_used, 4);
 
-  /* Zero out paddings.  */
-  padding = isa_1_descsz - isa_1_descsz_raw;
-  if (padding)
-    memset (p + 4 * 7, 0, padding);
+      /* Zero out paddings.  */
+      padding = isa_1_descsz - isa_1_descsz_raw;
+      if (padding)
+	memset (p + 4 * 7, 0, padding);
+    }
 
   /* Write 4-byte type.  */
   md_number_to_chars (p + isa_1_descsz + 4 * 4,
diff --git a/gas/testsuite/gas/i386/code16-2.d b/gas/testsuite/gas/i386/code16-2.d
new file mode 100644
index 00000000000..fb8bb742ac1
--- /dev/null
+++ b/gas/testsuite/gas/i386/code16-2.d
@@ -0,0 +1,8 @@
+#name: i386 code16
+#as: -mx86-used-note=no --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 feature used: x86, CODE16
diff --git a/gas/testsuite/gas/i386/code16-2.s b/gas/testsuite/gas/i386/code16-2.s
new file mode 100644
index 00000000000..66e5d558791
--- /dev/null
+++ b/gas/testsuite/gas/i386/code16-2.s
@@ -0,0 +1,10 @@
+	.code16gcc
+	.text
+	.section	.text.default_process_op.isra.0,"ax",@progbits
+	.type	default_process_op.isra.0, @function
+default_process_op.isra.0:
+	ret
+	.section	.text.mpt_scsi_process_op,"ax",@progbits
+	.type	mpt_scsi_process_op, @function
+mpt_scsi_process_op:
+	jmp	default_process_op.isra.0
diff --git a/gas/testsuite/gas/i386/i386.exp b/gas/testsuite/gas/i386/i386.exp
index 39010bdd500..a459c6fe392 100644
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -734,6 +734,7 @@ if {[is_elf_format] || [istarget "*-*-vxworks*"]} then {
     run_dump_test "property-ldmxcsr"
     run_dump_test "property-vldmxcsr"
     run_dump_test "property-vzeroall"
+    run_dump_test "code16-2"
 
     if {![istarget "*-*-dragonfly*"]
 	&& ![istarget "*-*-gnu*"]
@@ -1298,6 +1299,7 @@ if [gas_64_check] then {
 	run_dump_test "x86-64-property-8"
 	run_dump_test "x86-64-property-9"
 	run_dump_test "x86-64-property-14"
+	run_dump_test "x86-64-code16-2"
 
 	if {[istarget "*-*-linux*"]} then {
 	    run_dump_test "x86-64-align-branch-3"
diff --git a/gas/testsuite/gas/i386/x86-64-code16-2.d b/gas/testsuite/gas/i386/x86-64-code16-2.d
new file mode 100644
index 00000000000..3c2d660cd05
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-code16-2.d
@@ -0,0 +1,9 @@
+#source: code16-2.s
+#name: x86-64 code16
+#as: -mx86-used-note=no --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 feature used: x86, CODE16
diff --git a/include/elf/common.h b/include/elf/common.h
index 24d0a09b7c8..564ab711a20 100644
--- a/include/elf/common.h
+++ b/include/elf/common.h
@@ -872,6 +872,7 @@
 #define GNU_PROPERTY_X86_FEATURE_2_XSAVEC	(1U << 9)
 #define GNU_PROPERTY_X86_FEATURE_2_TMM		(1U << 10)
 #define GNU_PROPERTY_X86_FEATURE_2_MASK		(1U << 11)
+#define GNU_PROPERTY_X86_FEATURE_2_CODE16	(1U << 12)
 
 #define GNU_PROPERTY_X86_COMPAT_2_ISA_1_NEEDED \
   (GNU_PROPERTY_X86_UINT32_OR_LO + 0)
diff --git a/ld/testsuite/ld-i386/code16.d b/ld/testsuite/ld-i386/code16.d
new file mode 100644
index 00000000000..8b67861db91
--- /dev/null
+++ b/ld/testsuite/ld-i386/code16.d
@@ -0,0 +1,19 @@
+#name: i386 R_386_PC16 reloc in 16-bit mode
+#as: --32 -mx86-used-note=no --generate-missing-build-notes=no
+#source: ${srcdir}/../../../gas/testsuite/gas/i386/code16-2.s
+#ld: -T code16.t
+#objdump: -dw -Mi8086
+
+.*: +file format .*
+
+
+Disassembly of section .text.default_process_op.isra.0:
+
+0+737c <default_process_op.isra.0>:
+ +[a-f0-9]+:	66 c3                	retl   
+
+Disassembly of section .text.mpt_scsi_process_op:
+
+0+f869 <mpt_scsi_process_op>:
+ +[a-f0-9]+:	e9 10 7b             	jmp    737c <default_process_op.isra.0>
+#pass
diff --git a/ld/testsuite/ld-i386/code16.t b/ld/testsuite/ld-i386/code16.t
new file mode 100644
index 00000000000..0cf99042e40
--- /dev/null
+++ b/ld/testsuite/ld-i386/code16.t
@@ -0,0 +1,7 @@
+OUTPUT_FORMAT("elf32-i386")
+OUTPUT_ARCH("i386")
+SECTIONS
+{
+.text.default_process_op.isra.0 0x737c : { *(.text.default_process_op.isra.0) }
+.text.mpt_scsi_process_op 0xf869 : { *(.text.mpt_scsi_process_op) }
+}
diff --git a/ld/testsuite/ld-i386/i386.exp b/ld/testsuite/ld-i386/i386.exp
index d0b3f69fb8d..3d6047b790d 100644
--- a/ld/testsuite/ld-i386/i386.exp
+++ b/ld/testsuite/ld-i386/i386.exp
@@ -492,6 +492,7 @@ run_dump_test "property-x86-isa2"
 run_dump_test "property-x86-isa3"
 run_dump_test "property-x86-isa4"
 run_dump_test "pr26869"
+run_dump_test "code16"
 
 if { !([istarget "i?86-*-linux*"]
        || [istarget "i?86-*-gnu*"]
diff --git a/ld/testsuite/ld-x86-64/code16.d b/ld/testsuite/ld-x86-64/code16.d
new file mode 100644
index 00000000000..20096ab6abf
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/code16.d
@@ -0,0 +1,19 @@
+#name: x86-64 R_X86_64_PC16 reloc in 16-bit mode
+#as: --64 -mx86-used-note=no --generate-missing-build-notes=no
+#source: ${srcdir}/../../../gas/testsuite/gas/i386/code16-2.s
+#ld: -T code16.t
+#objdump: -dw -Mi8086
+
+.*: +file format .*
+
+
+Disassembly of section .text.default_process_op.isra.0:
+
+0+737c <default_process_op.isra.0>:
+ +[a-f0-9]+:	66 c3                	retl   
+
+Disassembly of section .text.mpt_scsi_process_op:
+
+0+f869 <mpt_scsi_process_op>:
+ +[a-f0-9]+:	e9 10 7b             	jmp    737c <default_process_op.isra.0>
+#pass
diff --git a/ld/testsuite/ld-x86-64/code16.t b/ld/testsuite/ld-x86-64/code16.t
new file mode 100644
index 00000000000..9ef00a3404d
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/code16.t
@@ -0,0 +1,7 @@
+OUTPUT_FORMAT("elf64-x86-64")
+OUTPUT_ARCH("i386:x86-64")
+SECTIONS
+{
+.text.default_process_op.isra.0 0x737c : { *(.text.default_process_op.isra.0) }
+.text.mpt_scsi_process_op 0xf869 : { *(.text.mpt_scsi_process_op) }
+}
diff --git a/ld/testsuite/ld-x86-64/x86-64.exp b/ld/testsuite/ld-x86-64/x86-64.exp
index 17fd10ee121..b71059afd5b 100644
--- a/ld/testsuite/ld-x86-64/x86-64.exp
+++ b/ld/testsuite/ld-x86-64/x86-64.exp
@@ -474,6 +474,7 @@ run_dump_test "property-x86-isa3"
 run_dump_test "property-x86-isa3-x32"
 run_dump_test "property-x86-isa4"
 run_dump_test "property-x86-isa4-x32"
+run_dump_test "code16"
 
 if ![istarget "x86_64-*-linux*"] {
     return
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-24 23:42 [PATCH] x86: Propery check PC16 reloc overflow in 16-bit mode instructions H.J. Lu
@ 2021-05-25  6:40 ` Jan Beulich
  2021-05-25 13:41   ` [PATCH v2] " H.J. Lu
  0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2021-05-25  6:40 UTC (permalink / raw)
  To: H.J. Lu; +Cc: binutils, Martin Liška

On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> Author: Jan Beulich <jbeulich@suse.com>
> Date:   Mon Apr 26 10:41:35 2021 +0200
> 
>     x86: correct overflow checking for 16-bit PC-relative relocs
> 
> caused linker failure when building 16-bit program in a 32-bit ELF
> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> 
>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> 
> as in
> 
> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> 
> to indicate that 16-bit mode instructions are used in the object to
> allow linker to properly perform relocation overflow check for 16-bit
> PC-relative relocations in 16-bit mode instructions.

I certainly don't mind the introduction of this flag; I think its
introduction wants to be separated from the specific use in the
linker, not the lease because ...

> 1. Update x86 assembler to always generate the GNU property note with
> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.

... I don't think this is an appropriate step to take. The majority
of cases of 16-bit code use that I know of is in projects where this
is just a small portion of code, and the rest of the code is 32-
and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
as indication to relax overflow checking, you undermine the main
goal of the earlier change.

Ideally we'd have annotations indicating the boundaries of 16-bit
code, e.g. along the lines of Arm's Arm/Thumb/data markers. An
intermediate workaround (which I still wouldn't like, but I
thought I'd mention it) would be to have a flag indicating use of
the problematic relocation _in 16-bit code_. (I don't like this
because it's rather likely for the flag to get set, as an %ip-
relative JMP or CALL is pretty likely to be present in such code,
and hence the original purpose of the earlier change would still
be undermined.)

I can't easily tell whether the SeaBIOS code triggering the issue
Martin has reported is _pure_ 16-bit code. If it is, for this
sub-case an almost-clean solution would be possible: Mark objects
containing _only_ 16-bit code as such, and AND together the
individual flags when linking. This is only "almost-clean" since
data sections could still be meant for 32-bit consumption. But I
view chances of such actually happening as relatively slim.

On the whole I view your suggestion (in the bugzilla entry) to
extend -z noreloc-overflow as the more viable one, at least for
now. This would express a developer's explicit consent to be
responsible themselves for avoiding any overflows.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-25  6:40 ` Jan Beulich
@ 2021-05-25 13:41   ` H.J. Lu
  2021-05-25 15:34     ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-05-25 13:41 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils, Martin Liška

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

On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> > commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> > Author: Jan Beulich <jbeulich@suse.com>
> > Date:   Mon Apr 26 10:41:35 2021 +0200
> >
> >     x86: correct overflow checking for 16-bit PC-relative relocs
> >
> > caused linker failure when building 16-bit program in a 32-bit ELF
> > container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> >
> >  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> >
> > as in
> >
> > https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> >
> > to indicate that 16-bit mode instructions are used in the object to
> > allow linker to properly perform relocation overflow check for 16-bit
> > PC-relative relocations in 16-bit mode instructions.
>
> I certainly don't mind the introduction of this flag; I think its
> introduction wants to be separated from the specific use in the
> linker, not the lease because ...
>
> > 1. Update x86 assembler to always generate the GNU property note with
> > GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> > 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> > input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
>
> ... I don't think this is an appropriate step to take. The majority
> of cases of 16-bit code use that I know of is in projects where this
> is just a small portion of code, and the rest of the code is 32-
> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
> as indication to relax overflow checking, you undermine the main
> goal of the earlier change.

commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
Author: Jan Beulich <jbeulich@suse.com>
Date:   Mon Apr 26 10:41:35 2021 +0200

    x86: correct overflow checking for 16-bit PC-relative relocs

is technically correct according to psABIs.  But GNU assembler
only generates PC16 relocations for 16-bit codes.  That is why
we never ran into any PC16 relocation overflow before.  My change
restores the old behavior only when input has 16-bit codes.  I think it
is a good compromise between usability and error checking.

> Ideally we'd have annotations indicating the boundaries of 16-bit
> code, e.g. along the lines of Arm's Arm/Thumb/data markers. An
> intermediate workaround (which I still wouldn't like, but I
> thought I'd mention it) would be to have a flag indicating use of
> the problematic relocation _in 16-bit code_. (I don't like this
> because it's rather likely for the flag to get set, as an %ip-
> relative JMP or CALL is pretty likely to be present in such code,
> and hence the original purpose of the earlier change would still
> be undermined.)
>
> I can't easily tell whether the SeaBIOS code triggering the issue
> Martin has reported is _pure_ 16-bit code. If it is, for this
> sub-case an almost-clean solution would be possible: Mark objects
> containing _only_ 16-bit code as such, and AND together the
> individual flags when linking. This is only "almost-clean" since
> data sections could still be meant for 32-bit consumption. But I
> view chances of such actually happening as relatively slim.
>
> On the whole I view your suggestion (in the bugzilla entry) to
> extend -z noreloc-overflow as the more viable one, at least for
> now. This would express a developer's explicit consent to be
> responsible themselves for avoiding any overflows.

-z noreloc-overflow isn't transparent.  We shouldn't error PC16
relocation overflow in 16-bit codes when there are none.

Here is the v2 patch to fix compilation errors.

-- 
H.J.

[-- Attachment #2: v2-0001-x86-Propery-check-PC16-reloc-overflow-in-16-bit-m.patch --]
[-- Type: text/x-patch, Size: 19297 bytes --]

From dd9abfa87f82dde7bc03560af531b8eb37314e59 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 24 May 2021 16:11:04 -0700
Subject: [PATCH v2] x86: Propery check PC16 reloc overflow in 16-bit mode
 instructions

commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
Author: Jan Beulich <jbeulich@suse.com>
Date:   Mon Apr 26 10:41:35 2021 +0200

    x86: correct overflow checking for 16-bit PC-relative relocs

caused linker failure when building 16-bit program in a 32-bit ELF
container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with

 #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)

to indicate that 16-bit mode instructions are used in the input object:

https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA

to indicate that 16-bit mode instructions are used in the object to
allow linker to properly perform relocation overflow check for 16-bit
PC-relative relocations in 16-bit mode instructions.

1. Update x86 assembler to always generate the GNU property note with
GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.

bfd/

	PR ld/27905
	* elf32-i386.c: Include "libiberty.h".
	(elf_howto_table): Add 16-bit R_386_PC16 entry.
	(elf_i386_rtype_to_howto): Add a BFD argument.  Use 16-bit
	R_386_PC16 if input has 16-bit mode instructions.
	(elf_i386_info_to_howto_rel): Update elf_i386_rtype_to_howto
	call.
	(elf_i386_tls_transition): Likewise.
	(elf_i386_relocate_section): Likewise.
	* elf64-x86-64.c (x86_64_elf_howto_table): Add 16-bit
	R_X86_64_PC16 entry.
	(elf_x86_64_rtype_to_howto): Use 16-bit R_X86_64_PC16 if input
	has 16-bit mode instructions.
	* elfxx-x86.c (_bfd_x86_elf_parse_gnu_properties): Set
	elf_x86_has_code16 if relocatable input is marked with
	GNU_PROPERTY_X86_FEATURE_2_CODE16.
	* elfxx-x86.h (elf_x86_obj_tdata): Add has_code16.
	(elf_x86_has_code16): New.

binutils/

	PR ld/27905
	* readelf.c (decode_x86_feature_2): Support
	GNU_PROPERTY_X86_FEATURE_2_CODE16.

gas/

	PR ld/27905
	* config/tc-i386.c (set_code_flag): Update x86_feature_2_used
	with GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF
	object.
	(set_16bit_gcc_code_flag): Likewise.
	(x86_cleanup): Always generate the GNU property note if
	x86_feature_2_used isn't 0.
	* testsuite/gas/i386/code16-2.d: New file.
	* testsuite/gas/i386/code16-2.s: Likewise.
	* testsuite/gas/i386/x86-64-code16-2.d: Likewise.
	* testsuite/gas/i386/i386.exp: Run code16-2 and x86-64-code16-2.

include/

	PR ld/27905
	* elf/common.h (GNU_PROPERTY_X86_FEATURE_2_CODE16): New.

ld/

	PR ld/27905
	* testsuite/ld-i386/code16.d: New file.
	* testsuite/ld-i386/code16.t: Likewise.
	* testsuite/ld-x86-64/code16.d: Likewise.
	* testsuite/ld-x86-64/code16.t: Likewise.
	* testsuite/ld-i386/i386.exp: Run code16.
	* testsuite/ld-x86-64/x86-64.exp: Likewise.
---
 bfd/elf32-i386.c                         | 23 +++++++---
 bfd/elf64-x86-64.c                       | 12 ++++++
 bfd/elfxx-x86.c                          |  9 +++-
 bfd/elfxx-x86.h                          |  6 +++
 binutils/readelf.c                       |  3 ++
 gas/config/tc-i386.c                     | 55 ++++++++++++++++--------
 gas/testsuite/gas/i386/code16-2.d        |  8 ++++
 gas/testsuite/gas/i386/code16-2.s        | 10 +++++
 gas/testsuite/gas/i386/i386.exp          |  2 +
 gas/testsuite/gas/i386/x86-64-code16-2.d |  9 ++++
 include/elf/common.h                     |  1 +
 ld/testsuite/ld-i386/code16.d            | 19 ++++++++
 ld/testsuite/ld-i386/code16.t            |  7 +++
 ld/testsuite/ld-i386/i386.exp            |  1 +
 ld/testsuite/ld-x86-64/code16.d          | 19 ++++++++
 ld/testsuite/ld-x86-64/code16.t          |  7 +++
 ld/testsuite/ld-x86-64/x86-64.exp        |  1 +
 17 files changed, 167 insertions(+), 25 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/code16-2.d
 create mode 100644 gas/testsuite/gas/i386/code16-2.s
 create mode 100644 gas/testsuite/gas/i386/x86-64-code16-2.d
 create mode 100644 ld/testsuite/ld-i386/code16.d
 create mode 100644 ld/testsuite/ld-i386/code16.t
 create mode 100644 ld/testsuite/ld-x86-64/code16.d
 create mode 100644 ld/testsuite/ld-x86-64/code16.t

diff --git a/bfd/elf32-i386.c b/bfd/elf32-i386.c
index 4451f9b451c..c68741af02c 100644
--- a/bfd/elf32-i386.c
+++ b/bfd/elf32-i386.c
@@ -22,6 +22,7 @@
 #include "elf-vxworks.h"
 #include "dwarf2.h"
 #include "opcode/i386.h"
+#include "libiberty.h"
 
 /* 386 uses REL relocations instead of RELA.  */
 #define USE_REL	1
@@ -175,10 +176,14 @@ static reloc_howto_type elf_howto_table[]=
 	 false,			/* partial_inplace */
 	 0,			/* src_mask */
 	 0,			/* dst_mask */
-	 false)			/* pcrel_offset */
+	 false),		/* pcrel_offset */
 
 #define R_386_vt (R_386_GNU_VTENTRY + 1 - R_386_vt_offset)
 
+/* Use complain_overflow_bitfield on R_386_PC16 for code16.  */
+  HOWTO(R_386_PC16, 0, 1, 16, true, 0, complain_overflow_bitfield,
+	bfd_elf_generic_reloc, "R_386_PC16",
+	true, 0xffff, 0xffff, true)
 };
 
 #define X86_PCREL_TYPE_P(TYPE) ((TYPE) == R_386_PC32)
@@ -369,7 +374,7 @@ elf_i386_reloc_name_lookup (bfd *abfd ATTRIBUTE_UNUSED,
 }
 
 static reloc_howto_type *
-elf_i386_rtype_to_howto (unsigned r_type)
+elf_i386_rtype_to_howto (bfd *abfd, unsigned r_type)
 {
   unsigned int indx;
 
@@ -384,6 +389,11 @@ elf_i386_rtype_to_howto (unsigned r_type)
   /* PR 17512: file: 0f67f69d.  */
   if (elf_howto_table [indx].type != r_type)
     return NULL;
+
+  /* Use complain_overflow_bitfield on R_386_PC16 for code16.  */
+  if (r_type == (unsigned int) R_386_PC16 && elf_x86_has_code16 (abfd))
+    indx = ARRAY_SIZE (elf_howto_table) - 1;
+
   return &elf_howto_table[indx];
 }
 
@@ -394,7 +404,8 @@ elf_i386_info_to_howto_rel (bfd *abfd,
 {
   unsigned int r_type = ELF32_R_TYPE (dst->r_info);
 
-  if ((cache_ptr->howto = elf_i386_rtype_to_howto (r_type)) == NULL)
+  if ((cache_ptr->howto = elf_i386_rtype_to_howto (abfd, r_type))
+      == NULL)
     {
       /* xgettext:c-format */
       _bfd_error_handler (_("%pB: unsupported relocation type %#x"),
@@ -1142,8 +1153,8 @@ elf_i386_tls_transition (struct bfd_link_info *info, bfd *abfd,
       reloc_howto_type *from, *to;
       const char *name;
 
-      from = elf_i386_rtype_to_howto (from_type);
-      to = elf_i386_rtype_to_howto (to_type);
+      from = elf_i386_rtype_to_howto (abfd, from_type);
+      to = elf_i386_rtype_to_howto (abfd, to_type);
 
       if (h)
 	name = h->root.root.string;
@@ -2074,7 +2085,7 @@ elf_i386_relocate_section (bfd *output_bfd,
 	  continue;
 	}
 
-      howto = elf_i386_rtype_to_howto (r_type);
+      howto = elf_i386_rtype_to_howto (input_bfd, r_type);
       if (howto == NULL)
 	return _bfd_unrecognized_reloc (input_bfd, input_section, r_type);
 
diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
index d0c994e570a..b6fefc968c0 100644
--- a/bfd/elf64-x86-64.c
+++ b/bfd/elf64-x86-64.c
@@ -187,6 +187,10 @@ static reloc_howto_type x86_64_elf_howto_table[] =
 	 _bfd_elf_rel_vtable_reloc_fn, "R_X86_64_GNU_VTENTRY", false, 0, 0,
 	 false),
 
+/* Use complain_overflow_bitfield on R_X86_64_PC16 for code16.  */
+  HOWTO(R_X86_64_PC16, 0, 1, 16, true, 0, complain_overflow_bitfield,
+	bfd_elf_generic_reloc, "R_X86_64_PC16", false, 0, 0xffff, true),
+
 /* Use complain_overflow_bitfield on R_X86_64_32 for x32.  */
   HOWTO(R_X86_64_32, 0, 2, 32, false, 0, complain_overflow_bitfield,
 	bfd_elf_generic_reloc, "R_X86_64_32", false, 0, 0xffffffff,
@@ -270,6 +274,14 @@ elf_x86_64_rtype_to_howto (bfd *abfd, unsigned r_type)
       else
 	i = ARRAY_SIZE (x86_64_elf_howto_table) - 1;
     }
+  if (r_type == (unsigned int) R_X86_64_PC16)
+    {
+      /* Use complain_overflow_bitfield on R_X86_64_PC16 for code16.  */
+      if (elf_x86_has_code16 (abfd))
+	i = ARRAY_SIZE (x86_64_elf_howto_table) - 2;
+      else
+	i = r_type;
+    }
   else if (r_type < (unsigned int) R_X86_64_GNU_VTINHERIT
 	   || r_type >= (unsigned int) R_X86_64_max)
     {
diff --git a/bfd/elfxx-x86.c b/bfd/elfxx-x86.c
index 62d516aab8d..29dc7f04b4d 100644
--- a/bfd/elfxx-x86.c
+++ b/bfd/elfxx-x86.c
@@ -2353,6 +2353,7 @@ _bfd_x86_elf_parse_gnu_properties (bfd *abfd, unsigned int type,
       || (type >= GNU_PROPERTY_X86_UINT32_OR_AND_LO
 	  && type <= GNU_PROPERTY_X86_UINT32_OR_AND_HI))
     {
+      unsigned int number;
       if (datasz != 4)
 	{
 	  _bfd_error_handler
@@ -2361,7 +2362,13 @@ _bfd_x86_elf_parse_gnu_properties (bfd *abfd, unsigned int type,
 	  return property_corrupt;
 	}
       prop = _bfd_elf_get_property (abfd, type, datasz);
-      prop->u.number |= bfd_h_get_32 (abfd, ptr);
+      number = bfd_h_get_32 (abfd, ptr);
+      if ((abfd->flags
+	   & (DYNAMIC | BFD_LINKER_CREATED | BFD_PLUGIN)) == 0
+	  && type == GNU_PROPERTY_X86_FEATURE_2_USED
+	  && (number & GNU_PROPERTY_X86_FEATURE_2_CODE16) != 0)
+	elf_x86_has_code16 (abfd) = 1;
+      prop->u.number |= number;
       prop->pr_kind = property_number;
       return property_number;
     }
diff --git a/bfd/elfxx-x86.h b/bfd/elfxx-x86.h
index db11327e96f..e8344305492 100644
--- a/bfd/elfxx-x86.h
+++ b/bfd/elfxx-x86.h
@@ -549,6 +549,9 @@ struct elf_x86_obj_tdata
 
   /* GOTPLT entries for TLS descriptors.  */
   bfd_vma *local_tlsdesc_gotent;
+
+  /* Set if the objec file has 16-bit code.  */
+  unsigned int has_code16 : 1;
 };
 
 enum elf_x86_plt_type
@@ -584,6 +587,9 @@ struct elf_x86_plt
 #define elf_x86_local_tlsdesc_gotent(abfd) \
   (elf_x86_tdata (abfd)->local_tlsdesc_gotent)
 
+#define elf_x86_has_code16(abfd) \
+  (elf_x86_tdata (abfd)->has_code16)
+
 #define elf_x86_compute_jump_table_size(htab) \
   ((htab)->elf.srelplt->reloc_count * (htab)->got_entry_size)
 
diff --git a/binutils/readelf.c b/binutils/readelf.c
index 2989adf31b6..8f24cc4071b 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -19140,6 +19140,9 @@ decode_x86_feature_2 (unsigned int bitmask)
 	case GNU_PROPERTY_X86_FEATURE_2_XSAVEC:
 	  printf ("XSAVEC");
 	  break;
+	case GNU_PROPERTY_X86_FEATURE_2_CODE16:
+	  printf ("CODE16");
+	  break;
 	default:
 	  printf (_("<unknown: %x>"), bit);
 	  break;
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index d3441988e34..c17f4da16fe 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2695,6 +2695,10 @@ static void
 set_code_flag (int value)
 {
   update_code_flag (value, 0);
+#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
+  if (value == CODE_16BIT)
+    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
+#endif
 }
 
 static void
@@ -2706,6 +2710,10 @@ set_16bit_gcc_code_flag (int new_code_flag)
   cpu_arch_flags.bitfield.cpu64 = 0;
   cpu_arch_flags.bitfield.cpuno64 = 1;
   stackop_size = LONG_MNEM_SUFFIX;
+#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
+  if (new_code_flag == CODE_16BIT)
+    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
+#endif
 }
 
 static void
@@ -9032,7 +9040,7 @@ x86_cleanup (void)
   unsigned int isa_1_descsz_raw, feature_2_descsz_raw;
   unsigned int padding;
 
-  if (!IS_ELF || !x86_used_note)
+  if (!IS_ELF || (!x86_used_note && !x86_feature_2_used))
     return;
 
   x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_X86;
@@ -9072,15 +9080,23 @@ x86_cleanup (void)
   bfd_set_section_alignment (sec, alignment);
   elf_section_type (sec) = SHT_NOTE;
 
-  /* GNU_PROPERTY_X86_ISA_1_USED: 4-byte type + 4-byte data size
-				  + 4-byte data  */
-  isa_1_descsz_raw = 4 + 4 + 4;
-  /* Align GNU_PROPERTY_X86_ISA_1_USED.  */
-  isa_1_descsz = (isa_1_descsz_raw + align_size_1) & ~align_size_1;
+  if (x86_used_note)
+    {
+      /* GNU_PROPERTY_X86_ISA_1_USED: 4-byte type + 4-byte data size
+	 + 4-byte data  */
+      isa_1_descsz_raw = 4 + 4 + 4;
+      /* Align GNU_PROPERTY_X86_ISA_1_USED.  */
+      isa_1_descsz = (isa_1_descsz_raw + align_size_1) & ~align_size_1;
+    }
+  else
+    {
+      isa_1_descsz_raw = 0;
+      isa_1_descsz = 0;
+    }
 
   feature_2_descsz_raw = isa_1_descsz;
   /* GNU_PROPERTY_X86_FEATURE_2_USED: 4-byte type + 4-byte data size
-				      + 4-byte data  */
+     + 4-byte data  */
   feature_2_descsz_raw += 4 + 4 + 4;
   /* Align GNU_PROPERTY_X86_FEATURE_2_USED.  */
   feature_2_descsz = ((feature_2_descsz_raw + align_size_1)
@@ -9102,20 +9118,23 @@ x86_cleanup (void)
   /* Write n_name.  */
   memcpy (p + 4 * 3, "GNU", 4);
 
-  /* Write 4-byte type.  */
-  md_number_to_chars (p + 4 * 4,
-		      (valueT) GNU_PROPERTY_X86_ISA_1_USED, 4);
+  if (isa_1_descsz != 0)
+    {
+      /* Write 4-byte type.  */
+      md_number_to_chars (p + 4 * 4,
+			  (valueT) GNU_PROPERTY_X86_ISA_1_USED, 4);
 
-  /* Write 4-byte data size.  */
-  md_number_to_chars (p + 4 * 5, (valueT) 4, 4);
+      /* Write 4-byte data size.  */
+      md_number_to_chars (p + 4 * 5, (valueT) 4, 4);
 
-  /* Write 4-byte data.  */
-  md_number_to_chars (p + 4 * 6, (valueT) x86_isa_1_used, 4);
+      /* Write 4-byte data.  */
+      md_number_to_chars (p + 4 * 6, (valueT) x86_isa_1_used, 4);
 
-  /* Zero out paddings.  */
-  padding = isa_1_descsz - isa_1_descsz_raw;
-  if (padding)
-    memset (p + 4 * 7, 0, padding);
+      /* Zero out paddings.  */
+      padding = isa_1_descsz - isa_1_descsz_raw;
+      if (padding)
+	memset (p + 4 * 7, 0, padding);
+    }
 
   /* Write 4-byte type.  */
   md_number_to_chars (p + isa_1_descsz + 4 * 4,
diff --git a/gas/testsuite/gas/i386/code16-2.d b/gas/testsuite/gas/i386/code16-2.d
new file mode 100644
index 00000000000..37b66c85f4e
--- /dev/null
+++ b/gas/testsuite/gas/i386/code16-2.d
@@ -0,0 +1,8 @@
+#name: i386 code16 2
+#as: -mx86-used-note=no --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 feature used: x86, CODE16
diff --git a/gas/testsuite/gas/i386/code16-2.s b/gas/testsuite/gas/i386/code16-2.s
new file mode 100644
index 00000000000..66e5d558791
--- /dev/null
+++ b/gas/testsuite/gas/i386/code16-2.s
@@ -0,0 +1,10 @@
+	.code16gcc
+	.text
+	.section	.text.default_process_op.isra.0,"ax",@progbits
+	.type	default_process_op.isra.0, @function
+default_process_op.isra.0:
+	ret
+	.section	.text.mpt_scsi_process_op,"ax",@progbits
+	.type	mpt_scsi_process_op, @function
+mpt_scsi_process_op:
+	jmp	default_process_op.isra.0
diff --git a/gas/testsuite/gas/i386/i386.exp b/gas/testsuite/gas/i386/i386.exp
index 39010bdd500..a459c6fe392 100644
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -734,6 +734,7 @@ if {[is_elf_format] || [istarget "*-*-vxworks*"]} then {
     run_dump_test "property-ldmxcsr"
     run_dump_test "property-vldmxcsr"
     run_dump_test "property-vzeroall"
+    run_dump_test "code16-2"
 
     if {![istarget "*-*-dragonfly*"]
 	&& ![istarget "*-*-gnu*"]
@@ -1298,6 +1299,7 @@ if [gas_64_check] then {
 	run_dump_test "x86-64-property-8"
 	run_dump_test "x86-64-property-9"
 	run_dump_test "x86-64-property-14"
+	run_dump_test "x86-64-code16-2"
 
 	if {[istarget "*-*-linux*"]} then {
 	    run_dump_test "x86-64-align-branch-3"
diff --git a/gas/testsuite/gas/i386/x86-64-code16-2.d b/gas/testsuite/gas/i386/x86-64-code16-2.d
new file mode 100644
index 00000000000..dbabd67e888
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-code16-2.d
@@ -0,0 +1,9 @@
+#source: code16-2.s
+#name: x86-64 code16 2
+#as: -mx86-used-note=no --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 feature used: x86, CODE16
diff --git a/include/elf/common.h b/include/elf/common.h
index 24d0a09b7c8..564ab711a20 100644
--- a/include/elf/common.h
+++ b/include/elf/common.h
@@ -872,6 +872,7 @@
 #define GNU_PROPERTY_X86_FEATURE_2_XSAVEC	(1U << 9)
 #define GNU_PROPERTY_X86_FEATURE_2_TMM		(1U << 10)
 #define GNU_PROPERTY_X86_FEATURE_2_MASK		(1U << 11)
+#define GNU_PROPERTY_X86_FEATURE_2_CODE16	(1U << 12)
 
 #define GNU_PROPERTY_X86_COMPAT_2_ISA_1_NEEDED \
   (GNU_PROPERTY_X86_UINT32_OR_LO + 0)
diff --git a/ld/testsuite/ld-i386/code16.d b/ld/testsuite/ld-i386/code16.d
new file mode 100644
index 00000000000..8b67861db91
--- /dev/null
+++ b/ld/testsuite/ld-i386/code16.d
@@ -0,0 +1,19 @@
+#name: i386 R_386_PC16 reloc in 16-bit mode
+#as: --32 -mx86-used-note=no --generate-missing-build-notes=no
+#source: ${srcdir}/../../../gas/testsuite/gas/i386/code16-2.s
+#ld: -T code16.t
+#objdump: -dw -Mi8086
+
+.*: +file format .*
+
+
+Disassembly of section .text.default_process_op.isra.0:
+
+0+737c <default_process_op.isra.0>:
+ +[a-f0-9]+:	66 c3                	retl   
+
+Disassembly of section .text.mpt_scsi_process_op:
+
+0+f869 <mpt_scsi_process_op>:
+ +[a-f0-9]+:	e9 10 7b             	jmp    737c <default_process_op.isra.0>
+#pass
diff --git a/ld/testsuite/ld-i386/code16.t b/ld/testsuite/ld-i386/code16.t
new file mode 100644
index 00000000000..0cf99042e40
--- /dev/null
+++ b/ld/testsuite/ld-i386/code16.t
@@ -0,0 +1,7 @@
+OUTPUT_FORMAT("elf32-i386")
+OUTPUT_ARCH("i386")
+SECTIONS
+{
+.text.default_process_op.isra.0 0x737c : { *(.text.default_process_op.isra.0) }
+.text.mpt_scsi_process_op 0xf869 : { *(.text.mpt_scsi_process_op) }
+}
diff --git a/ld/testsuite/ld-i386/i386.exp b/ld/testsuite/ld-i386/i386.exp
index d0b3f69fb8d..3d6047b790d 100644
--- a/ld/testsuite/ld-i386/i386.exp
+++ b/ld/testsuite/ld-i386/i386.exp
@@ -492,6 +492,7 @@ run_dump_test "property-x86-isa2"
 run_dump_test "property-x86-isa3"
 run_dump_test "property-x86-isa4"
 run_dump_test "pr26869"
+run_dump_test "code16"
 
 if { !([istarget "i?86-*-linux*"]
        || [istarget "i?86-*-gnu*"]
diff --git a/ld/testsuite/ld-x86-64/code16.d b/ld/testsuite/ld-x86-64/code16.d
new file mode 100644
index 00000000000..20096ab6abf
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/code16.d
@@ -0,0 +1,19 @@
+#name: x86-64 R_X86_64_PC16 reloc in 16-bit mode
+#as: --64 -mx86-used-note=no --generate-missing-build-notes=no
+#source: ${srcdir}/../../../gas/testsuite/gas/i386/code16-2.s
+#ld: -T code16.t
+#objdump: -dw -Mi8086
+
+.*: +file format .*
+
+
+Disassembly of section .text.default_process_op.isra.0:
+
+0+737c <default_process_op.isra.0>:
+ +[a-f0-9]+:	66 c3                	retl   
+
+Disassembly of section .text.mpt_scsi_process_op:
+
+0+f869 <mpt_scsi_process_op>:
+ +[a-f0-9]+:	e9 10 7b             	jmp    737c <default_process_op.isra.0>
+#pass
diff --git a/ld/testsuite/ld-x86-64/code16.t b/ld/testsuite/ld-x86-64/code16.t
new file mode 100644
index 00000000000..9ef00a3404d
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/code16.t
@@ -0,0 +1,7 @@
+OUTPUT_FORMAT("elf64-x86-64")
+OUTPUT_ARCH("i386:x86-64")
+SECTIONS
+{
+.text.default_process_op.isra.0 0x737c : { *(.text.default_process_op.isra.0) }
+.text.mpt_scsi_process_op 0xf869 : { *(.text.mpt_scsi_process_op) }
+}
diff --git a/ld/testsuite/ld-x86-64/x86-64.exp b/ld/testsuite/ld-x86-64/x86-64.exp
index 17fd10ee121..b71059afd5b 100644
--- a/ld/testsuite/ld-x86-64/x86-64.exp
+++ b/ld/testsuite/ld-x86-64/x86-64.exp
@@ -474,6 +474,7 @@ run_dump_test "property-x86-isa3"
 run_dump_test "property-x86-isa3-x32"
 run_dump_test "property-x86-isa4"
 run_dump_test "property-x86-isa4-x32"
+run_dump_test "code16"
 
 if ![istarget "x86_64-*-linux*"] {
     return
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-25 13:41   ` [PATCH v2] " H.J. Lu
@ 2021-05-25 15:34     ` Jan Beulich
  2021-05-25 15:40       ` H.J. Lu
  0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2021-05-25 15:34 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Binutils, Martin Liška

On 25.05.2021 15:41, H.J. Lu wrote:
> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>> Author: Jan Beulich <jbeulich@suse.com>
>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>
>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>
>>> caused linker failure when building 16-bit program in a 32-bit ELF
>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
>>>
>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
>>>
>>> as in
>>>
>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
>>>
>>> to indicate that 16-bit mode instructions are used in the object to
>>> allow linker to properly perform relocation overflow check for 16-bit
>>> PC-relative relocations in 16-bit mode instructions.
>>
>> I certainly don't mind the introduction of this flag; I think its
>> introduction wants to be separated from the specific use in the
>> linker, not the lease because ...
>>
>>> 1. Update x86 assembler to always generate the GNU property note with
>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
>>
>> ... I don't think this is an appropriate step to take. The majority
>> of cases of 16-bit code use that I know of is in projects where this
>> is just a small portion of code, and the rest of the code is 32-
>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
>> as indication to relax overflow checking, you undermine the main
>> goal of the earlier change.
> 
> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> Author: Jan Beulich <jbeulich@suse.com>
> Date:   Mon Apr 26 10:41:35 2021 +0200
> 
>     x86: correct overflow checking for 16-bit PC-relative relocs
> 
> is technically correct according to psABIs.  But GNU assembler
> only generates PC16 relocations for 16-bit codes.  That is why
> we never ran into any PC16 relocation overflow before.  My change
> restores the old behavior only when input has 16-bit codes.

When paying specific attention to resulting code size, people may
want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
code. This not having worked properly is what initially triggered
me touching this area. And of course assembler programmers could
even cause data to have PC16 relocations attached to it.

>  I think it
> is a good compromise between usability and error checking.

Maybe it is; I'm not convinced though as of yet.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-25 15:34     ` Jan Beulich
@ 2021-05-25 15:40       ` H.J. Lu
  2021-05-25 16:08         ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-05-25 15:40 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils, Martin Liška

On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.05.2021 15:41, H.J. Lu wrote:
> > On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> >>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>> Author: Jan Beulich <jbeulich@suse.com>
> >>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>
> >>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>
> >>> caused linker failure when building 16-bit program in a 32-bit ELF
> >>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> >>>
> >>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> >>>
> >>> as in
> >>>
> >>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> >>>
> >>> to indicate that 16-bit mode instructions are used in the object to
> >>> allow linker to properly perform relocation overflow check for 16-bit
> >>> PC-relative relocations in 16-bit mode instructions.
> >>
> >> I certainly don't mind the introduction of this flag; I think its
> >> introduction wants to be separated from the specific use in the
> >> linker, not the lease because ...
> >>
> >>> 1. Update x86 assembler to always generate the GNU property note with
> >>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> >>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> >>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
> >>
> >> ... I don't think this is an appropriate step to take. The majority
> >> of cases of 16-bit code use that I know of is in projects where this
> >> is just a small portion of code, and the rest of the code is 32-
> >> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
> >> as indication to relax overflow checking, you undermine the main
> >> goal of the earlier change.
> >
> > commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> > Author: Jan Beulich <jbeulich@suse.com>
> > Date:   Mon Apr 26 10:41:35 2021 +0200
> >
> >     x86: correct overflow checking for 16-bit PC-relative relocs
> >
> > is technically correct according to psABIs.  But GNU assembler
> > only generates PC16 relocations for 16-bit codes.  That is why
> > we never ran into any PC16 relocation overflow before.  My change
> > restores the old behavior only when input has 16-bit codes.
>
> When paying specific attention to resulting code size, people may
> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
> code. This not having worked properly is what initially triggered
> me touching this area. And of course assembler programmers could
> even cause data to have PC16 relocations attached to it.

As long as it doesn't .code16, PC16 relocation overflow check won't be
changed.

> >  I think it
> > is a good compromise between usability and error checking.
>
> Maybe it is; I'm not convinced though as of yet.
>

My solution isn't perfect.  But it should cover most use cases.
seabios should build out of box with binutils 2.37.

-- 
H.J.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-25 15:40       ` H.J. Lu
@ 2021-05-25 16:08         ` Jan Beulich
  2021-05-25 16:11           ` H.J. Lu
  0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2021-05-25 16:08 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Binutils, Martin Liška

On 25.05.2021 17:40, H.J. Lu wrote:
> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 25.05.2021 15:41, H.J. Lu wrote:
>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>
>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>
>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
>>>>>
>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
>>>>>
>>>>> as in
>>>>>
>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
>>>>>
>>>>> to indicate that 16-bit mode instructions are used in the object to
>>>>> allow linker to properly perform relocation overflow check for 16-bit
>>>>> PC-relative relocations in 16-bit mode instructions.
>>>>
>>>> I certainly don't mind the introduction of this flag; I think its
>>>> introduction wants to be separated from the specific use in the
>>>> linker, not the lease because ...
>>>>
>>>>> 1. Update x86 assembler to always generate the GNU property note with
>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
>>>>
>>>> ... I don't think this is an appropriate step to take. The majority
>>>> of cases of 16-bit code use that I know of is in projects where this
>>>> is just a small portion of code, and the rest of the code is 32-
>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
>>>> as indication to relax overflow checking, you undermine the main
>>>> goal of the earlier change.
>>>
>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>> Author: Jan Beulich <jbeulich@suse.com>
>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>
>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>
>>> is technically correct according to psABIs.  But GNU assembler
>>> only generates PC16 relocations for 16-bit codes.  That is why
>>> we never ran into any PC16 relocation overflow before.  My change
>>> restores the old behavior only when input has 16-bit codes.
>>
>> When paying specific attention to resulting code size, people may
>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
>> code. This not having worked properly is what initially triggered
>> me touching this area. And of course assembler programmers could
>> even cause data to have PC16 relocations attached to it.
> 
> As long as it doesn't .code16, PC16 relocation overflow check won't be
> changed.

Of course, but I expect typical use cases (kernels, hypervisors) to
contain small pieces of 16-bit code.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-25 16:08         ` Jan Beulich
@ 2021-05-25 16:11           ` H.J. Lu
  2021-05-25 16:27             ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-05-25 16:11 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils, Martin Liška

On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.05.2021 17:40, H.J. Lu wrote:
> > On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 25.05.2021 15:41, H.J. Lu wrote:
> >>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> >>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>
> >>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>
> >>>>> caused linker failure when building 16-bit program in a 32-bit ELF
> >>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> >>>>>
> >>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> >>>>>
> >>>>> as in
> >>>>>
> >>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> >>>>>
> >>>>> to indicate that 16-bit mode instructions are used in the object to
> >>>>> allow linker to properly perform relocation overflow check for 16-bit
> >>>>> PC-relative relocations in 16-bit mode instructions.
> >>>>
> >>>> I certainly don't mind the introduction of this flag; I think its
> >>>> introduction wants to be separated from the specific use in the
> >>>> linker, not the lease because ...
> >>>>
> >>>>> 1. Update x86 assembler to always generate the GNU property note with
> >>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> >>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> >>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
> >>>>
> >>>> ... I don't think this is an appropriate step to take. The majority
> >>>> of cases of 16-bit code use that I know of is in projects where this
> >>>> is just a small portion of code, and the rest of the code is 32-
> >>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
> >>>> as indication to relax overflow checking, you undermine the main
> >>>> goal of the earlier change.
> >>>
> >>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>> Author: Jan Beulich <jbeulich@suse.com>
> >>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>
> >>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>
> >>> is technically correct according to psABIs.  But GNU assembler
> >>> only generates PC16 relocations for 16-bit codes.  That is why
> >>> we never ran into any PC16 relocation overflow before.  My change
> >>> restores the old behavior only when input has 16-bit codes.
> >>
> >> When paying specific attention to resulting code size, people may
> >> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
> >> code. This not having worked properly is what initially triggered
> >> me touching this area. And of course assembler programmers could
> >> even cause data to have PC16 relocations attached to it.
> >
> > As long as it doesn't .code16, PC16 relocation overflow check won't be
> > changed.
>
> Of course, but I expect typical use cases (kernels, hypervisors) to
> contain small pieces of 16-bit code.

And we never ran into any issues before.  My patch just restores
the old behavior for them.

-- 
H.J.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-25 16:11           ` H.J. Lu
@ 2021-05-25 16:27             ` Jan Beulich
  2021-05-25 16:31               ` H.J. Lu
  0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2021-05-25 16:27 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Binutils, Martin Liška

On 25.05.2021 18:11, H.J. Lu wrote:
> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 25.05.2021 17:40, H.J. Lu wrote:
>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 25.05.2021 15:41, H.J. Lu wrote:
>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>
>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>
>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
>>>>>>>
>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
>>>>>>>
>>>>>>> as in
>>>>>>>
>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
>>>>>>>
>>>>>>> to indicate that 16-bit mode instructions are used in the object to
>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
>>>>>>> PC-relative relocations in 16-bit mode instructions.
>>>>>>
>>>>>> I certainly don't mind the introduction of this flag; I think its
>>>>>> introduction wants to be separated from the specific use in the
>>>>>> linker, not the lease because ...
>>>>>>
>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
>>>>>>
>>>>>> ... I don't think this is an appropriate step to take. The majority
>>>>>> of cases of 16-bit code use that I know of is in projects where this
>>>>>> is just a small portion of code, and the rest of the code is 32-
>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
>>>>>> as indication to relax overflow checking, you undermine the main
>>>>>> goal of the earlier change.
>>>>>
>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>
>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>
>>>>> is technically correct according to psABIs.  But GNU assembler
>>>>> only generates PC16 relocations for 16-bit codes.  That is why
>>>>> we never ran into any PC16 relocation overflow before.  My change
>>>>> restores the old behavior only when input has 16-bit codes.
>>>>
>>>> When paying specific attention to resulting code size, people may
>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
>>>> code. This not having worked properly is what initially triggered
>>>> me touching this area. And of course assembler programmers could
>>>> even cause data to have PC16 relocations attached to it.
>>>
>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
>>> changed.
>>
>> Of course, but I expect typical use cases (kernels, hypervisors) to
>> contain small pieces of 16-bit code.
> 
> And we never ran into any issues before.  My patch just restores
> the old behavior for them.

And my point is that the old behavior was wrong (and will be again if
we go with your change).

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-25 16:27             ` Jan Beulich
@ 2021-05-25 16:31               ` H.J. Lu
  2021-05-26 14:16                 ` [PATCH v3] " H.J. Lu
  0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-05-25 16:31 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils, Martin Liška

On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.05.2021 18:11, H.J. Lu wrote:
> > On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 25.05.2021 17:40, H.J. Lu wrote:
> >>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 25.05.2021 15:41, H.J. Lu wrote:
> >>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>
> >>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> >>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>>>
> >>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>>>
> >>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
> >>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> >>>>>>>
> >>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> >>>>>>>
> >>>>>>> as in
> >>>>>>>
> >>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> >>>>>>>
> >>>>>>> to indicate that 16-bit mode instructions are used in the object to
> >>>>>>> allow linker to properly perform relocation overflow check for 16-bit
> >>>>>>> PC-relative relocations in 16-bit mode instructions.
> >>>>>>
> >>>>>> I certainly don't mind the introduction of this flag; I think its
> >>>>>> introduction wants to be separated from the specific use in the
> >>>>>> linker, not the lease because ...
> >>>>>>
> >>>>>>> 1. Update x86 assembler to always generate the GNU property note with
> >>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> >>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> >>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
> >>>>>>
> >>>>>> ... I don't think this is an appropriate step to take. The majority
> >>>>>> of cases of 16-bit code use that I know of is in projects where this
> >>>>>> is just a small portion of code, and the rest of the code is 32-
> >>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
> >>>>>> as indication to relax overflow checking, you undermine the main
> >>>>>> goal of the earlier change.
> >>>>>
> >>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>
> >>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>
> >>>>> is technically correct according to psABIs.  But GNU assembler
> >>>>> only generates PC16 relocations for 16-bit codes.  That is why
> >>>>> we never ran into any PC16 relocation overflow before.  My change
> >>>>> restores the old behavior only when input has 16-bit codes.
> >>>>
> >>>> When paying specific attention to resulting code size, people may
> >>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
> >>>> code. This not having worked properly is what initially triggered
> >>>> me touching this area. And of course assembler programmers could
> >>>> even cause data to have PC16 relocations attached to it.
> >>>
> >>> As long as it doesn't .code16, PC16 relocation overflow check won't be
> >>> changed.
> >>
> >> Of course, but I expect typical use cases (kernels, hypervisors) to
> >> contain small pieces of 16-bit code.
> >
> > And we never ran into any issues before.  My patch just restores
> > the old behavior for them.
>
> And my point is that the old behavior was wrong (and will be again if
> we go with your change).

The odd behavior is correct for 16-bit codes.  My patch restores the
old behavior only if input has 16-bit codes.


-- 
H.J.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-25 16:31               ` H.J. Lu
@ 2021-05-26 14:16                 ` H.J. Lu
  2021-05-27  7:10                   ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-05-26 14:16 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils, Martin Liška

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

On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 25.05.2021 18:11, H.J. Lu wrote:
> > > On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>
> > >> On 25.05.2021 17:40, H.J. Lu wrote:
> > >>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>
> > >>>> On 25.05.2021 15:41, H.J. Lu wrote:
> > >>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>>>
> > >>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> > >>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> > >>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> > >>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> > >>>>>>>
> > >>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> > >>>>>>>
> > >>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
> > >>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> > >>>>>>>
> > >>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> > >>>>>>>
> > >>>>>>> as in
> > >>>>>>>
> > >>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> > >>>>>>>
> > >>>>>>> to indicate that 16-bit mode instructions are used in the object to
> > >>>>>>> allow linker to properly perform relocation overflow check for 16-bit
> > >>>>>>> PC-relative relocations in 16-bit mode instructions.
> > >>>>>>
> > >>>>>> I certainly don't mind the introduction of this flag; I think its
> > >>>>>> introduction wants to be separated from the specific use in the
> > >>>>>> linker, not the lease because ...
> > >>>>>>
> > >>>>>>> 1. Update x86 assembler to always generate the GNU property note with
> > >>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> > >>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> > >>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
> > >>>>>>
> > >>>>>> ... I don't think this is an appropriate step to take. The majority
> > >>>>>> of cases of 16-bit code use that I know of is in projects where this
> > >>>>>> is just a small portion of code, and the rest of the code is 32-
> > >>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
> > >>>>>> as indication to relax overflow checking, you undermine the main
> > >>>>>> goal of the earlier change.
> > >>>>>
> > >>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> > >>>>> Author: Jan Beulich <jbeulich@suse.com>
> > >>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> > >>>>>
> > >>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> > >>>>>
> > >>>>> is technically correct according to psABIs.  But GNU assembler
> > >>>>> only generates PC16 relocations for 16-bit codes.  That is why
> > >>>>> we never ran into any PC16 relocation overflow before.  My change
> > >>>>> restores the old behavior only when input has 16-bit codes.
> > >>>>
> > >>>> When paying specific attention to resulting code size, people may
> > >>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
> > >>>> code. This not having worked properly is what initially triggered
> > >>>> me touching this area. And of course assembler programmers could
> > >>>> even cause data to have PC16 relocations attached to it.
> > >>>
> > >>> As long as it doesn't .code16, PC16 relocation overflow check won't be
> > >>> changed.
> > >>
> > >> Of course, but I expect typical use cases (kernels, hypervisors) to
> > >> contain small pieces of 16-bit code.
> > >
> > > And we never ran into any issues before.  My patch just restores
> > > the old behavior for them.
> >
> > And my point is that the old behavior was wrong (and will be again if
> > we go with your change).
>
> The odd behavior is correct for 16-bit codes.  My patch restores the
> old behavior only if input has 16-bit codes.
>

This is the patch I am going to check in.

-- 
H.J.

[-- Attachment #2: v3-0001-x86-Propery-check-PC16-reloc-overflow-in-16-bit-m.patch --]
[-- Type: text/x-patch, Size: 19302 bytes --]

From 20ded8660f1a6eb4dd13002c54ead53e6ff58e88 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 24 May 2021 16:11:04 -0700
Subject: [PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode
 instructions

commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
Author: Jan Beulich <jbeulich@suse.com>
Date:   Mon Apr 26 10:41:35 2021 +0200

    x86: correct overflow checking for 16-bit PC-relative relocs

caused linker failure when building 16-bit program in a 32-bit ELF
container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with

 #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)

to indicate that 16-bit mode instructions are used in the input object:

https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA

to indicate that 16-bit mode instructions are used in the object to
allow linker to properly perform relocation overflow check for 16-bit
PC-relative relocations in 16-bit mode instructions.

1. Update x86 assembler to always generate the GNU property note with
GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.

bfd/

	PR ld/27905
	* elf32-i386.c: Include "libiberty.h".
	(elf_howto_table): Add 16-bit R_386_PC16 entry.
	(elf_i386_rtype_to_howto): Add a BFD argument.  Use 16-bit
	R_386_PC16 if input has 16-bit mode instructions.
	(elf_i386_info_to_howto_rel): Update elf_i386_rtype_to_howto
	call.
	(elf_i386_tls_transition): Likewise.
	(elf_i386_relocate_section): Likewise.
	* elf64-x86-64.c (x86_64_elf_howto_table): Add 16-bit
	R_X86_64_PC16 entry.
	(elf_x86_64_rtype_to_howto): Use 16-bit R_X86_64_PC16 if input
	has 16-bit mode instructions.
	* elfxx-x86.c (_bfd_x86_elf_parse_gnu_properties): Set
	elf_x86_has_code16 if relocatable input is marked with
	GNU_PROPERTY_X86_FEATURE_2_CODE16.
	* elfxx-x86.h (elf_x86_obj_tdata): Add has_code16.
	(elf_x86_has_code16): New.

binutils/

	PR ld/27905
	* readelf.c (decode_x86_feature_2): Support
	GNU_PROPERTY_X86_FEATURE_2_CODE16.

gas/

	PR ld/27905
	* config/tc-i386.c (set_code_flag): Update x86_feature_2_used
	with GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF
	object.
	(set_16bit_gcc_code_flag): Likewise.
	(x86_cleanup): Always generate the GNU property note if
	x86_feature_2_used isn't 0.
	* testsuite/gas/i386/code16-2.d: New file.
	* testsuite/gas/i386/code16-2.s: Likewise.
	* testsuite/gas/i386/x86-64-code16-2.d: Likewise.
	* testsuite/gas/i386/i386.exp: Run code16-2 and x86-64-code16-2.

include/

	PR ld/27905
	* elf/common.h (GNU_PROPERTY_X86_FEATURE_2_CODE16): New.

ld/

	PR ld/27905
	* testsuite/ld-i386/code16.d: New file.
	* testsuite/ld-i386/code16.t: Likewise.
	* testsuite/ld-x86-64/code16.d: Likewise.
	* testsuite/ld-x86-64/code16.t: Likewise.
	* testsuite/ld-i386/i386.exp: Run code16.
	* testsuite/ld-x86-64/x86-64.exp: Likewise.
---
 bfd/elf32-i386.c                         | 23 +++++++---
 bfd/elf64-x86-64.c                       | 12 ++++++
 bfd/elfxx-x86.c                          |  9 +++-
 bfd/elfxx-x86.h                          |  6 +++
 binutils/readelf.c                       |  3 ++
 gas/config/tc-i386.c                     | 55 ++++++++++++++++--------
 gas/testsuite/gas/i386/code16-2.d        |  8 ++++
 gas/testsuite/gas/i386/code16-2.s        | 10 +++++
 gas/testsuite/gas/i386/i386.exp          |  2 +
 gas/testsuite/gas/i386/x86-64-code16-2.d |  9 ++++
 include/elf/common.h                     |  1 +
 ld/testsuite/ld-i386/code16.d            | 19 ++++++++
 ld/testsuite/ld-i386/code16.t            |  7 +++
 ld/testsuite/ld-i386/i386.exp            |  1 +
 ld/testsuite/ld-x86-64/code16.d          | 19 ++++++++
 ld/testsuite/ld-x86-64/code16.t          |  7 +++
 ld/testsuite/ld-x86-64/x86-64.exp        |  1 +
 17 files changed, 167 insertions(+), 25 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/code16-2.d
 create mode 100644 gas/testsuite/gas/i386/code16-2.s
 create mode 100644 gas/testsuite/gas/i386/x86-64-code16-2.d
 create mode 100644 ld/testsuite/ld-i386/code16.d
 create mode 100644 ld/testsuite/ld-i386/code16.t
 create mode 100644 ld/testsuite/ld-x86-64/code16.d
 create mode 100644 ld/testsuite/ld-x86-64/code16.t

diff --git a/bfd/elf32-i386.c b/bfd/elf32-i386.c
index 4451f9b451c..c68741af02c 100644
--- a/bfd/elf32-i386.c
+++ b/bfd/elf32-i386.c
@@ -22,6 +22,7 @@
 #include "elf-vxworks.h"
 #include "dwarf2.h"
 #include "opcode/i386.h"
+#include "libiberty.h"
 
 /* 386 uses REL relocations instead of RELA.  */
 #define USE_REL	1
@@ -175,10 +176,14 @@ static reloc_howto_type elf_howto_table[]=
 	 false,			/* partial_inplace */
 	 0,			/* src_mask */
 	 0,			/* dst_mask */
-	 false)			/* pcrel_offset */
+	 false),		/* pcrel_offset */
 
 #define R_386_vt (R_386_GNU_VTENTRY + 1 - R_386_vt_offset)
 
+/* Use complain_overflow_bitfield on R_386_PC16 for code16.  */
+  HOWTO(R_386_PC16, 0, 1, 16, true, 0, complain_overflow_bitfield,
+	bfd_elf_generic_reloc, "R_386_PC16",
+	true, 0xffff, 0xffff, true)
 };
 
 #define X86_PCREL_TYPE_P(TYPE) ((TYPE) == R_386_PC32)
@@ -369,7 +374,7 @@ elf_i386_reloc_name_lookup (bfd *abfd ATTRIBUTE_UNUSED,
 }
 
 static reloc_howto_type *
-elf_i386_rtype_to_howto (unsigned r_type)
+elf_i386_rtype_to_howto (bfd *abfd, unsigned r_type)
 {
   unsigned int indx;
 
@@ -384,6 +389,11 @@ elf_i386_rtype_to_howto (unsigned r_type)
   /* PR 17512: file: 0f67f69d.  */
   if (elf_howto_table [indx].type != r_type)
     return NULL;
+
+  /* Use complain_overflow_bitfield on R_386_PC16 for code16.  */
+  if (r_type == (unsigned int) R_386_PC16 && elf_x86_has_code16 (abfd))
+    indx = ARRAY_SIZE (elf_howto_table) - 1;
+
   return &elf_howto_table[indx];
 }
 
@@ -394,7 +404,8 @@ elf_i386_info_to_howto_rel (bfd *abfd,
 {
   unsigned int r_type = ELF32_R_TYPE (dst->r_info);
 
-  if ((cache_ptr->howto = elf_i386_rtype_to_howto (r_type)) == NULL)
+  if ((cache_ptr->howto = elf_i386_rtype_to_howto (abfd, r_type))
+      == NULL)
     {
       /* xgettext:c-format */
       _bfd_error_handler (_("%pB: unsupported relocation type %#x"),
@@ -1142,8 +1153,8 @@ elf_i386_tls_transition (struct bfd_link_info *info, bfd *abfd,
       reloc_howto_type *from, *to;
       const char *name;
 
-      from = elf_i386_rtype_to_howto (from_type);
-      to = elf_i386_rtype_to_howto (to_type);
+      from = elf_i386_rtype_to_howto (abfd, from_type);
+      to = elf_i386_rtype_to_howto (abfd, to_type);
 
       if (h)
 	name = h->root.root.string;
@@ -2074,7 +2085,7 @@ elf_i386_relocate_section (bfd *output_bfd,
 	  continue;
 	}
 
-      howto = elf_i386_rtype_to_howto (r_type);
+      howto = elf_i386_rtype_to_howto (input_bfd, r_type);
       if (howto == NULL)
 	return _bfd_unrecognized_reloc (input_bfd, input_section, r_type);
 
diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
index d0c994e570a..d420561c156 100644
--- a/bfd/elf64-x86-64.c
+++ b/bfd/elf64-x86-64.c
@@ -187,6 +187,10 @@ static reloc_howto_type x86_64_elf_howto_table[] =
 	 _bfd_elf_rel_vtable_reloc_fn, "R_X86_64_GNU_VTENTRY", false, 0, 0,
 	 false),
 
+/* Use complain_overflow_bitfield on R_X86_64_PC16 for code16.  */
+  HOWTO(R_X86_64_PC16, 0, 1, 16, true, 0, complain_overflow_bitfield,
+	bfd_elf_generic_reloc, "R_X86_64_PC16", false, 0, 0xffff, true),
+
 /* Use complain_overflow_bitfield on R_X86_64_32 for x32.  */
   HOWTO(R_X86_64_32, 0, 2, 32, false, 0, complain_overflow_bitfield,
 	bfd_elf_generic_reloc, "R_X86_64_32", false, 0, 0xffffffff,
@@ -270,6 +274,14 @@ elf_x86_64_rtype_to_howto (bfd *abfd, unsigned r_type)
       else
 	i = ARRAY_SIZE (x86_64_elf_howto_table) - 1;
     }
+  else if (r_type == (unsigned int) R_X86_64_PC16)
+    {
+      /* Use complain_overflow_bitfield on R_X86_64_PC16 for code16.  */
+      if (elf_x86_has_code16 (abfd))
+	i = ARRAY_SIZE (x86_64_elf_howto_table) - 2;
+      else
+	i = r_type;
+    }
   else if (r_type < (unsigned int) R_X86_64_GNU_VTINHERIT
 	   || r_type >= (unsigned int) R_X86_64_max)
     {
diff --git a/bfd/elfxx-x86.c b/bfd/elfxx-x86.c
index 62d516aab8d..29dc7f04b4d 100644
--- a/bfd/elfxx-x86.c
+++ b/bfd/elfxx-x86.c
@@ -2353,6 +2353,7 @@ _bfd_x86_elf_parse_gnu_properties (bfd *abfd, unsigned int type,
       || (type >= GNU_PROPERTY_X86_UINT32_OR_AND_LO
 	  && type <= GNU_PROPERTY_X86_UINT32_OR_AND_HI))
     {
+      unsigned int number;
       if (datasz != 4)
 	{
 	  _bfd_error_handler
@@ -2361,7 +2362,13 @@ _bfd_x86_elf_parse_gnu_properties (bfd *abfd, unsigned int type,
 	  return property_corrupt;
 	}
       prop = _bfd_elf_get_property (abfd, type, datasz);
-      prop->u.number |= bfd_h_get_32 (abfd, ptr);
+      number = bfd_h_get_32 (abfd, ptr);
+      if ((abfd->flags
+	   & (DYNAMIC | BFD_LINKER_CREATED | BFD_PLUGIN)) == 0
+	  && type == GNU_PROPERTY_X86_FEATURE_2_USED
+	  && (number & GNU_PROPERTY_X86_FEATURE_2_CODE16) != 0)
+	elf_x86_has_code16 (abfd) = 1;
+      prop->u.number |= number;
       prop->pr_kind = property_number;
       return property_number;
     }
diff --git a/bfd/elfxx-x86.h b/bfd/elfxx-x86.h
index db11327e96f..e8344305492 100644
--- a/bfd/elfxx-x86.h
+++ b/bfd/elfxx-x86.h
@@ -549,6 +549,9 @@ struct elf_x86_obj_tdata
 
   /* GOTPLT entries for TLS descriptors.  */
   bfd_vma *local_tlsdesc_gotent;
+
+  /* Set if the objec file has 16-bit code.  */
+  unsigned int has_code16 : 1;
 };
 
 enum elf_x86_plt_type
@@ -584,6 +587,9 @@ struct elf_x86_plt
 #define elf_x86_local_tlsdesc_gotent(abfd) \
   (elf_x86_tdata (abfd)->local_tlsdesc_gotent)
 
+#define elf_x86_has_code16(abfd) \
+  (elf_x86_tdata (abfd)->has_code16)
+
 #define elf_x86_compute_jump_table_size(htab) \
   ((htab)->elf.srelplt->reloc_count * (htab)->got_entry_size)
 
diff --git a/binutils/readelf.c b/binutils/readelf.c
index 2989adf31b6..8f24cc4071b 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -19140,6 +19140,9 @@ decode_x86_feature_2 (unsigned int bitmask)
 	case GNU_PROPERTY_X86_FEATURE_2_XSAVEC:
 	  printf ("XSAVEC");
 	  break;
+	case GNU_PROPERTY_X86_FEATURE_2_CODE16:
+	  printf ("CODE16");
+	  break;
 	default:
 	  printf (_("<unknown: %x>"), bit);
 	  break;
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index d3441988e34..c17f4da16fe 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2695,6 +2695,10 @@ static void
 set_code_flag (int value)
 {
   update_code_flag (value, 0);
+#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
+  if (value == CODE_16BIT)
+    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
+#endif
 }
 
 static void
@@ -2706,6 +2710,10 @@ set_16bit_gcc_code_flag (int new_code_flag)
   cpu_arch_flags.bitfield.cpu64 = 0;
   cpu_arch_flags.bitfield.cpuno64 = 1;
   stackop_size = LONG_MNEM_SUFFIX;
+#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
+  if (new_code_flag == CODE_16BIT)
+    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
+#endif
 }
 
 static void
@@ -9032,7 +9040,7 @@ x86_cleanup (void)
   unsigned int isa_1_descsz_raw, feature_2_descsz_raw;
   unsigned int padding;
 
-  if (!IS_ELF || !x86_used_note)
+  if (!IS_ELF || (!x86_used_note && !x86_feature_2_used))
     return;
 
   x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_X86;
@@ -9072,15 +9080,23 @@ x86_cleanup (void)
   bfd_set_section_alignment (sec, alignment);
   elf_section_type (sec) = SHT_NOTE;
 
-  /* GNU_PROPERTY_X86_ISA_1_USED: 4-byte type + 4-byte data size
-				  + 4-byte data  */
-  isa_1_descsz_raw = 4 + 4 + 4;
-  /* Align GNU_PROPERTY_X86_ISA_1_USED.  */
-  isa_1_descsz = (isa_1_descsz_raw + align_size_1) & ~align_size_1;
+  if (x86_used_note)
+    {
+      /* GNU_PROPERTY_X86_ISA_1_USED: 4-byte type + 4-byte data size
+	 + 4-byte data  */
+      isa_1_descsz_raw = 4 + 4 + 4;
+      /* Align GNU_PROPERTY_X86_ISA_1_USED.  */
+      isa_1_descsz = (isa_1_descsz_raw + align_size_1) & ~align_size_1;
+    }
+  else
+    {
+      isa_1_descsz_raw = 0;
+      isa_1_descsz = 0;
+    }
 
   feature_2_descsz_raw = isa_1_descsz;
   /* GNU_PROPERTY_X86_FEATURE_2_USED: 4-byte type + 4-byte data size
-				      + 4-byte data  */
+     + 4-byte data  */
   feature_2_descsz_raw += 4 + 4 + 4;
   /* Align GNU_PROPERTY_X86_FEATURE_2_USED.  */
   feature_2_descsz = ((feature_2_descsz_raw + align_size_1)
@@ -9102,20 +9118,23 @@ x86_cleanup (void)
   /* Write n_name.  */
   memcpy (p + 4 * 3, "GNU", 4);
 
-  /* Write 4-byte type.  */
-  md_number_to_chars (p + 4 * 4,
-		      (valueT) GNU_PROPERTY_X86_ISA_1_USED, 4);
+  if (isa_1_descsz != 0)
+    {
+      /* Write 4-byte type.  */
+      md_number_to_chars (p + 4 * 4,
+			  (valueT) GNU_PROPERTY_X86_ISA_1_USED, 4);
 
-  /* Write 4-byte data size.  */
-  md_number_to_chars (p + 4 * 5, (valueT) 4, 4);
+      /* Write 4-byte data size.  */
+      md_number_to_chars (p + 4 * 5, (valueT) 4, 4);
 
-  /* Write 4-byte data.  */
-  md_number_to_chars (p + 4 * 6, (valueT) x86_isa_1_used, 4);
+      /* Write 4-byte data.  */
+      md_number_to_chars (p + 4 * 6, (valueT) x86_isa_1_used, 4);
 
-  /* Zero out paddings.  */
-  padding = isa_1_descsz - isa_1_descsz_raw;
-  if (padding)
-    memset (p + 4 * 7, 0, padding);
+      /* Zero out paddings.  */
+      padding = isa_1_descsz - isa_1_descsz_raw;
+      if (padding)
+	memset (p + 4 * 7, 0, padding);
+    }
 
   /* Write 4-byte type.  */
   md_number_to_chars (p + isa_1_descsz + 4 * 4,
diff --git a/gas/testsuite/gas/i386/code16-2.d b/gas/testsuite/gas/i386/code16-2.d
new file mode 100644
index 00000000000..37b66c85f4e
--- /dev/null
+++ b/gas/testsuite/gas/i386/code16-2.d
@@ -0,0 +1,8 @@
+#name: i386 code16 2
+#as: -mx86-used-note=no --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 feature used: x86, CODE16
diff --git a/gas/testsuite/gas/i386/code16-2.s b/gas/testsuite/gas/i386/code16-2.s
new file mode 100644
index 00000000000..66e5d558791
--- /dev/null
+++ b/gas/testsuite/gas/i386/code16-2.s
@@ -0,0 +1,10 @@
+	.code16gcc
+	.text
+	.section	.text.default_process_op.isra.0,"ax",@progbits
+	.type	default_process_op.isra.0, @function
+default_process_op.isra.0:
+	ret
+	.section	.text.mpt_scsi_process_op,"ax",@progbits
+	.type	mpt_scsi_process_op, @function
+mpt_scsi_process_op:
+	jmp	default_process_op.isra.0
diff --git a/gas/testsuite/gas/i386/i386.exp b/gas/testsuite/gas/i386/i386.exp
index 39010bdd500..a459c6fe392 100644
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -734,6 +734,7 @@ if {[is_elf_format] || [istarget "*-*-vxworks*"]} then {
     run_dump_test "property-ldmxcsr"
     run_dump_test "property-vldmxcsr"
     run_dump_test "property-vzeroall"
+    run_dump_test "code16-2"
 
     if {![istarget "*-*-dragonfly*"]
 	&& ![istarget "*-*-gnu*"]
@@ -1298,6 +1299,7 @@ if [gas_64_check] then {
 	run_dump_test "x86-64-property-8"
 	run_dump_test "x86-64-property-9"
 	run_dump_test "x86-64-property-14"
+	run_dump_test "x86-64-code16-2"
 
 	if {[istarget "*-*-linux*"]} then {
 	    run_dump_test "x86-64-align-branch-3"
diff --git a/gas/testsuite/gas/i386/x86-64-code16-2.d b/gas/testsuite/gas/i386/x86-64-code16-2.d
new file mode 100644
index 00000000000..dbabd67e888
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-code16-2.d
@@ -0,0 +1,9 @@
+#source: code16-2.s
+#name: x86-64 code16 2
+#as: -mx86-used-note=no --generate-missing-build-notes=no
+#readelf: -n
+
+Displaying notes found in: .note.gnu.property
+[ 	]+Owner[ 	]+Data size[ 	]+Description
+  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
+      Properties: x86 feature used: x86, CODE16
diff --git a/include/elf/common.h b/include/elf/common.h
index 24d0a09b7c8..564ab711a20 100644
--- a/include/elf/common.h
+++ b/include/elf/common.h
@@ -872,6 +872,7 @@
 #define GNU_PROPERTY_X86_FEATURE_2_XSAVEC	(1U << 9)
 #define GNU_PROPERTY_X86_FEATURE_2_TMM		(1U << 10)
 #define GNU_PROPERTY_X86_FEATURE_2_MASK		(1U << 11)
+#define GNU_PROPERTY_X86_FEATURE_2_CODE16	(1U << 12)
 
 #define GNU_PROPERTY_X86_COMPAT_2_ISA_1_NEEDED \
   (GNU_PROPERTY_X86_UINT32_OR_LO + 0)
diff --git a/ld/testsuite/ld-i386/code16.d b/ld/testsuite/ld-i386/code16.d
new file mode 100644
index 00000000000..8b67861db91
--- /dev/null
+++ b/ld/testsuite/ld-i386/code16.d
@@ -0,0 +1,19 @@
+#name: i386 R_386_PC16 reloc in 16-bit mode
+#as: --32 -mx86-used-note=no --generate-missing-build-notes=no
+#source: ${srcdir}/../../../gas/testsuite/gas/i386/code16-2.s
+#ld: -T code16.t
+#objdump: -dw -Mi8086
+
+.*: +file format .*
+
+
+Disassembly of section .text.default_process_op.isra.0:
+
+0+737c <default_process_op.isra.0>:
+ +[a-f0-9]+:	66 c3                	retl   
+
+Disassembly of section .text.mpt_scsi_process_op:
+
+0+f869 <mpt_scsi_process_op>:
+ +[a-f0-9]+:	e9 10 7b             	jmp    737c <default_process_op.isra.0>
+#pass
diff --git a/ld/testsuite/ld-i386/code16.t b/ld/testsuite/ld-i386/code16.t
new file mode 100644
index 00000000000..0cf99042e40
--- /dev/null
+++ b/ld/testsuite/ld-i386/code16.t
@@ -0,0 +1,7 @@
+OUTPUT_FORMAT("elf32-i386")
+OUTPUT_ARCH("i386")
+SECTIONS
+{
+.text.default_process_op.isra.0 0x737c : { *(.text.default_process_op.isra.0) }
+.text.mpt_scsi_process_op 0xf869 : { *(.text.mpt_scsi_process_op) }
+}
diff --git a/ld/testsuite/ld-i386/i386.exp b/ld/testsuite/ld-i386/i386.exp
index d0b3f69fb8d..3d6047b790d 100644
--- a/ld/testsuite/ld-i386/i386.exp
+++ b/ld/testsuite/ld-i386/i386.exp
@@ -492,6 +492,7 @@ run_dump_test "property-x86-isa2"
 run_dump_test "property-x86-isa3"
 run_dump_test "property-x86-isa4"
 run_dump_test "pr26869"
+run_dump_test "code16"
 
 if { !([istarget "i?86-*-linux*"]
        || [istarget "i?86-*-gnu*"]
diff --git a/ld/testsuite/ld-x86-64/code16.d b/ld/testsuite/ld-x86-64/code16.d
new file mode 100644
index 00000000000..20096ab6abf
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/code16.d
@@ -0,0 +1,19 @@
+#name: x86-64 R_X86_64_PC16 reloc in 16-bit mode
+#as: --64 -mx86-used-note=no --generate-missing-build-notes=no
+#source: ${srcdir}/../../../gas/testsuite/gas/i386/code16-2.s
+#ld: -T code16.t
+#objdump: -dw -Mi8086
+
+.*: +file format .*
+
+
+Disassembly of section .text.default_process_op.isra.0:
+
+0+737c <default_process_op.isra.0>:
+ +[a-f0-9]+:	66 c3                	retl   
+
+Disassembly of section .text.mpt_scsi_process_op:
+
+0+f869 <mpt_scsi_process_op>:
+ +[a-f0-9]+:	e9 10 7b             	jmp    737c <default_process_op.isra.0>
+#pass
diff --git a/ld/testsuite/ld-x86-64/code16.t b/ld/testsuite/ld-x86-64/code16.t
new file mode 100644
index 00000000000..9ef00a3404d
--- /dev/null
+++ b/ld/testsuite/ld-x86-64/code16.t
@@ -0,0 +1,7 @@
+OUTPUT_FORMAT("elf64-x86-64")
+OUTPUT_ARCH("i386:x86-64")
+SECTIONS
+{
+.text.default_process_op.isra.0 0x737c : { *(.text.default_process_op.isra.0) }
+.text.mpt_scsi_process_op 0xf869 : { *(.text.mpt_scsi_process_op) }
+}
diff --git a/ld/testsuite/ld-x86-64/x86-64.exp b/ld/testsuite/ld-x86-64/x86-64.exp
index 37cf998252c..80716668df6 100644
--- a/ld/testsuite/ld-x86-64/x86-64.exp
+++ b/ld/testsuite/ld-x86-64/x86-64.exp
@@ -474,6 +474,7 @@ run_dump_test "property-x86-isa3"
 run_dump_test "property-x86-isa3-x32"
 run_dump_test "property-x86-isa4"
 run_dump_test "property-x86-isa4-x32"
+run_dump_test "code16"
 
 if ![istarget "x86_64-*-linux*"] {
     return
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-26 14:16                 ` [PATCH v3] " H.J. Lu
@ 2021-05-27  7:10                   ` Jan Beulich
  2021-05-27 12:29                     ` H.J. Lu
  0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2021-05-27  7:10 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Binutils, Martin Liška

On 26.05.2021 16:16, H.J. Lu wrote:
> On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 25.05.2021 18:11, H.J. Lu wrote:
>>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>
>>>>> On 25.05.2021 17:40, H.J. Lu wrote:
>>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>
>>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
>>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>
>>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>>>
>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>>>
>>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
>>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
>>>>>>>>>>
>>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
>>>>>>>>>>
>>>>>>>>>> as in
>>>>>>>>>>
>>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
>>>>>>>>>>
>>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
>>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
>>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
>>>>>>>>>
>>>>>>>>> I certainly don't mind the introduction of this flag; I think its
>>>>>>>>> introduction wants to be separated from the specific use in the
>>>>>>>>> linker, not the lease because ...
>>>>>>>>>
>>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
>>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
>>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
>>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
>>>>>>>>>
>>>>>>>>> ... I don't think this is an appropriate step to take. The majority
>>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
>>>>>>>>> is just a small portion of code, and the rest of the code is 32-
>>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
>>>>>>>>> as indication to relax overflow checking, you undermine the main
>>>>>>>>> goal of the earlier change.
>>>>>>>>
>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>
>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>
>>>>>>>> is technically correct according to psABIs.  But GNU assembler
>>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
>>>>>>>> we never ran into any PC16 relocation overflow before.  My change
>>>>>>>> restores the old behavior only when input has 16-bit codes.
>>>>>>>
>>>>>>> When paying specific attention to resulting code size, people may
>>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
>>>>>>> code. This not having worked properly is what initially triggered
>>>>>>> me touching this area. And of course assembler programmers could
>>>>>>> even cause data to have PC16 relocations attached to it.
>>>>>>
>>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
>>>>>> changed.
>>>>>
>>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
>>>>> contain small pieces of 16-bit code.
>>>>
>>>> And we never ran into any issues before.  My patch just restores
>>>> the old behavior for them.
>>>
>>> And my point is that the old behavior was wrong (and will be again if
>>> we go with your change).
>>
>> The odd behavior is correct for 16-bit codes.  My patch restores the
>> old behavior only if input has 16-bit codes.
>>
> 
> This is the patch I am going to check in.

Sadly you've sent all but v1 only as attachments, making commenting
difficult. There are issues:
1) The workaround gets triggered from the handling of the actual
.code* directives, so would come into effect even if no insn at all
was emitted. Imo this should be tied to the specific case of a PC16
relocation getting emitted for 16-bit code. This would also take
care of 4) below.
2) Object files created by other tools (including older gas, which
would include an incremental build after having updated binutils),
or created with that note's emission suppressed (which, as said in
bug 27753, shouldn't be emitted by default anyway) will still cause
problems.
3) Imo the workaround would better be limited to things living in
the low 64k of address space. This would in particular avoid
negatively affecting OS kernels and alike, which typically get
linked for a (large) non-zero base address. Albeit I admit there
would be (theoretical) cases remaining where an overflow might
still get wrongly diagnosed.
4) Whether .code16gcc should trigger the workaround is at least
questionable. In this mode, all stack related operations are 32-bit
ones, and hence only JMP and Jcc (but not CALL) would require the
marker flag to get set.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-27  7:10                   ` Jan Beulich
@ 2021-05-27 12:29                     ` H.J. Lu
  2021-05-27 12:50                       ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-05-27 12:29 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils, Martin Liška

On Thu, May 27, 2021 at 12:10 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 26.05.2021 16:16, H.J. Lu wrote:
> > On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>
> >> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>
> >>> On 25.05.2021 18:11, H.J. Lu wrote:
> >>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>
> >>>>> On 25.05.2021 17:40, H.J. Lu wrote:
> >>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>
> >>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
> >>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>
> >>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> >>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>>>>>>
> >>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>>>>>>
> >>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
> >>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> >>>>>>>>>>
> >>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> >>>>>>>>>>
> >>>>>>>>>> as in
> >>>>>>>>>>
> >>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> >>>>>>>>>>
> >>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
> >>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
> >>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
> >>>>>>>>>
> >>>>>>>>> I certainly don't mind the introduction of this flag; I think its
> >>>>>>>>> introduction wants to be separated from the specific use in the
> >>>>>>>>> linker, not the lease because ...
> >>>>>>>>>
> >>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
> >>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> >>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> >>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
> >>>>>>>>>
> >>>>>>>>> ... I don't think this is an appropriate step to take. The majority
> >>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
> >>>>>>>>> is just a small portion of code, and the rest of the code is 32-
> >>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
> >>>>>>>>> as indication to relax overflow checking, you undermine the main
> >>>>>>>>> goal of the earlier change.
> >>>>>>>>
> >>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>>>>
> >>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>>>>
> >>>>>>>> is technically correct according to psABIs.  But GNU assembler
> >>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
> >>>>>>>> we never ran into any PC16 relocation overflow before.  My change
> >>>>>>>> restores the old behavior only when input has 16-bit codes.
> >>>>>>>
> >>>>>>> When paying specific attention to resulting code size, people may
> >>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
> >>>>>>> code. This not having worked properly is what initially triggered
> >>>>>>> me touching this area. And of course assembler programmers could
> >>>>>>> even cause data to have PC16 relocations attached to it.
> >>>>>>
> >>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
> >>>>>> changed.
> >>>>>
> >>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
> >>>>> contain small pieces of 16-bit code.
> >>>>
> >>>> And we never ran into any issues before.  My patch just restores
> >>>> the old behavior for them.
> >>>
> >>> And my point is that the old behavior was wrong (and will be again if
> >>> we go with your change).
> >>
> >> The odd behavior is correct for 16-bit codes.  My patch restores the
> >> old behavior only if input has 16-bit codes.
> >>
> >
> > This is the patch I am going to check in.
>
> Sadly you've sent all but v1 only as attachments, making commenting
> difficult. There are issues:
> 1) The workaround gets triggered from the handling of the actual
> .code* directives, so would come into effect even if no insn at all
> was emitted. Imo this should be tied to the specific case of a PC16
> relocation getting emitted for 16-bit code. This would also take
> care of 4) below.

We can do this:

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index c17f4da16fe..8adf0fce0a6 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2695,10 +2695,6 @@ static void
 set_code_flag (int value)
 {
   update_code_flag (value, 0);
-#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
-  if (value == CODE_16BIT)
-    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
-#endif
 }

 static void
@@ -2710,10 +2706,6 @@ set_16bit_gcc_code_flag (int new_code_flag)
   cpu_arch_flags.bitfield.cpu64 = 0;
   cpu_arch_flags.bitfield.cpuno64 = 1;
   stackop_size = LONG_MNEM_SUFFIX;
-#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
-  if (new_code_flag == CODE_16BIT)
-    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
-#endif
 }

 static void
@@ -8784,6 +8776,10 @@ output_branch (void)
   else
     subtype = ENCODE_RELAX_STATE (COND_JUMP86, size);
   subtype |= code16;
+#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
+  if (code16)
+    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
+#endif

   sym = i.op[0].disps->X_add_symbol;
   off = i.op[0].disps->X_add_number;

> 2) Object files created by other tools (including older gas, which
> would include an incremental build after having updated binutils),
> or created with that note's emission suppressed (which, as said in
> bug 27753, shouldn't be emitted by default anyway) will still cause
> problems.

GNU_PROPERTY_X86_FEATURE_2_CODE16 can't not be
suppressed.  But you can strip it at link-time.

> 3) Imo the workaround would better be limited to things living in
> the low 64k of address space. This would in particular avoid
> negatively affecting OS kernels and alike, which typically get
> linked for a (large) non-zero base address. Albeit I admit there
> would be (theoretical) cases remaining where an overflow might
> still get wrongly diagnosed.
> 4) Whether .code16gcc should trigger the workaround is at least
> questionable. In this mode, all stack related operations are 32-bit
> ones, and hence only JMP and Jcc (but not CALL) would require the
> marker flag to get set.
>
> Jan



-- 
H.J.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-27 12:29                     ` H.J. Lu
@ 2021-05-27 12:50                       ` Jan Beulich
  2021-05-27 13:08                         ` H.J. Lu
  0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2021-05-27 12:50 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Binutils, Martin Liška

On 27.05.2021 14:29, H.J. Lu wrote:
> On Thu, May 27, 2021 at 12:10 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 26.05.2021 16:16, H.J. Lu wrote:
>>> On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>
>>>> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>
>>>>> On 25.05.2021 18:11, H.J. Lu wrote:
>>>>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>
>>>>>>> On 25.05.2021 17:40, H.J. Lu wrote:
>>>>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>
>>>>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
>>>>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>>>>>
>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>>>>>
>>>>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
>>>>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
>>>>>>>>>>>>
>>>>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
>>>>>>>>>>>>
>>>>>>>>>>>> as in
>>>>>>>>>>>>
>>>>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
>>>>>>>>>>>>
>>>>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
>>>>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
>>>>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
>>>>>>>>>>>
>>>>>>>>>>> I certainly don't mind the introduction of this flag; I think its
>>>>>>>>>>> introduction wants to be separated from the specific use in the
>>>>>>>>>>> linker, not the lease because ...
>>>>>>>>>>>
>>>>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
>>>>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
>>>>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
>>>>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
>>>>>>>>>>>
>>>>>>>>>>> ... I don't think this is an appropriate step to take. The majority
>>>>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
>>>>>>>>>>> is just a small portion of code, and the rest of the code is 32-
>>>>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
>>>>>>>>>>> as indication to relax overflow checking, you undermine the main
>>>>>>>>>>> goal of the earlier change.
>>>>>>>>>>
>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>>>
>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>>>
>>>>>>>>>> is technically correct according to psABIs.  But GNU assembler
>>>>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
>>>>>>>>>> we never ran into any PC16 relocation overflow before.  My change
>>>>>>>>>> restores the old behavior only when input has 16-bit codes.
>>>>>>>>>
>>>>>>>>> When paying specific attention to resulting code size, people may
>>>>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
>>>>>>>>> code. This not having worked properly is what initially triggered
>>>>>>>>> me touching this area. And of course assembler programmers could
>>>>>>>>> even cause data to have PC16 relocations attached to it.
>>>>>>>>
>>>>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
>>>>>>>> changed.
>>>>>>>
>>>>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
>>>>>>> contain small pieces of 16-bit code.
>>>>>>
>>>>>> And we never ran into any issues before.  My patch just restores
>>>>>> the old behavior for them.
>>>>>
>>>>> And my point is that the old behavior was wrong (and will be again if
>>>>> we go with your change).
>>>>
>>>> The odd behavior is correct for 16-bit codes.  My patch restores the
>>>> old behavior only if input has 16-bit codes.
>>>>
>>>
>>> This is the patch I am going to check in.
>>
>> Sadly you've sent all but v1 only as attachments, making commenting
>> difficult. There are issues:
>> 1) The workaround gets triggered from the handling of the actual
>> .code* directives, so would come into effect even if no insn at all
>> was emitted. Imo this should be tied to the specific case of a PC16
>> relocation getting emitted for 16-bit code. This would also take
>> care of 4) below.
> 
> We can do this:
> 
> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> index c17f4da16fe..8adf0fce0a6 100644
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -2695,10 +2695,6 @@ static void
>  set_code_flag (int value)
>  {
>    update_code_flag (value, 0);
> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> -  if (value == CODE_16BIT)
> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> -#endif
>  }
> 
>  static void
> @@ -2710,10 +2706,6 @@ set_16bit_gcc_code_flag (int new_code_flag)
>    cpu_arch_flags.bitfield.cpu64 = 0;
>    cpu_arch_flags.bitfield.cpuno64 = 1;
>    stackop_size = LONG_MNEM_SUFFIX;
> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> -  if (new_code_flag == CODE_16BIT)
> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> -#endif
>  }
> 
>  static void
> @@ -8784,6 +8776,10 @@ output_branch (void)
>    else
>      subtype = ENCODE_RELAX_STATE (COND_JUMP86, size);
>    subtype |= code16;
> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> +  if (code16)
> +    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> +#endif

This would get us closer to what I have described, but doesn't go
quite as far. However, GNU_PROPERTY_X86_FEATURE_2_CODE16 then is
misleading and would probably want naming (and describing)
differently.

>> 2) Object files created by other tools (including older gas, which
>> would include an incremental build after having updated binutils),
>> or created with that note's emission suppressed (which, as said in
>> bug 27753, shouldn't be emitted by default anyway) will still cause
>> problems.
> 
> GNU_PROPERTY_X86_FEATURE_2_CODE16 can't not be
> suppressed.

Of course it can, via -mx86-used-note=no. Or did you change this
kind of silently as a "side effect" of your patch, without me
even noticing? That would be evil - as said earlier (and reported
as a bug), these notes shouldn't be there by default; even less
so should they be there unconditionally.

Plus even if it couldn't be suppressed, there would still be the
issue with object files created by other / older tools.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-27 12:50                       ` Jan Beulich
@ 2021-05-27 13:08                         ` H.J. Lu
  2021-05-27 13:15                           ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-05-27 13:08 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils, Martin Liška

On Thu, May 27, 2021 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 27.05.2021 14:29, H.J. Lu wrote:
> > On Thu, May 27, 2021 at 12:10 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 26.05.2021 16:16, H.J. Lu wrote:
> >>> On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>
> >>>> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>
> >>>>> On 25.05.2021 18:11, H.J. Lu wrote:
> >>>>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>
> >>>>>>> On 25.05.2021 17:40, H.J. Lu wrote:
> >>>>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>
> >>>>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
> >>>>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> >>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>>>>>>>>
> >>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>>>>>>>>
> >>>>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
> >>>>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> >>>>>>>>>>>>
> >>>>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> >>>>>>>>>>>>
> >>>>>>>>>>>> as in
> >>>>>>>>>>>>
> >>>>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> >>>>>>>>>>>>
> >>>>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
> >>>>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
> >>>>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
> >>>>>>>>>>>
> >>>>>>>>>>> I certainly don't mind the introduction of this flag; I think its
> >>>>>>>>>>> introduction wants to be separated from the specific use in the
> >>>>>>>>>>> linker, not the lease because ...
> >>>>>>>>>>>
> >>>>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
> >>>>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> >>>>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> >>>>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
> >>>>>>>>>>>
> >>>>>>>>>>> ... I don't think this is an appropriate step to take. The majority
> >>>>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
> >>>>>>>>>>> is just a small portion of code, and the rest of the code is 32-
> >>>>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
> >>>>>>>>>>> as indication to relax overflow checking, you undermine the main
> >>>>>>>>>>> goal of the earlier change.
> >>>>>>>>>>
> >>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>>>>>>
> >>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>>>>>>
> >>>>>>>>>> is technically correct according to psABIs.  But GNU assembler
> >>>>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
> >>>>>>>>>> we never ran into any PC16 relocation overflow before.  My change
> >>>>>>>>>> restores the old behavior only when input has 16-bit codes.
> >>>>>>>>>
> >>>>>>>>> When paying specific attention to resulting code size, people may
> >>>>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
> >>>>>>>>> code. This not having worked properly is what initially triggered
> >>>>>>>>> me touching this area. And of course assembler programmers could
> >>>>>>>>> even cause data to have PC16 relocations attached to it.
> >>>>>>>>
> >>>>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
> >>>>>>>> changed.
> >>>>>>>
> >>>>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
> >>>>>>> contain small pieces of 16-bit code.
> >>>>>>
> >>>>>> And we never ran into any issues before.  My patch just restores
> >>>>>> the old behavior for them.
> >>>>>
> >>>>> And my point is that the old behavior was wrong (and will be again if
> >>>>> we go with your change).
> >>>>
> >>>> The odd behavior is correct for 16-bit codes.  My patch restores the
> >>>> old behavior only if input has 16-bit codes.
> >>>>
> >>>
> >>> This is the patch I am going to check in.
> >>
> >> Sadly you've sent all but v1 only as attachments, making commenting
> >> difficult. There are issues:
> >> 1) The workaround gets triggered from the handling of the actual
> >> .code* directives, so would come into effect even if no insn at all
> >> was emitted. Imo this should be tied to the specific case of a PC16
> >> relocation getting emitted for 16-bit code. This would also take
> >> care of 4) below.
> >
> > We can do this:
> >
> > diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> > index c17f4da16fe..8adf0fce0a6 100644
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -2695,10 +2695,6 @@ static void
> >  set_code_flag (int value)
> >  {
> >    update_code_flag (value, 0);
> > -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> > -  if (value == CODE_16BIT)
> > -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> > -#endif
> >  }
> >
> >  static void
> > @@ -2710,10 +2706,6 @@ set_16bit_gcc_code_flag (int new_code_flag)
> >    cpu_arch_flags.bitfield.cpu64 = 0;
> >    cpu_arch_flags.bitfield.cpuno64 = 1;
> >    stackop_size = LONG_MNEM_SUFFIX;
> > -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> > -  if (new_code_flag == CODE_16BIT)
> > -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> > -#endif
> >  }
> >
> >  static void
> > @@ -8784,6 +8776,10 @@ output_branch (void)
> >    else
> >      subtype = ENCODE_RELAX_STATE (COND_JUMP86, size);
> >    subtype |= code16;
> > +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> > +  if (code16)
> > +    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> > +#endif
>
> This would get us closer to what I have described, but doesn't go
> quite as far. However, GNU_PROPERTY_X86_FEATURE_2_CODE16 then is
> misleading and would probably want naming (and describing)
> differently.

Do you have any suggestions?

> >> 2) Object files created by other tools (including older gas, which
> >> would include an incremental build after having updated binutils),
> >> or created with that note's emission suppressed (which, as said in
> >> bug 27753, shouldn't be emitted by default anyway) will still cause
> >> problems.
> >
> > GNU_PROPERTY_X86_FEATURE_2_CODE16 can't not be
> > suppressed.
>
> Of course it can, via -mx86-used-note=no. Or did you change this
> kind of silently as a "side effect" of your patch, without me
> even noticing? That would be evil - as said earlier (and reported
> as a bug), these notes shouldn't be there by default; even less
> so should they be there unconditionally.

GNU_PROPERTY_X86_FEATURE_2_CODE16 is mandatory.
Otherwise, it won't work for seabios.

> Plus even if it couldn't be suppressed, there would still be the
> issue with object files created by other / older tools.

This is a workaround/compromise.  There is no perfect solution.
It works for seabios.

-- 
H.J.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-27 13:08                         ` H.J. Lu
@ 2021-05-27 13:15                           ` Jan Beulich
  2021-05-27 13:46                             ` H.J. Lu
  0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2021-05-27 13:15 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Binutils, Martin Liška

On 27.05.2021 15:08, H.J. Lu wrote:
> On Thu, May 27, 2021 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 27.05.2021 14:29, H.J. Lu wrote:
>>> On Thu, May 27, 2021 at 12:10 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 26.05.2021 16:16, H.J. Lu wrote:
>>>>> On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>
>>>>>>> On 25.05.2021 18:11, H.J. Lu wrote:
>>>>>>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>
>>>>>>>>> On 25.05.2021 17:40, H.J. Lu wrote:
>>>>>>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
>>>>>>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
>>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
>>>>>>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> as in
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
>>>>>>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
>>>>>>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I certainly don't mind the introduction of this flag; I think its
>>>>>>>>>>>>> introduction wants to be separated from the specific use in the
>>>>>>>>>>>>> linker, not the lease because ...
>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
>>>>>>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
>>>>>>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
>>>>>>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
>>>>>>>>>>>>>
>>>>>>>>>>>>> ... I don't think this is an appropriate step to take. The majority
>>>>>>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
>>>>>>>>>>>>> is just a small portion of code, and the rest of the code is 32-
>>>>>>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
>>>>>>>>>>>>> as indication to relax overflow checking, you undermine the main
>>>>>>>>>>>>> goal of the earlier change.
>>>>>>>>>>>>
>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>>>>>
>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>>>>>
>>>>>>>>>>>> is technically correct according to psABIs.  But GNU assembler
>>>>>>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
>>>>>>>>>>>> we never ran into any PC16 relocation overflow before.  My change
>>>>>>>>>>>> restores the old behavior only when input has 16-bit codes.
>>>>>>>>>>>
>>>>>>>>>>> When paying specific attention to resulting code size, people may
>>>>>>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
>>>>>>>>>>> code. This not having worked properly is what initially triggered
>>>>>>>>>>> me touching this area. And of course assembler programmers could
>>>>>>>>>>> even cause data to have PC16 relocations attached to it.
>>>>>>>>>>
>>>>>>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
>>>>>>>>>> changed.
>>>>>>>>>
>>>>>>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
>>>>>>>>> contain small pieces of 16-bit code.
>>>>>>>>
>>>>>>>> And we never ran into any issues before.  My patch just restores
>>>>>>>> the old behavior for them.
>>>>>>>
>>>>>>> And my point is that the old behavior was wrong (and will be again if
>>>>>>> we go with your change).
>>>>>>
>>>>>> The odd behavior is correct for 16-bit codes.  My patch restores the
>>>>>> old behavior only if input has 16-bit codes.
>>>>>>
>>>>>
>>>>> This is the patch I am going to check in.
>>>>
>>>> Sadly you've sent all but v1 only as attachments, making commenting
>>>> difficult. There are issues:
>>>> 1) The workaround gets triggered from the handling of the actual
>>>> .code* directives, so would come into effect even if no insn at all
>>>> was emitted. Imo this should be tied to the specific case of a PC16
>>>> relocation getting emitted for 16-bit code. This would also take
>>>> care of 4) below.
>>>
>>> We can do this:
>>>
>>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
>>> index c17f4da16fe..8adf0fce0a6 100644
>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> @@ -2695,10 +2695,6 @@ static void
>>>  set_code_flag (int value)
>>>  {
>>>    update_code_flag (value, 0);
>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>>> -  if (value == CODE_16BIT)
>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
>>> -#endif
>>>  }
>>>
>>>  static void
>>> @@ -2710,10 +2706,6 @@ set_16bit_gcc_code_flag (int new_code_flag)
>>>    cpu_arch_flags.bitfield.cpu64 = 0;
>>>    cpu_arch_flags.bitfield.cpuno64 = 1;
>>>    stackop_size = LONG_MNEM_SUFFIX;
>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>>> -  if (new_code_flag == CODE_16BIT)
>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
>>> -#endif
>>>  }
>>>
>>>  static void
>>> @@ -8784,6 +8776,10 @@ output_branch (void)
>>>    else
>>>      subtype = ENCODE_RELAX_STATE (COND_JUMP86, size);
>>>    subtype |= code16;
>>> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>>> +  if (code16)
>>> +    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
>>> +#endif
>>
>> This would get us closer to what I have described, but doesn't go
>> quite as far. However, GNU_PROPERTY_X86_FEATURE_2_CODE16 then is
>> misleading and would probably want naming (and describing)
>> differently.
> 
> Do you have any suggestions?

GNU_PROPERTY_X86_FEATURE_2_BOGUS_PC16_RELOC plus further limiting
when the flag gets set (i.e. only when a PC16 reloc gets actually
generated).

>>>> 2) Object files created by other tools (including older gas, which
>>>> would include an incremental build after having updated binutils),
>>>> or created with that note's emission suppressed (which, as said in
>>>> bug 27753, shouldn't be emitted by default anyway) will still cause
>>>> problems.
>>>
>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 can't not be
>>> suppressed.
>>
>> Of course it can, via -mx86-used-note=no. Or did you change this
>> kind of silently as a "side effect" of your patch, without me
>> even noticing? That would be evil - as said earlier (and reported
>> as a bug), these notes shouldn't be there by default; even less
>> so should they be there unconditionally.
> 
> GNU_PROPERTY_X86_FEATURE_2_CODE16 is mandatory.
> Otherwise, it won't work for seabios.

Well, for both this and ...

>> Plus even if it couldn't be suppressed, there would still be the
>> issue with object files created by other / older tools.
> 
> This is a workaround/compromise.  There is no perfect solution.
> It works for seabios.

... this: Abusing relocations is something that may very well require
projects to adjust their build environment, e.g. by adding a command
line option to one of the tools involved.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-27 13:15                           ` Jan Beulich
@ 2021-05-27 13:46                             ` H.J. Lu
  2021-05-27 14:57                               ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-05-27 13:46 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils, Martin Liška

On Thu, May 27, 2021 at 6:15 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 27.05.2021 15:08, H.J. Lu wrote:
> > On Thu, May 27, 2021 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 27.05.2021 14:29, H.J. Lu wrote:
> >>> On Thu, May 27, 2021 at 12:10 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 26.05.2021 16:16, H.J. Lu wrote:
> >>>>> On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>>>
> >>>>>> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>
> >>>>>>> On 25.05.2021 18:11, H.J. Lu wrote:
> >>>>>>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>
> >>>>>>>>> On 25.05.2021 17:40, H.J. Lu wrote:
> >>>>>>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
> >>>>>>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> >>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
> >>>>>>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> as in
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
> >>>>>>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
> >>>>>>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I certainly don't mind the introduction of this flag; I think its
> >>>>>>>>>>>>> introduction wants to be separated from the specific use in the
> >>>>>>>>>>>>> linker, not the lease because ...
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
> >>>>>>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> >>>>>>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> >>>>>>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> ... I don't think this is an appropriate step to take. The majority
> >>>>>>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
> >>>>>>>>>>>>> is just a small portion of code, and the rest of the code is 32-
> >>>>>>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
> >>>>>>>>>>>>> as indication to relax overflow checking, you undermine the main
> >>>>>>>>>>>>> goal of the earlier change.
> >>>>>>>>>>>>
> >>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>>>>>>>>
> >>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>>>>>>>>
> >>>>>>>>>>>> is technically correct according to psABIs.  But GNU assembler
> >>>>>>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
> >>>>>>>>>>>> we never ran into any PC16 relocation overflow before.  My change
> >>>>>>>>>>>> restores the old behavior only when input has 16-bit codes.
> >>>>>>>>>>>
> >>>>>>>>>>> When paying specific attention to resulting code size, people may
> >>>>>>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
> >>>>>>>>>>> code. This not having worked properly is what initially triggered
> >>>>>>>>>>> me touching this area. And of course assembler programmers could
> >>>>>>>>>>> even cause data to have PC16 relocations attached to it.
> >>>>>>>>>>
> >>>>>>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
> >>>>>>>>>> changed.
> >>>>>>>>>
> >>>>>>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
> >>>>>>>>> contain small pieces of 16-bit code.
> >>>>>>>>
> >>>>>>>> And we never ran into any issues before.  My patch just restores
> >>>>>>>> the old behavior for them.
> >>>>>>>
> >>>>>>> And my point is that the old behavior was wrong (and will be again if
> >>>>>>> we go with your change).
> >>>>>>
> >>>>>> The odd behavior is correct for 16-bit codes.  My patch restores the
> >>>>>> old behavior only if input has 16-bit codes.
> >>>>>>
> >>>>>
> >>>>> This is the patch I am going to check in.
> >>>>
> >>>> Sadly you've sent all but v1 only as attachments, making commenting
> >>>> difficult. There are issues:
> >>>> 1) The workaround gets triggered from the handling of the actual
> >>>> .code* directives, so would come into effect even if no insn at all
> >>>> was emitted. Imo this should be tied to the specific case of a PC16
> >>>> relocation getting emitted for 16-bit code. This would also take
> >>>> care of 4) below.
> >>>
> >>> We can do this:
> >>>
> >>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> >>> index c17f4da16fe..8adf0fce0a6 100644
> >>> --- a/gas/config/tc-i386.c
> >>> +++ b/gas/config/tc-i386.c
> >>> @@ -2695,10 +2695,6 @@ static void
> >>>  set_code_flag (int value)
> >>>  {
> >>>    update_code_flag (value, 0);
> >>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> >>> -  if (value == CODE_16BIT)
> >>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> >>> -#endif
> >>>  }
> >>>
> >>>  static void
> >>> @@ -2710,10 +2706,6 @@ set_16bit_gcc_code_flag (int new_code_flag)
> >>>    cpu_arch_flags.bitfield.cpu64 = 0;
> >>>    cpu_arch_flags.bitfield.cpuno64 = 1;
> >>>    stackop_size = LONG_MNEM_SUFFIX;
> >>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> >>> -  if (new_code_flag == CODE_16BIT)
> >>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> >>> -#endif
> >>>  }
> >>>
> >>>  static void
> >>> @@ -8784,6 +8776,10 @@ output_branch (void)
> >>>    else
> >>>      subtype = ENCODE_RELAX_STATE (COND_JUMP86, size);
> >>>    subtype |= code16;
> >>> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> >>> +  if (code16)
> >>> +    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> >>> +#endif
> >>
> >> This would get us closer to what I have described, but doesn't go
> >> quite as far. However, GNU_PROPERTY_X86_FEATURE_2_CODE16 then is
> >> misleading and would probably want naming (and describing)
> >> differently.
> >
> > Do you have any suggestions?
>
> GNU_PROPERTY_X86_FEATURE_2_BOGUS_PC16_RELOC plus further limiting
> when the flag gets set (i.e. only when a PC16 reloc gets actually
> generated).

How about GNU_PROPERTY_X86_FEATURE_2_PC16_CODE16?
My proposed patch should be sufficient.  If not, do you have a testcase?

> >>>> 2) Object files created by other tools (including older gas, which
> >>>> would include an incremental build after having updated binutils),
> >>>> or created with that note's emission suppressed (which, as said in
> >>>> bug 27753, shouldn't be emitted by default anyway) will still cause
> >>>> problems.
> >>>
> >>> GNU_PROPERTY_X86_FEATURE_2_CODE16 can't not be
> >>> suppressed.
> >>
> >> Of course it can, via -mx86-used-note=no. Or did you change this
> >> kind of silently as a "side effect" of your patch, without me
> >> even noticing? That would be evil - as said earlier (and reported
> >> as a bug), these notes shouldn't be there by default; even less
> >> so should they be there unconditionally.
> >
> > GNU_PROPERTY_X86_FEATURE_2_CODE16 is mandatory.
> > Otherwise, it won't work for seabios.
>
> Well, for both this and ...
>
> >> Plus even if it couldn't be suppressed, there would still be the
> >> issue with object files created by other / older tools.
> >
> > This is a workaround/compromise.  There is no perfect solution.
> > It works for seabios.
>
> ... this: Abusing relocations is something that may very well require
> projects to adjust their build environment, e.g. by adding a command
> line option to one of the tools involved.

When I was reading the binutils source, my impression was that x86 PC16
relocations were intended for 16-bit codes in an ELF32/ELF64 container.
Whatever we do, we shouldn't break existing 16-bit programs.

-- 
H.J.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-27 13:46                             ` H.J. Lu
@ 2021-05-27 14:57                               ` Jan Beulich
  2021-05-27 15:53                                 ` H.J. Lu
  0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2021-05-27 14:57 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Binutils, Martin Liška

On 27.05.2021 15:46, H.J. Lu wrote:
> On Thu, May 27, 2021 at 6:15 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 27.05.2021 15:08, H.J. Lu wrote:
>>> On Thu, May 27, 2021 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 27.05.2021 14:29, H.J. Lu wrote:
>>>>> On Thu, May 27, 2021 at 12:10 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 26.05.2021 16:16, H.J. Lu wrote:
>>>>>>> On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>
>>>>>>>>> On 25.05.2021 18:11, H.J. Lu wrote:
>>>>>>>>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 25.05.2021 17:40, H.J. Lu wrote:
>>>>>>>>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
>>>>>>>>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
>>>>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
>>>>>>>>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> as in
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
>>>>>>>>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
>>>>>>>>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I certainly don't mind the introduction of this flag; I think its
>>>>>>>>>>>>>>> introduction wants to be separated from the specific use in the
>>>>>>>>>>>>>>> linker, not the lease because ...
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
>>>>>>>>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
>>>>>>>>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
>>>>>>>>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ... I don't think this is an appropriate step to take. The majority
>>>>>>>>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
>>>>>>>>>>>>>>> is just a small portion of code, and the rest of the code is 32-
>>>>>>>>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
>>>>>>>>>>>>>>> as indication to relax overflow checking, you undermine the main
>>>>>>>>>>>>>>> goal of the earlier change.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
>>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> is technically correct according to psABIs.  But GNU assembler
>>>>>>>>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
>>>>>>>>>>>>>> we never ran into any PC16 relocation overflow before.  My change
>>>>>>>>>>>>>> restores the old behavior only when input has 16-bit codes.
>>>>>>>>>>>>>
>>>>>>>>>>>>> When paying specific attention to resulting code size, people may
>>>>>>>>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
>>>>>>>>>>>>> code. This not having worked properly is what initially triggered
>>>>>>>>>>>>> me touching this area. And of course assembler programmers could
>>>>>>>>>>>>> even cause data to have PC16 relocations attached to it.
>>>>>>>>>>>>
>>>>>>>>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
>>>>>>>>>>>> changed.
>>>>>>>>>>>
>>>>>>>>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
>>>>>>>>>>> contain small pieces of 16-bit code.
>>>>>>>>>>
>>>>>>>>>> And we never ran into any issues before.  My patch just restores
>>>>>>>>>> the old behavior for them.
>>>>>>>>>
>>>>>>>>> And my point is that the old behavior was wrong (and will be again if
>>>>>>>>> we go with your change).
>>>>>>>>
>>>>>>>> The odd behavior is correct for 16-bit codes.  My patch restores the
>>>>>>>> old behavior only if input has 16-bit codes.
>>>>>>>>
>>>>>>>
>>>>>>> This is the patch I am going to check in.
>>>>>>
>>>>>> Sadly you've sent all but v1 only as attachments, making commenting
>>>>>> difficult. There are issues:
>>>>>> 1) The workaround gets triggered from the handling of the actual
>>>>>> .code* directives, so would come into effect even if no insn at all
>>>>>> was emitted. Imo this should be tied to the specific case of a PC16
>>>>>> relocation getting emitted for 16-bit code. This would also take
>>>>>> care of 4) below.
>>>>>
>>>>> We can do this:
>>>>>
>>>>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
>>>>> index c17f4da16fe..8adf0fce0a6 100644
>>>>> --- a/gas/config/tc-i386.c
>>>>> +++ b/gas/config/tc-i386.c
>>>>> @@ -2695,10 +2695,6 @@ static void
>>>>>  set_code_flag (int value)
>>>>>  {
>>>>>    update_code_flag (value, 0);
>>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>>>>> -  if (value == CODE_16BIT)
>>>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
>>>>> -#endif
>>>>>  }
>>>>>
>>>>>  static void
>>>>> @@ -2710,10 +2706,6 @@ set_16bit_gcc_code_flag (int new_code_flag)
>>>>>    cpu_arch_flags.bitfield.cpu64 = 0;
>>>>>    cpu_arch_flags.bitfield.cpuno64 = 1;
>>>>>    stackop_size = LONG_MNEM_SUFFIX;
>>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>>>>> -  if (new_code_flag == CODE_16BIT)
>>>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
>>>>> -#endif
>>>>>  }
>>>>>
>>>>>  static void
>>>>> @@ -8784,6 +8776,10 @@ output_branch (void)
>>>>>    else
>>>>>      subtype = ENCODE_RELAX_STATE (COND_JUMP86, size);
>>>>>    subtype |= code16;
>>>>> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>>>>> +  if (code16)
>>>>> +    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
>>>>> +#endif
>>>>
>>>> This would get us closer to what I have described, but doesn't go
>>>> quite as far. However, GNU_PROPERTY_X86_FEATURE_2_CODE16 then is
>>>> misleading and would probably want naming (and describing)
>>>> differently.
>>>
>>> Do you have any suggestions?
>>
>> GNU_PROPERTY_X86_FEATURE_2_BOGUS_PC16_RELOC plus further limiting
>> when the flag gets set (i.e. only when a PC16 reloc gets actually
>> generated).
> 
> How about GNU_PROPERTY_X86_FEATURE_2_PC16_CODE16?

Fine with me - I don't care much about the name as long as it's
not actively misleading.

> My proposed patch should be sufficient.  If not, do you have a testcase?

The decision whether to emit a relocation happens only during
relaxation, aiui. IOW it is my understanding that a forward
branch to a local symbol would also cause the flag to get set.

>>>>>> 2) Object files created by other tools (including older gas, which
>>>>>> would include an incremental build after having updated binutils),
>>>>>> or created with that note's emission suppressed (which, as said in
>>>>>> bug 27753, shouldn't be emitted by default anyway) will still cause
>>>>>> problems.
>>>>>
>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 can't not be
>>>>> suppressed.
>>>>
>>>> Of course it can, via -mx86-used-note=no. Or did you change this
>>>> kind of silently as a "side effect" of your patch, without me
>>>> even noticing? That would be evil - as said earlier (and reported
>>>> as a bug), these notes shouldn't be there by default; even less
>>>> so should they be there unconditionally.
>>>
>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 is mandatory.
>>> Otherwise, it won't work for seabios.
>>
>> Well, for both this and ...
>>
>>>> Plus even if it couldn't be suppressed, there would still be the
>>>> issue with object files created by other / older tools.
>>>
>>> This is a workaround/compromise.  There is no perfect solution.
>>> It works for seabios.
>>
>> ... this: Abusing relocations is something that may very well require
>> projects to adjust their build environment, e.g. by adding a command
>> line option to one of the tools involved.
> 
> When I was reading the binutils source, my impression was that x86 PC16
> relocations were intended for 16-bit codes in an ELF32/ELF64 container.
> Whatever we do, we shouldn't break existing 16-bit programs.

Well, if the treatment of PC16 isn't ABI-conforming, then perhaps
the ABI should be changed to express the intended behavior for this
reloc type (and make explicit that it's different from PC32 and PC8),
and there then perhaps ought to be a new reloc type with proper PC16
semantics (usable for 16-bit operand size XBEGIN as well as 16-bit
data items with PC-relative expressions)?

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-27 14:57                               ` Jan Beulich
@ 2021-05-27 15:53                                 ` H.J. Lu
  2021-05-27 16:05                                   ` H.J. Lu
  0 siblings, 1 reply; 19+ messages in thread
From: H.J. Lu @ 2021-05-27 15:53 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils, Martin Liška

On Thu, May 27, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 27.05.2021 15:46, H.J. Lu wrote:
> > On Thu, May 27, 2021 at 6:15 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 27.05.2021 15:08, H.J. Lu wrote:
> >>> On Thu, May 27, 2021 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 27.05.2021 14:29, H.J. Lu wrote:
> >>>>> On Thu, May 27, 2021 at 12:10 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>
> >>>>>> On 26.05.2021 16:16, H.J. Lu wrote:
> >>>>>>> On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>
> >>>>>>>>> On 25.05.2021 18:11, H.J. Lu wrote:
> >>>>>>>>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On 25.05.2021 17:40, H.J. Lu wrote:
> >>>>>>>>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
> >>>>>>>>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> >>>>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
> >>>>>>>>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> as in
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
> >>>>>>>>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
> >>>>>>>>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I certainly don't mind the introduction of this flag; I think its
> >>>>>>>>>>>>>>> introduction wants to be separated from the specific use in the
> >>>>>>>>>>>>>>> linker, not the lease because ...
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
> >>>>>>>>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> >>>>>>>>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> >>>>>>>>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> ... I don't think this is an appropriate step to take. The majority
> >>>>>>>>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
> >>>>>>>>>>>>>>> is just a small portion of code, and the rest of the code is 32-
> >>>>>>>>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
> >>>>>>>>>>>>>>> as indication to relax overflow checking, you undermine the main
> >>>>>>>>>>>>>>> goal of the earlier change.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> is technically correct according to psABIs.  But GNU assembler
> >>>>>>>>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
> >>>>>>>>>>>>>> we never ran into any PC16 relocation overflow before.  My change
> >>>>>>>>>>>>>> restores the old behavior only when input has 16-bit codes.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> When paying specific attention to resulting code size, people may
> >>>>>>>>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
> >>>>>>>>>>>>> code. This not having worked properly is what initially triggered
> >>>>>>>>>>>>> me touching this area. And of course assembler programmers could
> >>>>>>>>>>>>> even cause data to have PC16 relocations attached to it.
> >>>>>>>>>>>>
> >>>>>>>>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
> >>>>>>>>>>>> changed.
> >>>>>>>>>>>
> >>>>>>>>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
> >>>>>>>>>>> contain small pieces of 16-bit code.
> >>>>>>>>>>
> >>>>>>>>>> And we never ran into any issues before.  My patch just restores
> >>>>>>>>>> the old behavior for them.
> >>>>>>>>>
> >>>>>>>>> And my point is that the old behavior was wrong (and will be again if
> >>>>>>>>> we go with your change).
> >>>>>>>>
> >>>>>>>> The odd behavior is correct for 16-bit codes.  My patch restores the
> >>>>>>>> old behavior only if input has 16-bit codes.
> >>>>>>>>
> >>>>>>>
> >>>>>>> This is the patch I am going to check in.
> >>>>>>
> >>>>>> Sadly you've sent all but v1 only as attachments, making commenting
> >>>>>> difficult. There are issues:
> >>>>>> 1) The workaround gets triggered from the handling of the actual
> >>>>>> .code* directives, so would come into effect even if no insn at all
> >>>>>> was emitted. Imo this should be tied to the specific case of a PC16
> >>>>>> relocation getting emitted for 16-bit code. This would also take
> >>>>>> care of 4) below.
> >>>>>
> >>>>> We can do this:
> >>>>>
> >>>>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> >>>>> index c17f4da16fe..8adf0fce0a6 100644
> >>>>> --- a/gas/config/tc-i386.c
> >>>>> +++ b/gas/config/tc-i386.c
> >>>>> @@ -2695,10 +2695,6 @@ static void
> >>>>>  set_code_flag (int value)
> >>>>>  {
> >>>>>    update_code_flag (value, 0);
> >>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> >>>>> -  if (value == CODE_16BIT)
> >>>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> >>>>> -#endif
> >>>>>  }
> >>>>>
> >>>>>  static void
> >>>>> @@ -2710,10 +2706,6 @@ set_16bit_gcc_code_flag (int new_code_flag)
> >>>>>    cpu_arch_flags.bitfield.cpu64 = 0;
> >>>>>    cpu_arch_flags.bitfield.cpuno64 = 1;
> >>>>>    stackop_size = LONG_MNEM_SUFFIX;
> >>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> >>>>> -  if (new_code_flag == CODE_16BIT)
> >>>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> >>>>> -#endif
> >>>>>  }
> >>>>>
> >>>>>  static void
> >>>>> @@ -8784,6 +8776,10 @@ output_branch (void)
> >>>>>    else
> >>>>>      subtype = ENCODE_RELAX_STATE (COND_JUMP86, size);
> >>>>>    subtype |= code16;
> >>>>> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> >>>>> +  if (code16)
> >>>>> +    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> >>>>> +#endif
> >>>>
> >>>> This would get us closer to what I have described, but doesn't go
> >>>> quite as far. However, GNU_PROPERTY_X86_FEATURE_2_CODE16 then is
> >>>> misleading and would probably want naming (and describing)
> >>>> differently.
> >>>
> >>> Do you have any suggestions?
> >>
> >> GNU_PROPERTY_X86_FEATURE_2_BOGUS_PC16_RELOC plus further limiting
> >> when the flag gets set (i.e. only when a PC16 reloc gets actually
> >> generated).
> >
> > How about GNU_PROPERTY_X86_FEATURE_2_PC16_CODE16?
>
> Fine with me - I don't care much about the name as long as it's
> not actively misleading.
>
> > My proposed patch should be sufficient.  If not, do you have a testcase?
>
> The decision whether to emit a relocation happens only during
> relaxation, aiui. IOW it is my understanding that a forward
> branch to a local symbol would also cause the flag to get set.

I may be able to avoid it.  But ...

> >>>>>> 2) Object files created by other tools (including older gas, which
> >>>>>> would include an incremental build after having updated binutils),
> >>>>>> or created with that note's emission suppressed (which, as said in
> >>>>>> bug 27753, shouldn't be emitted by default anyway) will still cause
> >>>>>> problems.
> >>>>>
> >>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 can't not be
> >>>>> suppressed.
> >>>>
> >>>> Of course it can, via -mx86-used-note=no. Or did you change this
> >>>> kind of silently as a "side effect" of your patch, without me
> >>>> even noticing? That would be evil - as said earlier (and reported
> >>>> as a bug), these notes shouldn't be there by default; even less
> >>>> so should they be there unconditionally.
> >>>
> >>> GNU_PROPERTY_X86_FEATURE_2_CODE16 is mandatory.
> >>> Otherwise, it won't work for seabios.
> >>
> >> Well, for both this and ...
> >>
> >>>> Plus even if it couldn't be suppressed, there would still be the
> >>>> issue with object files created by other / older tools.
> >>>
> >>> This is a workaround/compromise.  There is no perfect solution.
> >>> It works for seabios.
> >>
> >> ... this: Abusing relocations is something that may very well require
> >> projects to adjust their build environment, e.g. by adding a command
> >> line option to one of the tools involved.
> >
> > When I was reading the binutils source, my impression was that x86 PC16
> > relocations were intended for 16-bit codes in an ELF32/ELF64 container.
> > Whatever we do, we shouldn't break existing 16-bit programs.
>
> Well, if the treatment of PC16 isn't ABI-conforming, then perhaps
> the ABI should be changed to express the intended behavior for this
> reloc type (and make explicit that it's different from PC32 and PC8),
> and there then perhaps ought to be a new reloc type with proper PC16
> semantics (usable for 16-bit operand size XBEGIN as well as 16-bit
> data items with PC-relative expressions)?

This sounds promising.  We can update the current PC16 description
for 16-bit objects.   We can add a new PC16 relocation for XBEGIN if
it is really desirable.  I will open an issue for this.

-- 
H.J.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions
  2021-05-27 15:53                                 ` H.J. Lu
@ 2021-05-27 16:05                                   ` H.J. Lu
  0 siblings, 0 replies; 19+ messages in thread
From: H.J. Lu @ 2021-05-27 16:05 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils, Martin Liška

On Thu, May 27, 2021 at 8:53 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, May 27, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 27.05.2021 15:46, H.J. Lu wrote:
> > > On Thu, May 27, 2021 at 6:15 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>
> > >> On 27.05.2021 15:08, H.J. Lu wrote:
> > >>> On Thu, May 27, 2021 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>
> > >>>> On 27.05.2021 14:29, H.J. Lu wrote:
> > >>>>> On Thu, May 27, 2021 at 12:10 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>>>
> > >>>>>> On 26.05.2021 16:16, H.J. Lu wrote:
> > >>>>>>> On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >>>>>>>>
> > >>>>>>>> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>>>>>>
> > >>>>>>>>> On 25.05.2021 18:11, H.J. Lu wrote:
> > >>>>>>>>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>>>>>>>>
> > >>>>>>>>>>> On 25.05.2021 17:40, H.J. Lu wrote:
> > >>>>>>>>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
> > >>>>>>>>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> > >>>>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> > >>>>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> > >>>>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
> > >>>>>>>>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>> as in
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
> > >>>>>>>>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
> > >>>>>>>>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> I certainly don't mind the introduction of this flag; I think its
> > >>>>>>>>>>>>>>> introduction wants to be separated from the specific use in the
> > >>>>>>>>>>>>>>> linker, not the lease because ...
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
> > >>>>>>>>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> > >>>>>>>>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> > >>>>>>>>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> ... I don't think this is an appropriate step to take. The majority
> > >>>>>>>>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
> > >>>>>>>>>>>>>>> is just a small portion of code, and the rest of the code is 32-
> > >>>>>>>>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
> > >>>>>>>>>>>>>>> as indication to relax overflow checking, you undermine the main
> > >>>>>>>>>>>>>>> goal of the earlier change.
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> > >>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> > >>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> is technically correct according to psABIs.  But GNU assembler
> > >>>>>>>>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
> > >>>>>>>>>>>>>> we never ran into any PC16 relocation overflow before.  My change
> > >>>>>>>>>>>>>> restores the old behavior only when input has 16-bit codes.
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> When paying specific attention to resulting code size, people may
> > >>>>>>>>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
> > >>>>>>>>>>>>> code. This not having worked properly is what initially triggered
> > >>>>>>>>>>>>> me touching this area. And of course assembler programmers could
> > >>>>>>>>>>>>> even cause data to have PC16 relocations attached to it.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
> > >>>>>>>>>>>> changed.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
> > >>>>>>>>>>> contain small pieces of 16-bit code.
> > >>>>>>>>>>
> > >>>>>>>>>> And we never ran into any issues before.  My patch just restores
> > >>>>>>>>>> the old behavior for them.
> > >>>>>>>>>
> > >>>>>>>>> And my point is that the old behavior was wrong (and will be again if
> > >>>>>>>>> we go with your change).
> > >>>>>>>>
> > >>>>>>>> The odd behavior is correct for 16-bit codes.  My patch restores the
> > >>>>>>>> old behavior only if input has 16-bit codes.
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> This is the patch I am going to check in.
> > >>>>>>
> > >>>>>> Sadly you've sent all but v1 only as attachments, making commenting
> > >>>>>> difficult. There are issues:
> > >>>>>> 1) The workaround gets triggered from the handling of the actual
> > >>>>>> .code* directives, so would come into effect even if no insn at all
> > >>>>>> was emitted. Imo this should be tied to the specific case of a PC16
> > >>>>>> relocation getting emitted for 16-bit code. This would also take
> > >>>>>> care of 4) below.
> > >>>>>
> > >>>>> We can do this:
> > >>>>>
> > >>>>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> > >>>>> index c17f4da16fe..8adf0fce0a6 100644
> > >>>>> --- a/gas/config/tc-i386.c
> > >>>>> +++ b/gas/config/tc-i386.c
> > >>>>> @@ -2695,10 +2695,6 @@ static void
> > >>>>>  set_code_flag (int value)
> > >>>>>  {
> > >>>>>    update_code_flag (value, 0);
> > >>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> > >>>>> -  if (value == CODE_16BIT)
> > >>>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> > >>>>> -#endif
> > >>>>>  }
> > >>>>>
> > >>>>>  static void
> > >>>>> @@ -2710,10 +2706,6 @@ set_16bit_gcc_code_flag (int new_code_flag)
> > >>>>>    cpu_arch_flags.bitfield.cpu64 = 0;
> > >>>>>    cpu_arch_flags.bitfield.cpuno64 = 1;
> > >>>>>    stackop_size = LONG_MNEM_SUFFIX;
> > >>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> > >>>>> -  if (new_code_flag == CODE_16BIT)
> > >>>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> > >>>>> -#endif
> > >>>>>  }
> > >>>>>
> > >>>>>  static void
> > >>>>> @@ -8784,6 +8776,10 @@ output_branch (void)
> > >>>>>    else
> > >>>>>      subtype = ENCODE_RELAX_STATE (COND_JUMP86, size);
> > >>>>>    subtype |= code16;
> > >>>>> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> > >>>>> +  if (code16)
> > >>>>> +    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> > >>>>> +#endif
> > >>>>
> > >>>> This would get us closer to what I have described, but doesn't go
> > >>>> quite as far. However, GNU_PROPERTY_X86_FEATURE_2_CODE16 then is
> > >>>> misleading and would probably want naming (and describing)
> > >>>> differently.
> > >>>
> > >>> Do you have any suggestions?
> > >>
> > >> GNU_PROPERTY_X86_FEATURE_2_BOGUS_PC16_RELOC plus further limiting
> > >> when the flag gets set (i.e. only when a PC16 reloc gets actually
> > >> generated).
> > >
> > > How about GNU_PROPERTY_X86_FEATURE_2_PC16_CODE16?
> >
> > Fine with me - I don't care much about the name as long as it's
> > not actively misleading.
> >
> > > My proposed patch should be sufficient.  If not, do you have a testcase?
> >
> > The decision whether to emit a relocation happens only during
> > relaxation, aiui. IOW it is my understanding that a forward
> > branch to a local symbol would also cause the flag to get set.
>
> I may be able to avoid it.  But ...
>
> > >>>>>> 2) Object files created by other tools (including older gas, which
> > >>>>>> would include an incremental build after having updated binutils),
> > >>>>>> or created with that note's emission suppressed (which, as said in
> > >>>>>> bug 27753, shouldn't be emitted by default anyway) will still cause
> > >>>>>> problems.
> > >>>>>
> > >>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 can't not be
> > >>>>> suppressed.
> > >>>>
> > >>>> Of course it can, via -mx86-used-note=no. Or did you change this
> > >>>> kind of silently as a "side effect" of your patch, without me
> > >>>> even noticing? That would be evil - as said earlier (and reported
> > >>>> as a bug), these notes shouldn't be there by default; even less
> > >>>> so should they be there unconditionally.
> > >>>
> > >>> GNU_PROPERTY_X86_FEATURE_2_CODE16 is mandatory.
> > >>> Otherwise, it won't work for seabios.
> > >>
> > >> Well, for both this and ...
> > >>
> > >>>> Plus even if it couldn't be suppressed, there would still be the
> > >>>> issue with object files created by other / older tools.
> > >>>
> > >>> This is a workaround/compromise.  There is no perfect solution.
> > >>> It works for seabios.
> > >>
> > >> ... this: Abusing relocations is something that may very well require
> > >> projects to adjust their build environment, e.g. by adding a command
> > >> line option to one of the tools involved.
> > >
> > > When I was reading the binutils source, my impression was that x86 PC16
> > > relocations were intended for 16-bit codes in an ELF32/ELF64 container.
> > > Whatever we do, we shouldn't break existing 16-bit programs.
> >
> > Well, if the treatment of PC16 isn't ABI-conforming, then perhaps
> > the ABI should be changed to express the intended behavior for this
> > reloc type (and make explicit that it's different from PC32 and PC8),
> > and there then perhaps ought to be a new reloc type with proper PC16
> > semantics (usable for 16-bit operand size XBEGIN as well as 16-bit
> > data items with PC-relative expressions)?
>
> This sounds promising.  We can update the current PC16 description
> for 16-bit objects.   We can add a new PC16 relocation for XBEGIN if
> it is really desirable.  I will open an issue for this.
>

The current psABI has

---
A program or object file using R_X86_64_8, R_X86_64_16, R_X86_64_PC16
or R_X86_64_PC8 relocations is not conformant to this ABI, these
relocations are only
added for documentation purposes.
----

I opened:

https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/7

I think both commits should be reverted.

-- 
H.J.

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-05-27 16:06 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-24 23:42 [PATCH] x86: Propery check PC16 reloc overflow in 16-bit mode instructions H.J. Lu
2021-05-25  6:40 ` Jan Beulich
2021-05-25 13:41   ` [PATCH v2] " H.J. Lu
2021-05-25 15:34     ` Jan Beulich
2021-05-25 15:40       ` H.J. Lu
2021-05-25 16:08         ` Jan Beulich
2021-05-25 16:11           ` H.J. Lu
2021-05-25 16:27             ` Jan Beulich
2021-05-25 16:31               ` H.J. Lu
2021-05-26 14:16                 ` [PATCH v3] " H.J. Lu
2021-05-27  7:10                   ` Jan Beulich
2021-05-27 12:29                     ` H.J. Lu
2021-05-27 12:50                       ` Jan Beulich
2021-05-27 13:08                         ` H.J. Lu
2021-05-27 13:15                           ` Jan Beulich
2021-05-27 13:46                             ` H.J. Lu
2021-05-27 14:57                               ` Jan Beulich
2021-05-27 15:53                                 ` H.J. Lu
2021-05-27 16:05                                   ` H.J. Lu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).