| Message ID | 14008801.HoYYt5Iu7g@polaris |
|---|---|
| State | New |
| Headers | show |
On 12/09/13 04:16, Eric Botcazou wrote: > Hi, > > the new test gnat.dg/pack19.adb doesn't pass on some platforms because of the > target-dependent result of loads from bit-fields with size 0. > > Unlike the stores to these bit-fields which are handled in an uniform way in > store_field: > > /* If we have nothing to store, do nothing unless the expression has > side-effects. */ > if (bitsize == 0) > return expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL); > > the result of the loads depends on SHIFT_COUNT_TRUNCATED: the result is 0 if > the macro is 0, otherwise it's garbage. The attached patch makes it so that > the result is always 0 independently of the target. > > Tested on x86/Linux and PowerPC/Linux, OK for the mainline? > > > 2013-12-09 Eric Botcazou <ebotcazou@adacore.com> > > * expr.c (expand_expr_real_1) <normal_inner_ref>: Always return 0 for > the extraction of a bit-field of null size. But isn't the test bogus if it depends on the result of loading a zero sized bitfield? Jeff
> But isn't the test bogus if it depends on the result of loading a zero > sized bitfield? That's supported in Ada and the result is specified, it's the only allowed value of the type of the bitfield (the type has precision 0 and is biased if this value isn't zero). So this boils down to an implementation choice: either we specifically detect this case up in the chain or we let it flow down as usual. But since we handle the stores in an uniform way, I think that it could make sense to do the same for the loads.
On 12/09/13 15:41, Eric Botcazou wrote: >> But isn't the test bogus if it depends on the result of loading a zero >> sized bitfield? > > That's supported in Ada and the result is specified, it's the only allowed > value of the type of the bitfield (the type has precision 0 and is biased if > this value isn't zero). So this boils down to an implementation choice: > either we specifically detect this case up in the chain or we let it flow down > as usual. But since we handle the stores in an uniform way, I think that it > could make sense to do the same for the loads. Ugh. I don't care that much :-) If you want to handle it, go ahead. I would suggest a comment indicating why we've chosen to handle it. The only question left is would it be better to handle it in extract_bit_field to catch other cases, or is that too late? jeff
> Ugh. I don't care that much :-) If you want to handle it, go ahead. I > would suggest a comment indicating why we've chosen to handle it. OK, I can do that. > The only question left is would it be better to handle it in > extract_bit_field to catch other cases, or is that too late? I put it there because there is a similar bitsize == 0 case just above: if (ext_mode == BLKmode) { if (target == 0) target = assign_temp (type, 1, 1); if (bitsize == 0) return target; Not clear how we can reach here though, I could add a comment if I find. :-)
On 12/09/13 16:17, Eric Botcazou wrote: >> Ugh. I don't care that much :-) If you want to handle it, go ahead. I >> would suggest a comment indicating why we've chosen to handle it. > > OK, I can do that. > >> The only question left is would it be better to handle it in >> extract_bit_field to catch other cases, or is that too late? > > I put it there because there is a similar bitsize == 0 case just above: > > if (ext_mode == BLKmode) > { > if (target == 0) > target = assign_temp (type, 1, 1); > > if (bitsize == 0) > return target; > > Not clear how we can reach here though, I could add a comment if I find. :-) :-) From a cleanup standpoint, I think the expansion code is ripe for someone to spend (condsiderable) time killing dead code. I suspect there's still significant gcc-1.91 (or even older) bits in there that have been dead for at least a decade. Jeff
Index: expr.c =================================================================== --- expr.c (revision 205774) +++ expr.c (working copy) @@ -10177,6 +10177,12 @@ expand_expr_real_1 (tree exp, rtx target return target; } + /* If we have nothing to extract, the result will be 0 for targets + with SHIFT_COUNT_TRUNCATED == 0 and garbage otherwise. Always + return 0 for the sake of consistency. */ + if (bitsize == 0) + return const0_rtx; + op0 = validize_mem (op0); if (MEM_P (op0) && REG_P (XEXP (op0, 0)))
Hi, the new test gnat.dg/pack19.adb doesn't pass on some platforms because of the target-dependent result of loads from bit-fields with size 0. Unlike the stores to these bit-fields which are handled in an uniform way in store_field: /* If we have nothing to store, do nothing unless the expression has side-effects. */ if (bitsize == 0) return expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL); the result of the loads depends on SHIFT_COUNT_TRUNCATED: the result is 0 if the macro is 0, otherwise it's garbage. The attached patch makes it so that the result is always 0 independently of the target. Tested on x86/Linux and PowerPC/Linux, OK for the mainline? 2013-12-09 Eric Botcazou <ebotcazou@adacore.com> * expr.c (expand_expr_real_1) <normal_inner_ref>: Always return 0 for the extraction of a bit-field of null size.