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]

Re: [PING] tighten gimple addressability checks

Richard Guenther wrote:
> "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."
> Can you elaborate on this somewhat?

 Sure. Consider something like

 <<  type Data_T is array (1 .. 2) of Integer;

     type Block_T is record
	Bump : Character;
	Data : Data_T;
     end record;
     pragma Pack (Block_T);

     function Match (B1, B2 : Block_T; Size : Natural) return Boolean is
	return B1.Data (1 .. Size) = B2.Data (1 .. Size);

 The field_decl for 'Data' is typically packed at byte position 1 and has
 DECL_BIT_FIELD set on STRICT_ALIGNMENT targets because it is misaligned wrt
 its type. Then, for instance on sparc, B1.Data is a COMPONENT_REF with

     <field_decl 0xb7c91600 data type <array_type 0xb7c9cb64 x__data_t>
        external packed bit-field BLK size <integer_cst 0xb7c73480 64> ...
        align 8 offset_align 64
        offset <integer_cst 0xb7c73750 constant invariant 0>
        bit offset <integer_cst 0xb7c73144 constant invariant 8> 

 The BLKmode variable size comparison is handled with a call to memcmp, and
 the gimple output from the current mainline reads:

   D.254 = &b1->data[1 ...]{lb: 1 sz: 4};
   D.255 = &b2->data[1 ...]{lb: 1 sz: 4};
   D.256 = __builtin_memcmp (D.254, D.255, D.231);

 Such address taking is potentially troublesome in the general case because
 the address is not aligned enough for the type. The thought is that it's fine
 for the specific purpose of block oriented operations like memcmp. IOW, '.data'
 is not generally addressable but it's addressable (enough) for such a purpose.

 If we omit this subtelty and declare every field_decl with DECL_BIT_FIELD set
 not addressable, the gimplifier tries to create temporaries for ADDR_EXPR arguments.
 For the testcase at hand, this would abort because the size isn't constant. Even
 if it had somehow succeeded (in this or another context), this would have made a
 useless copy.

> I believe that for
> bool
> is_gimple_addressable (tree t)
> {
>   return (is_gimple_id (t) || handled_component_p (t)
>           || INDIRECT_REF_P (t));
> }
> there is indeed the defect that this allows BIT_FIELD_REF.

 Right, and our issue is a variant: it also allows any COMPONENT_REF, whatever
 the field position and the address-taking operation purpose.

 Of course, the key is

> I don't think that the middle-end expects a sub-reference of a
> non-aligned unit

 At least it used to in the GCC 3.4 days and many parts of it (rtl expansion
 comes to mind) still do. For instance,  for

 <<  type Data_T is array (1 .. 1) of Integer;

     type Block_T is record
	Bump : Boolean;
	Data : Data_T;
     end record;
     pragma Pack (Block_T);

     procedure Process (B : Block_T) is
	X : Integer;
	X := B.Data (1);

 we get down to expand_expr_real_1 with 'exp' like

  <array_ref 0xb7fd1208
    type <integer_type 0xb7d474a4 integer

    arg 0 <component_ref 0xb7d292f8
        type <array_type 0xb7d47b64 x__data_t type <integer_type 0xb7d474a4 integer>
        arg 0 <indirect_ref 0xb7d48680 type <record_type 0xb7d47dec x__block_t>
            readonly static arg 0 <parm_decl 0xb7d45150 b>>
        arg 1 <field_decl 0xb7d3c600 data type <array_type 0xb7d47b64 x__data_t>
            external packed bit-field nonaddressable SI
            size <integer_cst 0xb7d1e33c 32> unit size <integer_cst 0xb7d1e090 4>
            align 1 offset_align 64
            offset <integer_cst 0xb7d1e750 constant invariant 0>
            bit offset <integer_cst 0xb7d1e798 constant invariant 1>

    arg 1 <integer_cst 0xb7d494a4 type <integer_type 0xb7d47a8c> constant invariant 1>>

 and it works just fine.
> (that is, your claim that we would need to recurse sounds like
> another Ada speciality - *sigh*).

 I don't see anything explicitly forbidding those constructs in 'tree' and I'm
 pretty sure we have been generating for a loooong time. Now, maybe only Ada is
 rich enough to let the user exercise them :)

 This is actually one of the hottest spots we noticed in moving to GCC 4: Ada's
 ability to "generate" bitfields of aggregate types revealed a number of places
 where this was visibly unexpected by the new infrastructure. We have fixed a
 couple already, for instance: 
   (with ping, followups and commits elswhere)

> But without some elaboration on why we cannot just fix
> is_gimple_addressable I would go with such a patch instead.

 Sure. Let me know if you need more input.

 Thanks much for your feedback,





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