Hi, On Mon, 2 Dec 2013 15:55:08Richard Biener wrote: > > On Mon, Nov 25, 2013 at 1:07 PM, Bernd Edlinger > <bernd.edlin...@hotmail.de> wrote: >> Hello, >> >> I had forgotten to run the Ada test suite when I submitted the previous >> version of this patch. >> And indeed there were some Ada test cases failing because in Ada packed >> structures are >> like bit fields, but without the DECL_BIT_FIELD_TYPE attribute. > > I think they may have DECL_BIT_FIELD set though, not sure. > >> Please find attached the updated version of this patch. >> >> Boot-strapped and regression-tested on x86_64-linux-gnu. >> Ok for trunk? > > So you mimic what Eric added in get_bit_range? Btw, I'm not sure > the "conservative" way of failing get_bit_range with not limiting the > access at all is good. > > That is, we may want to do > > + /* The C++ memory model naturally applies to byte-aligned fields. > + However, if we do not have a DECL_BIT_FIELD_TYPE but BITPOS or > + BITSIZE are not byte-aligned, there is no need to limit the range > + we can access. This can occur with packed structures in Ada. */ > + if (bitregion_start == 0 && bitregion_end == 0 > + && bitsize> 0 > + && bitsize % BITS_PER_UNIT == 0 > + && bitpos % BITS_PER_UNIT == 0) > + { > + bitregion_start = bitpos; > + bitregion_end = bitpos + bitsize - 1; > + } > > thus not else if but also apply it when get_bit_range "failed" (as it may > fail for other reasons). A better fallback would be to track down > the outermost byte-aligned handled-component and limit the access > to that (though I guess Ada doesn't care at all about the C++ memory > model and only Ada has bit-aligned aggregates). >
Good question. Most of the time the expansion can not know if it expands Ada, C, or Fortran. In this case we know it can only be Ada, so the C++ memory model is not mandatory. Maybe Eric can tell, if a data store race condition may be an issue in Ada if structure is laid out like __attribute((packed,aligned(1))) I mean, if that is at all possible. > That said, the patch looks ok as-is to me, let's see if we can clean > things up for the next stage1. > Ok, applied as-is. Thanks Bernd. > Thanks, > Richard. > >> Bernd. >> >>> On Mon, 18 Nov 2013 11:37:05, Bernd Edlinger wrote: >>> >>> Hi, >>> >>> On Fri, 15 Nov 2013 13:30:51, Richard Biener wrote: >>>>> That looks like always pretending it is a bit field. >>>>> But it is not a bit field, and bitregion_start=bitregion_end=0 >>>>> means it is an ordinary value. >>>> >>>> I don't think it is supposed to mean that. It's supposed to mean >>>> "the access is unconstrained". >>>> >>> >>> Ok, agreed, I did not regard that as a feature. >>> And apparently only the code path in expand_assigment >>> really has a problem with it. >>> >>> So here my second attempt at fixing this. >>> >>> Boot-strapped and regression-tested on x86_64-linux-gnu. >>> >>> OK for trunk? >>> >>> >>> Thanks >>> Bernd.