diff mbox series

Don't put objects with flexible array members in small data

Message ID f195ee57-4ca1-1f48-b67c-99dd252b0604@codesourcery.com
State New
Headers show
Series Don't put objects with flexible array members in small data | expand

Commit Message

Sandra Loosemore Nov. 20, 2019, 11:59 p.m. UTC 
This patch is for PR target/92499, a bug reported by GLIBC maintainers 
against nios2 target.  The initial failure symptom was a linker error 
resulting from use of GP-relative addressing on an object that wasn't 
allocated in the small data section.  It turns out that the real problem 
is that the TARGET_IN_SMALL_DATA_P hook uses the function 
int_size_in_bytes to check whether an object is "small", and this 
function treats flexible array members as having size 0.  All backends 
except for tic6x that implement TARGET_IN_SMALL_DATA_P use a similar 
implementation of the hook that has the same problem.

The solution here is to add a new function that returns -1 as the size 
of any type with a flexible array member, and adjust all the affected 
backends to use it.  I swiped the predicate for identifying these types 
from the C front end.

I regression-tested this on nios2-elf and nios2-linux-gnu.  I'm not set 
up to test any of the other affected back ends, but the code change is 
trivial and identical to what I did for nios2.  OK to commit?  I'd like 
to backport this patch to all active branches as well, once it is in on 
trunk.

-Sandra

Comments

Richard Sandiford Nov. 21, 2019, 10:59 a.m. UTC | #1 
Sandra Loosemore <sandra@codesourcery.com> writes:
> This patch is for PR target/92499, a bug reported by GLIBC maintainers 
> against nios2 target.  The initial failure symptom was a linker error 
> resulting from use of GP-relative addressing on an object that wasn't 
> allocated in the small data section.  It turns out that the real problem 
> is that the TARGET_IN_SMALL_DATA_P hook uses the function 
> int_size_in_bytes to check whether an object is "small", and this 
> function treats flexible array members as having size 0.  All backends 
> except for tic6x that implement TARGET_IN_SMALL_DATA_P use a similar 
> implementation of the hook that has the same problem.
>
> The solution here is to add a new function that returns -1 as the size 
> of any type with a flexible array member, and adjust all the affected 
> backends to use it.  I swiped the predicate for identifying these types 
> from the C front end.
>
> I regression-tested this on nios2-elf and nios2-linux-gnu.  I'm not set 
> up to test any of the other affected back ends, but the code change is 
> trivial and identical to what I did for nios2.  OK to commit?  I'd like 
> to backport this patch to all active branches as well, once it is in on 
> trunk.

For local decls like the ones in the testcase, we should be able to
query the actual size of the decl using DECL_SIZE(_UNIT).  But for
externs we have to make conservatively correct assumptions based only
on the type, so I agree this looks like the right way of fixing it.

The problem is that it's going to be an ABI break for existing code that
does something like:

    struct flexible
    {
      int length;
      int data[];
    };

    extern struct flexible foo;
    int get_foo (void) { return foo.data[0]; }

since the code might have been compiled on the assumption that foo
fits within the small-data limit.

So I think we need to restrict the change to targets that have
explicitly bought into the ABI change, i.e. just nios2 for now.
It might also be worth mentioning in the release notes.

The target-independent bits are OK for trunk unless someone objects
in the next 24 hrs.  I'm not sure it's appropriate to backport given
the ABI change though.

Thanks,
Richard
Sandra Loosemore Nov. 21, 2019, 3:03 p.m. UTC | #2 
On 11/21/19 3:59 AM, Richard Sandiford wrote:
> Sandra Loosemore <sandra@codesourcery.com> writes:
>> This patch is for PR target/92499, a bug reported by GLIBC maintainers
>> against nios2 target.  The initial failure symptom was a linker error
>> resulting from use of GP-relative addressing on an object that wasn't
>> allocated in the small data section.  It turns out that the real problem
>> is that the TARGET_IN_SMALL_DATA_P hook uses the function
>> int_size_in_bytes to check whether an object is "small", and this
>> function treats flexible array members as having size 0.  All backends
>> except for tic6x that implement TARGET_IN_SMALL_DATA_P use a similar
>> implementation of the hook that has the same problem.
>>
>> The solution here is to add a new function that returns -1 as the size
>> of any type with a flexible array member, and adjust all the affected
>> backends to use it.  I swiped the predicate for identifying these types
>> from the C front end.
>>
>> I regression-tested this on nios2-elf and nios2-linux-gnu.  I'm not set
>> up to test any of the other affected back ends, but the code change is
>> trivial and identical to what I did for nios2.  OK to commit?  I'd like
>> to backport this patch to all active branches as well, once it is in on
>> trunk.
> 
> For local decls like the ones in the testcase, we should be able to
> query the actual size of the decl using DECL_SIZE(_UNIT).  But for
> externs we have to make conservatively correct assumptions based only
> on the type, so I agree this looks like the right way of fixing it.
> 
> The problem is that it's going to be an ABI break for existing code that
> does something like:
> 
>      struct flexible
>      {
>        int length;
>        int data[];
>      };
> 
>      extern struct flexible foo;
>      int get_foo (void) { return foo.data[0]; }
> 
> since the code might have been compiled on the assumption that foo
> fits within the small-data limit.
> 
> So I think we need to restrict the change to targets that have
> explicitly bought into the ABI change, i.e. just nios2 for now.
> It might also be worth mentioning in the release notes.
> 
> The target-independent bits are OK for trunk unless someone objects
> in the next 24 hrs.  I'm not sure it's appropriate to backport given
> the ABI change though.

Hmmm.  It sounds like I need to go back to the drawing board on this 
one; I don't think the test case attached to the issue accurately 
captured the actual problem affecting GLIBC.  The current code is 
consistent about using the same assumptions for both data placement and 
addressing, but maybe the type of the object isn't being declared or 
represented consistently in all compilation units?  Or there are such 
large flexibly-sized objects being allocated in small data that they're 
causing a range overflow for %gprel?  :-S

Anyway, I'll withdraw this patch for the time being, at least until I'm 
convinced that it's actually solving the right problem.

-Sandra
Martin Sebor Nov. 21, 2019, 5:34 p.m. UTC | #3 
On 11/21/19 3:59 AM, Richard Sandiford wrote:
> Sandra Loosemore <sandra@codesourcery.com> writes:
>> This patch is for PR target/92499, a bug reported by GLIBC maintainers
>> against nios2 target.  The initial failure symptom was a linker error
>> resulting from use of GP-relative addressing on an object that wasn't
>> allocated in the small data section.  It turns out that the real problem
>> is that the TARGET_IN_SMALL_DATA_P hook uses the function
>> int_size_in_bytes to check whether an object is "small", and this
>> function treats flexible array members as having size 0.  All backends
>> except for tic6x that implement TARGET_IN_SMALL_DATA_P use a similar
>> implementation of the hook that has the same problem.
>>
>> The solution here is to add a new function that returns -1 as the size
>> of any type with a flexible array member, and adjust all the affected
>> backends to use it.  I swiped the predicate for identifying these types
>> from the C front end.
>>
>> I regression-tested this on nios2-elf and nios2-linux-gnu.  I'm not set
>> up to test any of the other affected back ends, but the code change is
>> trivial and identical to what I did for nios2.  OK to commit?  I'd like
>> to backport this patch to all active branches as well, once it is in on
>> trunk.
> 
> For local decls like the ones in the testcase, we should be able to
> query the actual size of the decl using DECL_SIZE(_UNIT).  But for
> externs we have to make conservatively correct assumptions based only
> on the type, so I agree this looks like the right way of fixing it.

In my testing, DECL_SIZE(_UNIT) doesn't reliably report the size
of an object with an initialized flexible array member (I think
pr91672 might be one of the symptoms of this problem).  To work
around it I recently added the component_ref_size function to
tree.c that computes this size.

Martin

> 
> The problem is that it's going to be an ABI break for existing code that
> does something like:
> 
>      struct flexible
>      {
>        int length;
>        int data[];
>      };
> 
>      extern struct flexible foo;
>      int get_foo (void) { return foo.data[0]; }
> 
> since the code might have been compiled on the assumption that foo
> fits within the small-data limit.
> 
> So I think we need to restrict the change to targets that have
> explicitly bought into the ABI change, i.e. just nios2 for now.
> It might also be worth mentioning in the release notes.
> 
> The target-independent bits are OK for trunk unless someone objects
> in the next 24 hrs.  I'm not sure it's appropriate to backport given
> the ABI change though.
> 
> Thanks,
> Richard
>
diff mbox series

Patch

Index: gcc/c/c-decl.c
===================================================================
--- gcc/c/c-decl.c	(revision 278366)
+++ gcc/c/c-decl.c	(working copy)
@@ -5667,39 +5667,6 @@  check_compound_literal_type (location_t
 		"defining a type in a compound literal is invalid in C++");
 }
 
-/* Determine whether TYPE is a structure with a flexible array member,
-   or a union containing such a structure (possibly recursively).  */
-
-static bool
-flexible_array_type_p (tree type)
-{
-  tree x;
-  switch (TREE_CODE (type))
-    {
-    case RECORD_TYPE:
-      x = TYPE_FIELDS (type);
-      if (x == NULL_TREE)
-	return false;
-      while (DECL_CHAIN (x) != NULL_TREE)
-	x = DECL_CHAIN (x);
-      if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE
-	  && TYPE_SIZE (TREE_TYPE (x)) == NULL_TREE
-	  && TYPE_DOMAIN (TREE_TYPE (x)) != NULL_TREE
-	  && TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (x))) == NULL_TREE)
-	return true;
-      return false;
-    case UNION_TYPE:
-      for (x = TYPE_FIELDS (type); x != NULL_TREE; x = DECL_CHAIN (x))
-	{
-	  if (flexible_array_type_p (TREE_TYPE (x)))
-	    return true;
-	}
-      return false;
-    default:
-    return false;
-  }
-}
-
 /* Performs sanity checks on the TYPE and WIDTH of the bit-field NAME,
    replacing with appropriate values if they are invalid.  */
 
Index: gcc/config/alpha/alpha.c
===================================================================
--- gcc/config/alpha/alpha.c	(revision 278366)
+++ gcc/config/alpha/alpha.c	(working copy)
@@ -796,7 +796,7 @@  alpha_in_small_data_p (const_tree exp)
     }
   else
     {
-      HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (exp));
+      HOST_WIDE_INT size = fixed_int_size_in_bytes (TREE_TYPE (exp));
 
       /* If this is an incomplete type with size 0, then we can't put it
 	 in sdata because it might be too big when completed.  */
Index: gcc/config/arc/arc.c
===================================================================
--- gcc/config/arc/arc.c	(revision 278366)
+++ gcc/config/arc/arc.c	(working copy)
@@ -8643,7 +8643,7 @@  arc_in_small_data_p (const_tree decl)
      section.  */
   else if (TREE_PUBLIC (decl))
     {
-      size = int_size_in_bytes (TREE_TYPE (decl));
+      size = fixed_int_size_in_bytes (TREE_TYPE (decl));
       return (size > 0 && size <= g_switch_value);
     }
   return false;
Index: gcc/config/frv/frv.c
===================================================================
--- gcc/config/frv/frv.c	(revision 278366)
+++ gcc/config/frv/frv.c	(working copy)
@@ -9319,7 +9319,7 @@  frv_in_small_data_p (const_tree decl)
       return false;
     }
 
-  size = int_size_in_bytes (TREE_TYPE (decl));
+  size = fixed_int_size_in_bytes (TREE_TYPE (decl));
   if (size > 0 && size <= g_switch_value)
     return true;
 
Index: gcc/config/ia64/ia64.c
===================================================================
--- gcc/config/ia64/ia64.c	(revision 278366)
+++ gcc/config/ia64/ia64.c	(working copy)
@@ -10016,7 +10016,7 @@  ia64_in_small_data_p (const_tree exp)
     }
   else
     {
-      HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (exp));
+      HOST_WIDE_INT size = fixed_int_size_in_bytes (TREE_TYPE (exp));
 
       /* If this is an incomplete type with size 0, then we can't put it
 	 in sdata because it might be too big when completed.  */
Index: gcc/config/lm32/lm32.c
===================================================================
--- gcc/config/lm32/lm32.c	(revision 278366)
+++ gcc/config/lm32/lm32.c	(working copy)
@@ -795,7 +795,7 @@  lm32_in_small_data_p (const_tree exp)
     }
   else
     {
-      HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (exp));
+      HOST_WIDE_INT size = fixed_int_size_in_bytes (TREE_TYPE (exp));
 
       /* If this is an incomplete type with size 0, then we can't put it
          in sdata because it might be too big when completed.  */
Index: gcc/config/m32r/m32r.c
===================================================================
--- gcc/config/m32r/m32r.c	(revision 278366)
+++ gcc/config/m32r/m32r.c	(working copy)
@@ -513,7 +513,7 @@  m32r_in_small_data_p (const_tree decl)
     {
       if (! TREE_READONLY (decl) && ! TARGET_SDATA_NONE)
 	{
-	  int size = int_size_in_bytes (TREE_TYPE (decl));
+	  int size = fixed_int_size_in_bytes (TREE_TYPE (decl));
 
 	  if (size > 0 && size <= g_switch_value)
 	    return true;
Index: gcc/config/microblaze/microblaze.c
===================================================================
--- gcc/config/microblaze/microblaze.c	(revision 278366)
+++ gcc/config/microblaze/microblaze.c	(working copy)
@@ -3236,7 +3236,7 @@  microblaze_elf_in_small_data_p (const_tr
 	return true;
     }
 
-  size = int_size_in_bytes (TREE_TYPE (decl));
+  size = fixed_int_size_in_bytes (TREE_TYPE (decl));
 
   return (size > 0 && size <= microblaze_section_threshold);
 }
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 278366)
+++ gcc/config/mips/mips.c	(working copy)
@@ -9399,7 +9399,7 @@  mips_in_small_data_p (const_tree decl)
 
   /* We have traditionally not treated zero-sized objects as small data,
      so this is now effectively part of the ABI.  */
-  size = int_size_in_bytes (TREE_TYPE (decl));
+  size = fixed_int_size_in_bytes (TREE_TYPE (decl));
   return size > 0 && size <= mips_small_data_threshold;
 }
 
Index: gcc/config/nios2/nios2.c
===================================================================
--- gcc/config/nios2/nios2.c	(revision 278366)
+++ gcc/config/nios2/nios2.c	(working copy)
@@ -2375,7 +2375,7 @@  nios2_in_small_data_p (const_tree exp)
 	}
       else
 	{
-	  HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (exp));
+	  HOST_WIDE_INT size = fixed_int_size_in_bytes (TREE_TYPE (exp));
 
 	  /* If this is an incomplete type with size 0, then we can't put it
 	     in sdata because it might be too big when completed.  */
Index: gcc/config/riscv/riscv.c
===================================================================
--- gcc/config/riscv/riscv.c	(revision 278366)
+++ gcc/config/riscv/riscv.c	(working copy)
@@ -3377,7 +3377,7 @@  riscv_in_small_data_p (const_tree x)
       return strcmp (sec, ".sdata") == 0 || strcmp (sec, ".sbss") == 0;
     }
 
-  return riscv_size_ok_for_small_data_p (int_size_in_bytes (TREE_TYPE (x)));
+  return riscv_size_ok_for_small_data_p (fixed_int_size_in_bytes (TREE_TYPE (x)));
 }
 
 /* Switch to the appropriate section for output of DECL.  */
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 278366)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -19442,7 +19442,7 @@  rs6000_elf_in_small_data_p (const_tree d
 	  && !rs6000_readonly_in_sdata)
 	return false;
 
-      HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (decl));
+      HOST_WIDE_INT size = fixed_int_size_in_bytes (TREE_TYPE (decl));
 
       if (size > 0
 	  && size <= g_switch_value
Index: gcc/config/rx/rx.c
===================================================================
--- gcc/config/rx/rx.c	(revision 278366)
+++ gcc/config/rx/rx.c	(working copy)
@@ -2289,7 +2289,7 @@  rx_in_small_data (const_tree decl)
   if (section)
     return (strcmp (section, "D_2") == 0) || (strcmp (section, "B_2") == 0);
 
-  size = int_size_in_bytes (TREE_TYPE (decl));
+  size = fixed_int_size_in_bytes (TREE_TYPE (decl));
 
   return (size > 0) && (size <= rx_small_data_limit);
 }
Index: gcc/testsuite/gcc.dg/pr92499.c
===================================================================
--- gcc/testsuite/gcc.dg/pr92499.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/pr92499.c	(working copy)
@@ -0,0 +1,67 @@ 
+/* PR target/92499 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+enum { size = 100 };
+
+struct flexible
+{
+  int length;
+  int data[];
+};
+
+struct inflexible
+{
+  int length;
+  int data[size];
+};
+
+static struct flexible flexible =
+  {
+    .data = { [size - 1] = 0, }
+  };
+
+static struct flexible flexible_nonzero =
+  {
+    .length = size,
+    .data = { [size - 1] = 0, }
+  };
+
+static struct inflexible inflexible =
+  {
+    .data = { [size - 1] = 0, }
+  };
+
+struct flexible *
+get_flexible (void)
+{
+  return &flexible;
+}
+
+struct flexible *
+get_flexible_nonzero (void)
+{
+  return &flexible_nonzero;
+}
+
+struct inflexible *
+get_inflexible (void)
+{
+  return &inflexible;
+}
+
+/* Check that variables containing flexible array elements are not placed
+   in small data.  Almost all targets that support small data sections use
+   the canonical names for them.  */
+/* { dg-final { scan-assembler-not "\\.sdata" } } */
+/* { dg-final { scan-assembler-not "\\.sbss" } } */
+/* { dg-final { scan-assembler-not "\\.section D_2" { target rx*-*-* } } } */
+/* { dg-final { scan-assembler-not "\\.section B_2" { target rx*-*-* } } } */
+
+/* Also check that GP-relative addressing is not used to reference any
+   of the variables, on targets that support that form of addressing for
+   small data.  Since the assembler syntax varies on the targets that support
+   this, we need to list the targets individually.  This list is not a
+   complete set.  */
+/* { dg-final { scan-assembler-not "%gp_rel\(.*flexible.*\)" { target mips*-*-* } } } */
+/* { dg-final { scan-assembler-not "%gprel\(.*flexible.*\)" { target nios2*-*-* } } } */
Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 278366)
+++ gcc/tree.c	(working copy)
@@ -3266,7 +3266,10 @@  size_in_bytes_loc (location_t loc, const
 }
 
 /* Return the size of TYPE (in bytes) as a wide integer
-   or return -1 if the size can vary or is larger than an integer.  */
+   or return -1 if the size can vary or is larger than an integer.
+   Note that this function returns the non-variable size for objects
+   containing flexible array members (like the C sizeof operator
+   does) instead of -1; see fixed_int_size_in_bytes below.  */
 
 HOST_WIDE_INT
 int_size_in_bytes (const_tree type)
@@ -15000,6 +15003,54 @@  default_is_empty_record (const_tree type
   return default_is_empty_type (TYPE_MAIN_VARIANT (type));
 }
 
+/* Determine whether TYPE is a structure with a flexible array member,
+   or a union containing such a structure (possibly recursively).  */
+
+bool
+flexible_array_type_p (const_tree type)
+{
+  tree x, last;
+  switch (TREE_CODE (type))
+    {
+    case RECORD_TYPE:
+      last = NULL_TREE;
+      for (x = TYPE_FIELDS (type); x != NULL_TREE; x = DECL_CHAIN (x))
+	if (TREE_CODE (x) == FIELD_DECL)
+	  last = x;
+      if (last == NULL_TREE)
+	return false;
+      if (TREE_CODE (TREE_TYPE (last)) == ARRAY_TYPE
+	  && TYPE_SIZE (TREE_TYPE (last)) == NULL_TREE
+	  && TYPE_DOMAIN (TREE_TYPE (last)) != NULL_TREE
+	  && TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (last))) == NULL_TREE)
+	return true;
+      return false;
+    case UNION_TYPE:
+      for (x = TYPE_FIELDS (type); x != NULL_TREE; x = DECL_CHAIN (x))
+	{
+	  if (TREE_CODE (x) == FIELD_DECL
+	      && flexible_array_type_p (TREE_TYPE (x)))
+	    return true;
+	}
+      return false;
+    default:
+      return false;
+  }
+}
+
+/* Like int_size_in_bytes, but also return -1 for flexible array
+   types, so that it only returns a positive value for types with a
+   fixed size.  */
+
+HOST_WIDE_INT
+fixed_int_size_in_bytes (const_tree type)
+{
+  if (flexible_array_type_p (type))
+    return -1;
+  else
+    return int_size_in_bytes (type);
+}
+
 /* Like int_size_in_bytes, but handle empty records specially.  */
 
 HOST_WIDE_INT
Index: gcc/tree.h
===================================================================
--- gcc/tree.h	(revision 278366)
+++ gcc/tree.h	(working copy)
@@ -6077,6 +6077,8 @@  extern void gt_pch_nx (tree &, gt_pointe
 
 extern bool nonnull_arg_p (const_tree);
 extern bool default_is_empty_record (const_tree);
+extern bool flexible_array_type_p (const_tree);
+extern HOST_WIDE_INT fixed_int_size_in_bytes (const_tree);
 extern HOST_WIDE_INT arg_int_size_in_bytes (const_tree);
 extern tree arg_size_in_bytes (const_tree);
 extern bool expr_type_first_operand_type_p (tree_code);