| Message ID | ac791b27-1fba-393b-7be3-09f37696dbe9@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [stage,1] remove unreachable code in expand_expr_real_1 (PR 21433) | expand |
On Fri, Feb 12, 2021 at 1:35 AM Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > While trawling through old bugs I came across one from 2005: PR 21433 > - The COMPONENT_REF case of expand_expr_real_1 is probably wrong. > > The report looks correct in that argument 0 in COMPONENT_REF cannot > be a CONSTRUCTOR. In my tests it's only been one of the following > codes: > > array_ref > component_ref > mem_ref > parm_decl > result_decl > var_decl > > The attached patch removes the CONSTRUCTOR code and replaces it with > an assert verifying it doesn't come up there. Besides testing on > x86_64-linux, the change is supported by comments in code and also > in the internals manual (although that looks incorrect and should > be changed to avoid suggesting the first operand is a decl). Note the CTOR operand is valid GENERIC and likely came up before we introduced GIMPLE. GIMPLE simply feeds more restrictive GENERIC to the RTL expansion routines nowadays (so the patch is OK eventually), but please avoid altering GENERIC or tree.def documentation which documents _GENERIC_. The restricted boundary of GIMPLE -> RTL expansion is not documented and in theory we might even run into your assert when processing global initializers (in case the CTOR ends up TREE_CONSTANT). > tree.def: > > /* Value is structure or union component. > Operand 0 is the structure or union (an expression). > Operand 1 is the field (a node of type FIELD_DECL). > Operand 2, if present, is the value of DECL_FIELD_OFFSET, measured > in units of DECL_OFFSET_ALIGN / BITS_PER_UNIT. */ > DEFTREECODE (COMPONENT_REF, "component_ref", tcc_reference, 3) > > generic.texi: > > @item COMPONENT_REF > These nodes represent non-static data member accesses. The first > operand is the object (rather than a pointer to it); the second operand > is the @code{FIELD_DECL} for the data member. The third operand represents > the byte offset of the field, but should not be used directly; call > @code{component_ref_field_offset} instead.
On 2/12/21 1:55 AM, Richard Biener wrote: > On Fri, Feb 12, 2021 at 1:35 AM Martin Sebor via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> While trawling through old bugs I came across one from 2005: PR 21433 >> - The COMPONENT_REF case of expand_expr_real_1 is probably wrong. >> >> The report looks correct in that argument 0 in COMPONENT_REF cannot >> be a CONSTRUCTOR. In my tests it's only been one of the following >> codes: >> >> array_ref >> component_ref >> mem_ref >> parm_decl >> result_decl >> var_decl >> >> The attached patch removes the CONSTRUCTOR code and replaces it with >> an assert verifying it doesn't come up there. Besides testing on >> x86_64-linux, the change is supported by comments in code and also >> in the internals manual (although that looks incorrect and should >> be changed to avoid suggesting the first operand is a decl). > > Note the CTOR operand is valid GENERIC and likely came up before > we introduced GIMPLE. GIMPLE simply feeds more restrictive GENERIC > to the RTL expansion routines nowadays (so the patch is OK > eventually), but please > avoid altering GENERIC or tree.def documentation which documents > _GENERIC_. I have committed the patch in r12-728. Martin > > The restricted boundary of GIMPLE -> RTL expansion is not documented > and in theory we might even run into your assert when processing > global initializers (in case the CTOR ends up TREE_CONSTANT). > >> tree.def: >> >> /* Value is structure or union component. >> Operand 0 is the structure or union (an expression). >> Operand 1 is the field (a node of type FIELD_DECL). >> Operand 2, if present, is the value of DECL_FIELD_OFFSET, measured >> in units of DECL_OFFSET_ALIGN / BITS_PER_UNIT. */ >> DEFTREECODE (COMPONENT_REF, "component_ref", tcc_reference, 3) >> >> generic.texi: >> >> @item COMPONENT_REF >> These nodes represent non-static data member accesses. The first >> operand is the object (rather than a pointer to it); the second operand >> is the @code{FIELD_DECL} for the data member. The third operand represents >> the byte offset of the field, but should not be used directly; call >> @code{component_ref_field_offset} instead.
PR middle-end/21433 - The COMPONENT_REF case of expand_expr_real_1 is probably wrong gcc/ChangeLog: PR middle-end/21433 * expr.c (expand_expr_real_1): Replace unreachable code with an assert. diff --git a/gcc/expr.c b/gcc/expr.c index 86dc1b6c973..7d5efe5722a 100644 --- a/gcc/expr.c +++ b/gcc/expr.c @@ -10767,61 +10767,8 @@ expand_expr_real_1 (tree exp, rtx target, machine_mode tmode, goto normal_inner_ref; case COMPONENT_REF: - /* If the operand is a CONSTRUCTOR, we can just extract the - appropriate field if it is present. */ - if (TREE_CODE (treeop0) == CONSTRUCTOR) - { - unsigned HOST_WIDE_INT idx; - tree field, value; - scalar_int_mode field_mode; - - FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (treeop0), - idx, field, value) - if (field == treeop1 - /* We can normally use the value of the field in the - CONSTRUCTOR. However, if this is a bitfield in - an integral mode that we can fit in a HOST_WIDE_INT, - we must mask only the number of bits in the bitfield, - since this is done implicitly by the constructor. If - the bitfield does not meet either of those conditions, - we can't do this optimization. */ - && (! DECL_BIT_FIELD (field) - || (is_int_mode (DECL_MODE (field), &field_mode) - && (GET_MODE_PRECISION (field_mode) - <= HOST_BITS_PER_WIDE_INT)))) - { - if (DECL_BIT_FIELD (field) - && modifier == EXPAND_STACK_PARM) - target = 0; - op0 = expand_expr (value, target, tmode, modifier); - if (DECL_BIT_FIELD (field)) - { - HOST_WIDE_INT bitsize = TREE_INT_CST_LOW (DECL_SIZE (field)); - scalar_int_mode imode - = SCALAR_INT_TYPE_MODE (TREE_TYPE (field)); - - if (TYPE_UNSIGNED (TREE_TYPE (field))) - { - op1 = gen_int_mode ((HOST_WIDE_INT_1 << bitsize) - 1, - imode); - op0 = expand_and (imode, op0, op1, target); - } - else - { - int count = GET_MODE_PRECISION (imode) - bitsize; - - op0 = expand_shift (LSHIFT_EXPR, imode, op0, count, - target, 0); - op0 = expand_shift (RSHIFT_EXPR, imode, op0, count, - target, 0); - } - } - - return op0; - } - } - goto normal_inner_ref; - + gcc_assert (TREE_CODE (treeop0) != CONSTRUCTOR); + /* Fall through. */ case BIT_FIELD_REF: case ARRAY_RANGE_REF: normal_inner_ref:
While trawling through old bugs I came across one from 2005: PR 21433 - The COMPONENT_REF case of expand_expr_real_1 is probably wrong. The report looks correct in that argument 0 in COMPONENT_REF cannot be a CONSTRUCTOR. In my tests it's only been one of the following codes: array_ref component_ref mem_ref parm_decl result_decl var_decl The attached patch removes the CONSTRUCTOR code and replaces it with an assert verifying it doesn't come up there. Besides testing on x86_64-linux, the change is supported by comments in code and also in the internals manual (although that looks incorrect and should be changed to avoid suggesting the first operand is a decl). tree.def: /* Value is structure or union component. Operand 0 is the structure or union (an expression). Operand 1 is the field (a node of type FIELD_DECL). Operand 2, if present, is the value of DECL_FIELD_OFFSET, measured in units of DECL_OFFSET_ALIGN / BITS_PER_UNIT. */ DEFTREECODE (COMPONENT_REF, "component_ref", tcc_reference, 3) generic.texi: @item COMPONENT_REF These nodes represent non-static data member accesses. The first operand is the object (rather than a pointer to it); the second operand is the @code{FIELD_DECL} for the data member. The third operand represents the byte offset of the field, but should not be used directly; call @code{component_ref_field_offset} instead.