This is the mail archive of the 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]

[PATCH] tighten gimple addressability checks


Attached is a patch suggestion to fix regressions compared to the 3.4
series. Problem/context description first:

is_gimple_addressable is fairly permissive today, and I think too
permissive in some cases.

In particular, COMPONENT_REFs are all considered addressable while
the address of BIT_FIELD decls sometimes really can't be taken.

This is an issue for the gimplifier itself, as it uses the predicate
to decide if it can take the address of an operand, for instance as
part of

     /* If [...] we can make things a bit more straightforward
	by converting the assignment to memcpy or memset.  */
     if (TREE_CODE (*from_p) == WITH_SIZE_EXPR)
       { ...
	 if (is_gimple_addressable (from))

In this specific case, we are even totally missing a check on the
destination's addressability and get misbehavior on the Ada testcase

     procedure P5 is

	type Message is record
	   B : Boolean;
	   Data : String (1 .. 4);
	end record;
	pragma Pack (Message);

	procedure Process (IM : Message; Size : Natural) is
	   LM : Message;
	   LM.Data (1 .. Size) := IM.Data (1 .. Size);


>From the gimplifier code mentioned above, the assignment in "Process"
is gimplified into

     D.555 = (character[1:D.536] *) &[1 ...]{lb: 1 sz: 1};
     __builtin_memcpy (&[1 ...]{lb: 1 sz: 1}, D.555, D.543);

taking the address of both and The field is packed at
*bit* position 1, and the compiler (x86_64-linux in my case) ICEs later
on from

      /* Someone beforehand should have rejected taking the address
	 of such an object.  */
      gcc_assert ((bitpos % BITS_PER_UNIT) == 0);

The general idea of the patch suggested here is simple: tighten the
addressability checks to prevent those too wild address-taking

There are two complications:

1) Problematic COMPONENT_REFs can be nested within outer tree constructs,
   for instance, "from" in the testcase above is like

	 type <array_type p5__process__T4b
	arg 0 <array_range_ref
	    arg 0 <component_ref 
		arg 0 <parm_decl im>
		arg 1 <field_decl data
		       bit offset <integer_cst constant invariant 1>
   so we have to recurse to tell.

2) Some DECL_BIT_FIELD components can actually have their address taken
   and rejecting all of those would lead to severe pessimization, typically
   from useless creation of temporaries, if not compiler crashes when
   variable sizes are at sake.

   The approach chosen here is to consider BLKmode fields, known to be
   byte aligned, addressable when the address is taken for the purposes
   of a block oriented operation, where address vs type alignment
   expectations are less strict than in the general case.

   We introduce a new flag on ADDR_EXPR (BLOCK_ADDRESS_P) to convey
   this purpose and an alternate predicate (is_gimple_block_addressable)
   to perform the check.

The patch was bootstrapped and regtested with languages=all,ada on
x86_64-unknown-linux-gnu. We have been using it internally in 4.1
based compilers on a large number of targets for more than a year now.

I realize this is invasive and would be happy to discuss enhancements
or alternate ways out as necessary.

Thanks in advance,


2007-10-25  Olivier Hainque  <>
            Eric Botcazou  <>

	* tree.h (BLOCK_ADDRESS_P): New flag on ADDR_EXPR - indicates if
	the address is taken for the purposes of a block oriented operation
	such as memset, memcmp or memcpy.
	* tree-gimple.c (is_gimple_addressable_1): New function.  Similar to
	the former is_gimple_addressable with an extra argument to state if
	the check is made for a BLOCK_ADDRESS_P outer node.  Reject bitfield
	COMPONENT_REFs unless BLKmode for a block address, and recurse to find
	them out.
	(is_gimple_addressable): is_gimple_addressable_1 for the general
	case, with full	constraints on bitfield COMPONENT_REFs.
	(is_gimple_block_addressable): New function, is_gimple_addressable_1
	for the purpose of a block oriented operation.
	(is_gimple_lvalue): Adjust not to rely on is_gimple_addressable.
	* tree-gimple.h: Declare it.
	* gimplify.c: Add notes to explain the need for different sorts of
	addressability checks.
	(build_fold_block_addr_expr): New function.  Returns an expression
	representing the address of its operand for the purposes of a block
	oriented operation.
	(gimplify_modify_expr_to_memcpy): Use build_fold_block_addr_expr
	instead of build_fold_addr_expr to capture the operands addresses.
	(gimplify_modify_expr_to_memset): Likewise.
	(gimplify_variable_sized_compare): Likewise.
	(gimplify_modify_expr): For assignments we turn into memcpy or memset,
	check that both source and destination are block-addressable.
	(gimplify_addr_expr) <VIEW_CONVERT_EXPR operand>: If the input
	ADDR_EXPR is a block address, so is the transposed operand address we
	<default case>: If we are gimplifying a BLOCK_ADDRESS_P expression,
	use is_gimple_block_addressable instead of is_gimple_addressable as
	the predicate for our operand gimplification.
	* builtins.c (get_memory_rtx): Accept BLKmode DECL_BIT_FIELDs in

	* gnat.dg/packed_varsize_copy.adb: New test.



Attachment: block_address.dif
Description: Text document

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