This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Memory corruption due to word sharing


(Somehow my reply was private to Aldy ... forwarding to gcc-patches
now, given that it contains a patch and we changed topics)

On Fri, 3 Feb 2012, Richard Guenther wrote:

> On Thu, 2 Feb 2012, Aldy Hernandez wrote:
> 
> > Linus Torvalds <torvalds@linux-foundation.org> writes:
> > 
> > > Seriously - is there any real argument *against* just using the base
> > > type as a hint for access size?
> > 
> > If I'm on the hook for attempting to fix this again, I'd also like to
> > know if there are any arguments against using the base type.
> 
> Well, if you consider
> 
> struct {
>   int i : 1;
>   char c;
> };
> 
> then you'll realize that 'i' has SImode (and int type) but the
> underlying bitfield has only 1 byte size (thus, QImode) and
> 'c' starts at offset 1.
> 
> So no, you cannot use the base type either.
> 
> I've playing with the following patch yesterday, which computes
> an "underlying object" for all bitfields and forcefully lowers
> all bitfield component-refs to use that underlying object
> (just to check correctness, it doesn't generate nice code as
> BIT_FIELD_REF on memory is effectively resulting in the same
> code as if using the bitfield FIELD_DECLs directly - we'd
> need to explicitely split things into separate stmts with RMW
> cycles).
> 
> You should be able to re-use the underlying object compute though
> (and we can make it more intelligent even) during expansion
> for the C++ memory model (and in fact underlying object compute
> might just do sth different dependent on the memory model in
> effect).
> 
> Disclaimer: untested.

The following works (roughly, still mostly untested).  SRA needs
a fix (included) and the gimplify.c hunk really only shows what
we are supposed to be able to do (access the representative).
As-is SRA could now do a nice job on bitfields, but that needs
some changes - or we lower all bitfield ops in some extra pass
(if not then expand would need to be changed to look at the
representatives instead).

Still the idea is to compute all these things up-front during
type layout instead of re-discovering them at each bitfield
access we expand in get_bit_range.  And we can use that information
consistently across passes.

We should of course try harder to avoid adding a new field to
struct tree_field_decl - DECL_INITIAL came to my mind, but
the C frontend happens to use that for bitfields ... while
it probably could as well use lang_type.enum_{min,max}?

Comments?

Thanks,
Richard.

2012-02-03  Richard Guenther  <rguenther@suse.de>

	* tree.h (DECL_BIT_FIELD_REPRESENTATIVE): New define.
	(struct tree_field_decl): New field bit_field_representative.
	* stor-layout.c (start_bitfield_representative): New function.
	(finish_bitfield_representative): Likewise.
	(finish_bitfield_layout): Likewise.
	(finish_record_layout): Call finish_bitfield_layout.

	* gimplify.c (gimplify_expr): Translate bitfield accesses
	to BIT_FIELD_REFs of the representative.

        * tree-sra.c (create_access_replacement): Only rename the
        replacement if we can rewrite it into SSA form.  Properly
        mark register typed replacements that we cannot rewrite
        with TREE_ADDRESSABLE.

Index: gcc/stor-layout.c
===================================================================
*** gcc/stor-layout.c.orig	2012-02-03 10:42:49.000000000 +0100
--- gcc/stor-layout.c	2012-02-03 12:37:38.000000000 +0100
*************** finalize_type_size (tree type)
*** 1722,1727 ****
--- 1722,1889 ----
      }
  }
  
+ /* Return a new underlying object for a bitfield started with FIELD.  */
+ 
+ static tree
+ start_bitfield_representative (tree field)
+ {
+   tree repr = make_node (FIELD_DECL);
+   DECL_FIELD_OFFSET (repr) = DECL_FIELD_OFFSET (field);
+   DECL_FIELD_BIT_OFFSET (repr) = DECL_FIELD_BIT_OFFSET (field);
+   SET_DECL_OFFSET_ALIGN (repr, DECL_OFFSET_ALIGN (field));
+   DECL_SIZE (repr) = DECL_SIZE (field);
+   DECL_PACKED (repr) = DECL_PACKED (field);
+   DECL_CONTEXT (repr) = DECL_CONTEXT (field);
+   return repr;
+ }
+ 
+ /* Finish up a bitfield group that was started by creating the underlying
+    object REPR with the last fied in the bitfield group FIELD.  */
+ 
+ static void
+ finish_bitfield_representative (tree repr, tree field)
+ {
+   unsigned HOST_WIDE_INT bitsize, maxbitsize, modesize;
+   enum machine_mode mode;
+   tree nextf, size;
+ 
+   size = size_diffop (DECL_FIELD_OFFSET (field),
+ 		      DECL_FIELD_OFFSET (repr));
+   gcc_assert (host_integerp (size, 1));
+   bitsize = (tree_low_cst (size, 1) * BITS_PER_UNIT
+ 	     + tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1)
+ 	     - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)
+ 	     + tree_low_cst (DECL_SIZE (field), 1));
+ 
+   /* Now nothing tells us how to pad out bitsize ...  */
+   nextf = DECL_CHAIN (field);
+   while (nextf && TREE_CODE (nextf) != FIELD_DECL)
+     nextf = DECL_CHAIN (nextf);
+   if (nextf)
+     {
+       tree maxsize = size_diffop (DECL_FIELD_OFFSET (nextf),
+ 				  DECL_FIELD_OFFSET (repr));
+       gcc_assert (host_integerp (maxsize, 1));
+       maxbitsize = (tree_low_cst (maxsize, 1) * BITS_PER_UNIT
+ 		    + tree_low_cst (DECL_FIELD_BIT_OFFSET (nextf), 1)
+ 		    - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));
+     }
+   else
+     {
+       tree maxsize = size_diffop (TYPE_SIZE_UNIT (DECL_CONTEXT (field)),
+ 				  DECL_FIELD_OFFSET (repr));
+       gcc_assert (host_integerp (maxsize, 1));
+       maxbitsize = (tree_low_cst (maxsize, 1) * BITS_PER_UNIT
+ 		    - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));
+     }
+ 
+   /* Only if we don't artificially break up the representative in
+      the middle of a large bitfield with different possibly
+      overlapping representatives.  And all representatives start
+      at byte offset.  */
+   gcc_assert (maxbitsize % BITS_PER_UNIT == 0);
+ 
+   /* Round up bitsize to multiples of BITS_PER_UNIT.  */
+   bitsize = (bitsize + BITS_PER_UNIT - 1) & ~(BITS_PER_UNIT - 1);
+ 
+   /* Find the smallest nice mode to use.
+      ???  Possibly use get_best_mode with appropriate arguments instead
+      (which would eventually require splitting representatives here).  */
+   for (modesize = bitsize; modesize <= maxbitsize; modesize += BITS_PER_UNIT)
+     {
+       mode = mode_for_size (modesize, MODE_INT, 1);
+       if (mode != BLKmode)
+ 	break;
+     }
+ 
+   if (mode == BLKmode)
+     {
+       /* We really want a BLKmode representative only as a last resort,
+          considering the member b in
+ 	   struct { int a : 7; int b : 17; int c; } __attribute__((packed));
+ 	 Otherwise we simply want to split the representative up
+ 	 allowing for overlaps within the bitfield region as required for
+ 	   struct { int a : 7; int b : 7; int c : 10; int d; } __attribute__((packed));
+ 	 [0, 15] HImode for a and b, [8, 23] HImode for c.  */
+       DECL_SIZE (repr) = bitsize_int (bitsize);
+       DECL_SIZE_UNIT (repr) = size_int (bitsize / BITS_PER_UNIT);
+       DECL_MODE (repr) = BLKmode;
+       TREE_TYPE (repr) = build_array_type_nelts (unsigned_char_type_node,
+ 						 bitsize / BITS_PER_UNIT);
+     }
+   else
+     {
+       DECL_SIZE (repr) = bitsize_int (modesize);
+       DECL_SIZE_UNIT (repr) = size_int (modesize / BITS_PER_UNIT);
+       DECL_MODE (repr) = mode;
+       TREE_TYPE (repr) = lang_hooks.types.type_for_mode (mode, 1);
+     }
+ }
+ 
+ /* Compute and set FIELD_DECLs for the underlying objects we should
+    use for bitfield access for the structure layed out with RLI.  */
+ 
+ static void
+ finish_bitfield_layout (record_layout_info rli)
+ {
+   tree field, prev;
+   tree repr = NULL_TREE;
+ 
+   /* Unions would be special, for the ease of type-punning optimizations
+      we could use the underlying type as hint for the representative
+      if the bitfield would fit and the representative would not exceed
+      the union in size.  */
+   if (TREE_CODE (rli->t) != RECORD_TYPE)
+     return;
+ 
+   for (prev = NULL_TREE, field = TYPE_FIELDS (rli->t);
+        field; field = DECL_CHAIN (field))
+     {
+       if (TREE_CODE (field) != FIELD_DECL)
+ 	continue;
+ 
+       if (!repr
+ 	  && DECL_BIT_FIELD_TYPE (field))
+ 	{
+ 	  /* Start new representative.  */
+ 	  repr = start_bitfield_representative (field);
+ 	}
+       else if (repr
+ 	       && ! DECL_BIT_FIELD_TYPE (field))
+ 	{
+ 	  /* Finish off new representative.  */
+ 	  finish_bitfield_representative (repr, prev);
+ 	  repr = NULL_TREE;
+ 	}
+       else if (DECL_BIT_FIELD_TYPE (field))
+ 	{
+ 	  /* Zero-size bitfields finish off a representative and
+ 	     do not have a representative themselves.  */
+ 	  if (integer_zerop (DECL_SIZE (field)))
+ 	    {
+ 	      finish_bitfield_representative (repr, prev);
+ 	      repr = NULL_TREE;
+ 	    }
+ 	  /* FIXME.  A gap finishes off a representative(?).  */
+ 	  else if (0 /* offset + size of repr != offset of field */)
+ 	    {
+ 	      finish_bitfield_representative (repr, prev);
+ 	      repr = start_bitfield_representative (field);
+ 	    }
+ 	}
+       else
+ 	continue;
+ 
+       if (repr)
+ 	DECL_BIT_FIELD_REPRESENTATIVE (field) = repr;
+ 
+       prev = field;
+     }
+ 
+   if (repr)
+     finish_bitfield_representative (repr, prev);
+ }
+ 
  /* Do all of the work required to layout the type indicated by RLI,
     once the fields have been laid out.  This function will call `free'
     for RLI, unless FREE_P is false.  Passing a value other than false
*************** finish_record_layout (record_layout_info
*** 1742,1747 ****
--- 1904,1912 ----
    /* Perform any last tweaks to the TYPE_SIZE, etc.  */
    finalize_type_size (rli->t);
  
+   /* Compute bitfield representatives.  */
+   finish_bitfield_layout (rli);
+ 
    /* Propagate TYPE_PACKED to variants.  With C++ templates,
       handle_packed_attribute is too early to do this.  */
    for (variant = TYPE_NEXT_VARIANT (rli->t); variant;
Index: gcc/tree.h
===================================================================
*** gcc/tree.h.orig	2012-02-03 10:42:49.000000000 +0100
--- gcc/tree.h	2012-02-03 10:45:08.000000000 +0100
*************** struct GTY(()) tree_decl_with_rtl {
*** 3021,3026 ****
--- 3021,3032 ----
  #define DECL_BIT_FIELD_TYPE(NODE) \
    (FIELD_DECL_CHECK (NODE)->field_decl.bit_field_type)
  
+ /* In a FIELD_DECL, this is a pointer to the storage representative
+    FIELD_DECL.
+    ???  Try harder to find a pointer we can re-use.  */
+ #define DECL_BIT_FIELD_REPRESENTATIVE(NODE) \
+   (FIELD_DECL_CHECK (NODE)->field_decl.bit_field_representative)
+ 
  /* For a FIELD_DECL in a QUAL_UNION_TYPE, records the expression, which
     if nonzero, indicates that the field occupies the type.  */
  #define DECL_QUALIFIER(NODE) (FIELD_DECL_CHECK (NODE)->field_decl.qualifier)
*************** struct GTY(()) tree_field_decl {
*** 3071,3076 ****
--- 3077,3083 ----
  
    tree offset;
    tree bit_field_type;
+   tree bit_field_representative;
    tree qualifier;
    tree bit_offset;
    tree fcontext;
Index: gcc/gimplify.c
===================================================================
*** gcc/gimplify.c.orig	2012-02-03 10:42:49.000000000 +0100
--- gcc/gimplify.c	2012-02-03 11:12:04.000000000 +0100
*************** gimplify_expr (tree *expr_p, gimple_seq
*** 6887,6897 ****
  					fallback != fb_none);
  	  break;
  
  	case ARRAY_REF:
  	case ARRAY_RANGE_REF:
  	case REALPART_EXPR:
  	case IMAGPART_EXPR:
- 	case COMPONENT_REF:
  	case VIEW_CONVERT_EXPR:
  	  ret = gimplify_compound_lval (expr_p, pre_p, post_p,
  					fallback ? fallback : fb_rvalue);
--- 6887,6929 ----
  					fallback != fb_none);
  	  break;
  
+ 	case COMPONENT_REF:
+ 	  if (DECL_BIT_FIELD_TYPE (TREE_OPERAND (*expr_p, 1)))
+ 	    {
+ 	      tree field = TREE_OPERAND (*expr_p, 1);
+ 	      tree repr = DECL_BIT_FIELD_REPRESENTATIVE (field);
+ 	      gcc_assert (TREE_OPERAND (*expr_p, 2) == NULL_TREE);
+ 	      if (repr != NULL_TREE)
+ 		{
+ 		  tree offset;
+ 		  offset = size_diffop (DECL_FIELD_OFFSET (field),
+ 					DECL_FIELD_OFFSET (repr));
+ 		  offset = size_binop (MULT_EXPR,
+ 				       offset, ssize_int (BITS_PER_UNIT));
+ 		  offset = fold_convert (sbitsizetype, offset);
+ 		  offset = size_binop (PLUS_EXPR, offset,
+ 				       size_diffop (DECL_FIELD_BIT_OFFSET (field),
+ 						    DECL_FIELD_BIT_OFFSET (repr)));
+ 		  *expr_p = build3_loc (input_location,
+ 					BIT_FIELD_REF,
+ 					TREE_TYPE (*expr_p),
+ 					build3_loc (input_location,
+ 						    COMPONENT_REF,
+ 						    TREE_TYPE (repr),
+ 						    TREE_OPERAND (*expr_p, 0),
+ 						    repr,
+ 						    NULL_TREE),
+ 					DECL_SIZE (field), offset);
+ 		  ret = GS_OK;
+ 		  break;
+ 		}
+ 	    }
+ 
+ 	  /* Fall thru.  */
  	case ARRAY_REF:
  	case ARRAY_RANGE_REF:
  	case REALPART_EXPR:
  	case IMAGPART_EXPR:
  	case VIEW_CONVERT_EXPR:
  	  ret = gimplify_compound_lval (expr_p, pre_p, post_p,
  					fallback ? fallback : fb_rvalue);
Index: gcc/tree-sra.c
===================================================================
*** gcc/tree-sra.c.orig	2012-02-03 12:22:26.000000000 +0100
--- gcc/tree-sra.c	2012-02-03 12:22:36.000000000 +0100
*************** create_access_replacement (struct access
*** 1908,1920 ****
  
    repl = create_tmp_var (access->type, "SR");
    add_referenced_var (repl);
!   if (rename)
      mark_sym_for_renaming (repl);
  
!   if (!access->grp_partial_lhs
!       && (TREE_CODE (access->type) == COMPLEX_TYPE
! 	  || TREE_CODE (access->type) == VECTOR_TYPE))
!     DECL_GIMPLE_REG_P (repl) = 1;
  
    DECL_SOURCE_LOCATION (repl) = DECL_SOURCE_LOCATION (access->base);
    DECL_ARTIFICIAL (repl) = 1;
--- 1908,1926 ----
  
    repl = create_tmp_var (access->type, "SR");
    add_referenced_var (repl);
!   if (!access->grp_partial_lhs
!       && rename)
      mark_sym_for_renaming (repl);
  
!   if (TREE_CODE (access->type) == COMPLEX_TYPE
!       || TREE_CODE (access->type) == VECTOR_TYPE)
!     {
!       if (!access->grp_partial_lhs)
! 	DECL_GIMPLE_REG_P (repl) = 1;
!     }
!   else if (access->grp_partial_lhs
! 	   && is_gimple_reg_type (access->type))
!     TREE_ADDRESSABLE (repl) = 1;
  
    DECL_SOURCE_LOCATION (repl) = DECL_SOURCE_LOCATION (access->base);
    DECL_ARTIFICIAL (repl) = 1;


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]