This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch] Fix wrong code with VCE to bit-field type at -O
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Eric Botcazou <ebotcazou at adacore dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 11 Feb 2014 14:39:46 +0100
- Subject: Re: [patch] Fix wrong code with VCE to bit-field type at -O
- Authentication-results: sourceware.org; auth=none
- References: <1407954 dot 7L4M0YkQRE at polaris> <CAFiYyc2LoKr+iT68vxVP4AqbFVdMt6YQBo_JHydauHtQcHhmhg at mail dot gmail dot com> <20140211132210 dot GT20378 at tucnak dot redhat 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