diff mbox series

[v2,committed] Fix bugs relating to flexibly-sized objects in nios2 backend.

Message ID 82f14b71-d8e8-1fbc-1f6e-392a4edd97a3@codesourcery.com
State New
Headers show
Series [v2,committed] Fix bugs relating to flexibly-sized objects in nios2 backend. | expand

Commit Message

Sandra Loosemore Dec. 2, 2019, 4:36 a.m. UTC
I've checked in this patch to fix PR target/92499 in a more restricted 
way than my initial patch, without breaking ABI compatibility.  This 
version of the patch is for the nios2 backend only although (as I noted 
in the issue) I'm pretty sure other backends have similar problems.

I've fixed the immediate problem reported by the GLIBC project by 
replacing ASM_OUTPUT_ALIGNED_LOCAL with ASM_OUTPUT_ALIGNED_DECL_LOCAL so 
that it can use the targetm.in_small_data_p hook instead of looking at 
the allocated size of the object.  That inconsistency was what was 
causing it to use gp-relative addressing on a flexibly-sized local 
object that it was placing in .bss instead of .sbss.  I've also tweaked 
nios2_in_small_data_p to avoid treating objects with flexible array 
members in small data except where it's necessary to maintain ABI 
compatibility, and future-proofed it against a potential ABI change by 
not using gp-relative addressing for extern references to these types. 
Note that the nios2 backend doesn't default to generating gp-relative 
addresses for external objects of any type (you have to ask for that 
with -mgpopt=global) so probably there is hardly any code that would be 
affected if we did change the ABI down the road.

I plan to backport this patch to the GCC 9 branch but probably not any 
older branches.

-Sandra
diff mbox series

Patch

Index: gcc/c/c-decl.c
===================================================================
--- gcc/c/c-decl.c	(revision 278890)
+++ gcc/c/c-decl.c	(working copy)
@@ -5709,39 +5709,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/nios2/nios2.c
===================================================================
--- gcc/config/nios2/nios2.c	(revision 278890)
+++ gcc/config/nios2/nios2.c	(working copy)
@@ -2373,6 +2373,22 @@  nios2_in_small_data_p (const_tree exp)
 	  if (nios2_small_section_name_p (section))
 	    return true;
 	}
+      else if (flexible_array_type_p (TREE_TYPE (exp))
+	       && (!TREE_PUBLIC (exp) || DECL_EXTERNAL (exp)))
+	{
+	  /* We really should not consider any objects of any flexibly-sized
+	     type to be small data, but pre-GCC 10 did not test
+	     for this and just fell through to the next case.  Thus older
+	     code compiled with -mgpopt=global could contain GP-relative
+	     accesses to objects defined in this compilation unit with
+	     external linkage.  We retain the possible small-data treatment
+	     of such definitions for backward ABI compatibility, but
+	     no longer generate GP-relative accesses for external
+	     references (so that the ABI could be changed in the future
+	     with less potential impact), or objects with internal
+	     linkage.  */
+	  return false;
+	}
       else
 	{
 	  HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (exp));
Index: gcc/config/nios2/nios2.h
===================================================================
--- gcc/config/nios2/nios2.h	(revision 278890)
+++ gcc/config/nios2/nios2.h	(working copy)
@@ -467,10 +467,10 @@  while (0)
    the linker seems to want the alignment of data objects
    to depend on their types.  We do exactly that here.  */
 
-#undef  ASM_OUTPUT_ALIGNED_LOCAL
-#define ASM_OUTPUT_ALIGNED_LOCAL(FILE, NAME, SIZE, ALIGN)               \
+#undef  ASM_OUTPUT_ALIGNED_DECL_LOCAL
+#define ASM_OUTPUT_ALIGNED_DECL_LOCAL(FILE, DECL, NAME, SIZE, ALIGN)	\
 do {                                                                    \
-  if ((SIZE) <= nios2_section_threshold)                                \
+ if (targetm.in_small_data_p (DECL))					\
     switch_to_section (sbss_section);					\
   else                                                                  \
     switch_to_section (bss_section);					\
Index: gcc/testsuite/gcc.target/nios2/pr92499-1.c
===================================================================
--- gcc/testsuite/gcc.target/nios2/pr92499-1.c	(nonexistent)
+++ gcc/testsuite/gcc.target/nios2/pr92499-1.c	(working copy)
@@ -0,0 +1,48 @@ 
+/* PR target/92499 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -mgpopt=global -G8" } */
+
+/* Check placement and addressing of flexibly-sized objects with internal
+   linkage.  */
+
+enum { size = 100 };
+
+struct flexible
+{
+  int length;
+  int data[];
+};
+
+static struct flexible local_flexible =
+  {
+    .data = { [size - 1] = 0, }
+  };
+
+static struct flexible local_flexible_nonzero =
+  {
+    .length = size,
+    .data = { [size - 1] = 0, }
+  };
+
+struct flexible *
+get_local_flexible (void)
+{
+  return &local_flexible;
+}
+
+struct flexible *
+get_local_flexible_nonzero (void)
+{
+  return &local_flexible_nonzero;
+}
+
+/* We should not place the flexibly-sized objects in small data
+   sections, or generate gp-relative addresses for them.  */
+
+/* { dg-final { scan-assembler-not "\\.sdata" } } */
+/* { dg-final { scan-assembler-not "\\.sbss" } } */
+/* { dg-final { scan-assembler-not "%gprel\(.*flexible.*\)" } } */
+
+
+
+
Index: gcc/testsuite/gcc.target/nios2/pr92499-2.c
===================================================================
--- gcc/testsuite/gcc.target/nios2/pr92499-2.c	(nonexistent)
+++ gcc/testsuite/gcc.target/nios2/pr92499-2.c	(working copy)
@@ -0,0 +1,45 @@ 
+/* PR target/92499 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -mgpopt=global -G8" } */
+
+/* Check placement and addressing of flexibly-sized objects with external
+   linkage.  */
+
+enum { size = 100 };
+
+struct flexible
+{
+  int length;
+  int data[];
+};
+
+extern struct flexible global_flexible;
+struct flexible global_flexible =
+  {
+    .data = { [size - 1] = 0, }
+  };
+
+extern struct flexible global_flexible_nonzero;
+struct flexible global_flexible_nonzero =
+  {
+    .length = size,
+    .data = { [size - 1] = 0, }
+  };
+
+struct flexible *
+get_global_flexible (void)
+{
+  return &global_flexible;
+}
+
+struct flexible *
+get_global_flexible_nonzero (void)
+{
+  return &global_flexible_nonzero;
+}
+
+/* To preserve ABI compatibility we place the flexibly-sized objects in
+   small data sections.  */
+
+/* { dg-final { scan-assembler-times "\\.sdata" 1 } } */
+/* { dg-final { scan-assembler-times "\\.sbss" 1 } } */
Index: gcc/testsuite/gcc.target/nios2/pr92499-3.c
===================================================================
--- gcc/testsuite/gcc.target/nios2/pr92499-3.c	(nonexistent)
+++ gcc/testsuite/gcc.target/nios2/pr92499-3.c	(working copy)
@@ -0,0 +1,23 @@ 
+/* PR target/92499 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -mgpopt=global -G8" } */
+
+/* Check addressing of extern flexibly-sized objects.  */
+
+struct flexible
+{
+  int length;
+  int data[];
+};
+
+extern struct flexible extern_flexible;
+
+struct flexible *
+get_extern_flexible (void)
+{
+  return &extern_flexible;
+}
+
+/* We should not generate GP-relative addresses for external objects of
+   unknown size.  */
+/* { dg-final { scan-assembler-not "%gprel\(.*flexible.*\)" } } */
Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 278890)
+++ gcc/tree.c	(working copy)
@@ -15003,6 +15003,41 @@  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 handle empty records specially.  */
 
 HOST_WIDE_INT
Index: gcc/tree.h
===================================================================
--- gcc/tree.h	(revision 278890)
+++ gcc/tree.h	(working copy)
@@ -6138,6 +6138,7 @@  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 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);