Re: [patch] Fix wrong code with VCE to bit-field type at -O

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Feb 11, 2014 at 2:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Feb 11, 2014 at 02:17:04PM +0100, Richard Biener wrote:
>> > this is an interesting regression from GCC 4.5 present on all active branches
>> > and triggered by recent SRA improvements.  For the attached testcase, we have
>> > an unchecked conversion of a 3-byte slice of an array of 4 bytes to a record
>> > type containing a 3-byte bit-field; an unchecked conversion in this case
>> > directly translates into a VIEW_CONVERT_EXPR.  Then SRA scalarizes the bit-
>> > field and turns the original VCE into a VCE of the 3-byte slice to the bit-
>> > field type, which is a 32-bit integer with precision 24.
>> >
>> > But the expansion of VCE isn't prepared for this and generates a full 32-bit
>> > read, which thus reads 1 additional byte and doesn't mask it afterwards, thus
>> > resulting in a wrong value for the scalarized bit-field.
>> >
>> > Proposed fix attached, tested on x86-64/Linux, OK for the mainline?
>>
>> Hmm.  The intent was of course to only allow truly no-op converts via
>> VIEW_CONVERT_EXPR - that is, the size of the operand type and the
>> result type should be the same.  So, isn't SRA doing it wrong when
>> creating the VIEW_CONVERT_EXPR of a 3-byte type to a 4-byte type?
>>
>> The verification we do in tree-cfg.c:verify_types_in_gimple_reference
>> hints at that we _do_ have even grosser mismatches - so reality may
>> trump desired design here.
>
> I thought we only allow VCE if the bitsize of both types is the same.
> If you have different bitsizes, then supposedly VCE to same bitsize integer
> followed by zero/sign extension or truncation followed by another VCE should
> be used.

Yeah, but the verification code is conveniently "crippled":

      if (TREE_CODE (expr) == VIEW_CONVERT_EXPR)
        {
          /* For VIEW_CONVERT_EXPRs which are allowed here too, we only check
             that their operand is not an SSA name or an invariant when
             requiring an lvalue (this usually means there is a SRA or IPA-SRA
             bug).  Otherwise there is nothing to verify, gross mismatches at
             most invoke undefined behavior.  */
          if (require_lvalue
              && (TREE_CODE (op) == SSA_NAME
                  || is_gimple_min_invariant (op)))
            {
              error ("conversion of an SSA_NAME on the left hand side");
              debug_generic_stmt (expr);
              return true;
            }
          else if (TREE_CODE (op) == SSA_NAME
                   && TYPE_SIZE (TREE_TYPE (expr)) != TYPE_SIZE
(TREE_TYPE (op)))
            {
              error ("conversion of register to a different size");
              debug_generic_stmt (expr);
              return true;
            }

thus that only applies to register VIEW_CONVERT_EXPRs.  But maybe
we two are missing sth here - it's an Ada testcase after all ;)

Richard.

>         Jakub