Hi Martin, On Tue, 17 Sep 2013 01:09:45, Martin Jambor wrote: > Hi, > > On Sun, Sep 15, 2013 at 06:55:17PM +0200, Bernd Edlinger wrote: >> Hello Richard, >> >> attached is my second attempt at fixing PR 57748. This time the >> movmisalign path is completely removed and a similar bug in the read >> handling of misaligned structures with a non-BLKmode is fixed >> too. There are several new test cases for the different possible >> failure modes. > > I think the third and fourth testcases are undefined as the > description of zero-length arrays extension clearly says the whole > thing only makes sense when used as the last field of the > outermost-aggregate type. I have not really understood what the third > testcase is supposed to test but I did not try too much. Instead of > the fourth testcase, you can demonstrate the need for your change in > expand_expr_real_1 by augmenting the original testcase a little like > in attached pr57748-m1.c.
The third test case tries to demonstrate the possible write data store race (by checking the assembler output). But you are right, this example is probably not valid C at all. I was actually worried about unions with non-BLK mode and a movmisalign optab handler. When you look at stor-layout.c (compute_record_mode) you'll see, that in the case of a union usually an integer mode is chosen, which is exactly the same size as the whole union. And just by chance this does not have a movmisalign optab. Therefore I tried to cheat with that zero-sized array, which should probably be rejected at stor-layout.c in the first place. When I tried to make a test case out of it, the bug on the read side hit me as a total surprise... > The hunk in expand_expr_real_1 can prove problematic if at any point > we need to pass some other modifier to the expansion of tem. I'll try > to see if I can come up with a testcase tomorrow. But perhaps we > never do (and can hope we never will) and then it would be sort of > OKish (note that I cannot approve anything) even though it can > pessimize unaligned access paths (by not using movmisalign_optab even > when perfectly possible - which is always when there is no zero sized > array). It really just shows how evil non-BLKmode structures with > zero-sized arrays are and how they complicate things. The expansion > of component_refs is reasonably built around the assumption that we'd > expand the structure in its mode in the most efficient manner and then > chuck the correct part out of it, but here we need to tell the > expansion of the structure to hold itself back because we'll be > looking outside of the structure (as specified by mode). I too am under the very strong impression that this was not the intention of the design to use a non-BLKmode on a structure with zero-sized arrays. > I'm not sure to what extent the hunk adding tests for bitregion_start > and bitregion_end being zero is connected to this issue. I do not see > any of the testcases exercising that path. If it is indeed another > problem, I think it should be submitted (and potentially committed) as > a separate patch, preferably with a testcase. Yes, you're probably right. I was unable to find a test case where this code path executes with bitregions. As I said, it maybe possible to prove that bitregion_start and bitregion_end == 0 if the other conditions are satisfied. What is obvious, that it would cause problems to set bitpos=0 when bitregion_start/end is pointing elsewhere. It is however much easier to prove that not going into that code path would not cause any problems if bitregion_start/end is not zero. So this was just for safer programming, but probably no real bug. Thanks, Bernd. > Having said all that, I think that removing the misalignp path from > expand_assignment altogether is a good idea. I have verified that > when the expander is now presented with basically the same thing that > 4.7 choked on, expand_expr (..., EXPAND_WRITE) can cope with it (see > attached file c.c) and doing that simplifies this complex code path. > > Thanks, > > Martin > >> >> This patch was boot-strapped and regression tested on >> x86_64-unknown-linux-gnu >> and i686-pc-linux-gnu. >> >> Additionally I generated eCos and an eCos-application (on ARMv5 using packed >> structures) with an arm-eabi cross compiler, and looked for differences in >> the >> disassembled code with and without this patch, but there were none. >> >> OK for trunk? >> >> Regards >> Bernd. > >> 2013-09-15 Bernd Edlinger <bernd.edlin...@hotmail.de> >> >> PR middle-end/57748 >> * expr.c (expand_assignment): Remove misalignp code path. >> Check for bitregion in offset arithmetic. >> (expand_expr_real_1): Use EXAND_MEMORY on base object. >> >> testsuite: >> >> PR middle-end/57748 >> * gcc.dg/torture/pr57748-1.c: New test. >> * gcc.dg/torture/pr57748-2.c: New test. >> * gcc.dg/torture/pr57748-3.c: New test. >> * gcc.dg/torture/pr57748-3a.c: New test. >> * gcc.dg/torture/pr57748-4.c: New test. >> * gcc.dg/torture/pr57748-4a.c: New test. >> > >
