| Message ID | 53D4A9A2.2050402@LimeGreenSocks.com |
|---|---|
| State | New |
| Headers | show |
On 07/27/14 01:26, David Wohlferd wrote: > I'm not sure which maintainer to cc for inline asm stuff? > > I have a release on file with the FSF, but don't have SVN write access. > > Problem: > extract_insn() in recog.c will ICE if (noperands > MAX_RECOG_OPERANDS). > Normally this isn't a problem since expand_asm_operands() in cfgexpand.c > catches and reports a proper error for this condition. However, > expand_asm_operands() only checks (ninputs + noutputs) instead of > (ninputs + noutputs + nlabels), so you can get the ICE when using "asm > goto." See the bugzilla entry for sample code. > > ChangeLog: > 2014-07-27 David Wohlferd <dw@LimeGreenSocks.com> > > PR target/61692 > * cfgexpand.c (expand_asm_operands): Count all inline asm > parameters. You should also include 'nclobbers'. jeff
On 7/28/2014 12:42 PM, Jeff Law wrote: > On 07/27/14 01:26, David Wohlferd wrote: >> I'm not sure which maintainer to cc for inline asm stuff? >> >> I have a release on file with the FSF, but don't have SVN write access. >> >> Problem: >> extract_insn() in recog.c will ICE if (noperands > MAX_RECOG_OPERANDS). >> Normally this isn't a problem since expand_asm_operands() in cfgexpand.c >> catches and reports a proper error for this condition. However, >> expand_asm_operands() only checks (ninputs + noutputs) instead of >> (ninputs + noutputs + nlabels), so you can get the ICE when using "asm >> goto." See the bugzilla entry for sample code. >> >> ChangeLog: >> 2014-07-27 David Wohlferd <dw@LimeGreenSocks.com> >> >> PR target/61692 >> * cfgexpand.c (expand_asm_operands): Count all inline asm >> parameters. > You should also include 'nclobbers'. Reading thru asm_noperands (which is what extract_insn uses to count operands), I would have thought you were right. But while making this fail with nLabels was easy, I wasn't able to get this to ICE at all using clobbers (30 labels + 11 clobbers still didn't ICE). And I'm reluctant to propose that change unless I can see it fail. dw
On 07/28/14 16:39, David Wohlferd wrote: > > On 7/28/2014 12:42 PM, Jeff Law wrote: >> On 07/27/14 01:26, David Wohlferd wrote: >>> I'm not sure which maintainer to cc for inline asm stuff? >>> >>> I have a release on file with the FSF, but don't have SVN write access. >>> >>> Problem: >>> extract_insn() in recog.c will ICE if (noperands > MAX_RECOG_OPERANDS). >>> Normally this isn't a problem since expand_asm_operands() in cfgexpand.c >>> catches and reports a proper error for this condition. However, >>> expand_asm_operands() only checks (ninputs + noutputs) instead of >>> (ninputs + noutputs + nlabels), so you can get the ICE when using "asm >>> goto." See the bugzilla entry for sample code. >>> >>> ChangeLog: >>> 2014-07-27 David Wohlferd <dw@LimeGreenSocks.com> >>> >>> PR target/61692 >>> * cfgexpand.c (expand_asm_operands): Count all inline asm >>> parameters. >> You should also include 'nclobbers'. > > Reading thru asm_noperands (which is what extract_insn uses to count > operands), I would have thought you were right. But while making this > fail with nLabels was easy, I wasn't able to get this to ICE at all > using clobbers (30 labels + 11 clobbers still didn't ICE). > > And I'm reluctant to propose that change unless I can see it fail. I understand, but I'm still quite confident it's the right thing to do. Running that 30 label + 11 clobber testcase under valgrind might show the problem, if you can stand waiting that long... Also, please include the testcase you had nlabels part. Jeff
On 7/30/2014 9:58 PM, Jeff Law wrote: > On 07/28/14 16:39, David Wohlferd wrote: >> >> On 7/28/2014 12:42 PM, Jeff Law wrote: >>> On 07/27/14 01:26, David Wohlferd wrote: >>>> I'm not sure which maintainer to cc for inline asm stuff? >>>> >>>> I have a release on file with the FSF, but don't have SVN write >>>> access. >>>> >>>> Problem: >>>> extract_insn() in recog.c will ICE if (noperands > >>>> MAX_RECOG_OPERANDS). >>>> Normally this isn't a problem since expand_asm_operands() in >>>> cfgexpand.c >>>> catches and reports a proper error for this condition. However, >>>> expand_asm_operands() only checks (ninputs + noutputs) instead of >>>> (ninputs + noutputs + nlabels), so you can get the ICE when using "asm >>>> goto." See the bugzilla entry for sample code. >>>> >>>> ChangeLog: >>>> 2014-07-27 David Wohlferd <dw@LimeGreenSocks.com> >>>> >>>> PR target/61692 >>>> * cfgexpand.c (expand_asm_operands): Count all inline asm >>>> parameters. >>> You should also include 'nclobbers'. >> >> Reading thru asm_noperands (which is what extract_insn uses to count >> operands), I would have thought you were right. But while making this >> fail with nLabels was easy, I wasn't able to get this to ICE at all >> using clobbers (30 labels + 11 clobbers still didn't ICE). >> >> And I'm reluctant to propose that change unless I can see it fail. > I understand, but I'm still quite confident it's the right thing to > do. Running that 30 label + 11 clobber testcase under valgrind might > show the problem, if you can stand waiting that long... I'd love to. Unfortunately, my platform doesn't support valgrind. > Also, please include the testcase you had nlabels part. I have created the testcase for the 31 labels problem. However, not so much for the nclobbers part. And if I'm going to patch both, I should have testcases for both. I also worry about potentially breaking existing code. While the 31 labels thing I proposed won't break existing code (it would have been ICE-ing), adding nclobbers to the count of parameters could do so. When Jeff Law says that something is so about gcc, I tend to believe that it is so. But, unless I can see an example of the actual problem here, I have no idea how to create a test case for it. So I spent the last several hours hunting for it. If valgrind was going to show the problem, presumably we were expecting some type of array overrun. And there are a LOT of places that do foo[MAX_RECOG_OPERANDS]. Absent valgrind, I resorted to using printfs sprinkled throughout the code to check indices. However, despite my best efforts, I was unable to see any out of range accesses, gcc_asserts, segment faults or any other indications of a problem. All the places I tried seem to treat clobbers separately. That doesn't prove anything: I could easily have missed something. But I did make a sincere effort. Given how confident you are that there's a problem, could you point out a place+condition I should focus on? That way I can create the additional test case, satisfy my curiosity, and sleep soundly at night knowing I haven't (unnecessarily) broken people's code. Thanks, dw
On 08/01/14 02:07, David Wohlferd wrote: > > I'd love to. Unfortunately, my platform doesn't support valgrind. Ah. > >> Also, please include the testcase you had nlabels part. > > I have created the testcase for the 31 labels problem. However, not so > much for the nclobbers part. And if I'm going to patch both, I should > have testcases for both. Tell you what, pass along what you've got and I'll run it under valgrind here. I'm quite confident both need to be changed -- though it is possible nothing will trigger with the nclobbers stuff if it is indeed handled separately throughout the guts of GCC. Jeff
I sent you the file you requested (off list), but never heard back from you about the valgrind results. In an effort to move this along, I installed ubuntu under virtualbox and did a build of gcc. When running the output of this build with valgrind, I saw a number of memory *leaks* reported, but no overruns, despite having maxed out the operands + clobbers in a variety of ways. I have only tested this on x86, and only with inline asm, but I have had no luck (using code inspection, sprinkling printfs, and now valgrind) locating the error you are expecting to see. Without knowing what is making you "quite confident" there is a problem, I don't know what else to try. Suggestions? Theoretically I could add the nclobbers in "just in case." But unlike adding nlabels, adding nclobbers here will almost certainly break someone's code. I'm not prepared to do that unless there is a clear problem to be fixed, and I'm just not seeing it. If you are also out of ideas, I can re-send the patch for the original ninputs + noutputs + nlabels problem (along with the testcase you requested), and we can at least fix the known ICE. dw On 8/1/2014 11:29 AM, Jeff Law wrote: > On 08/01/14 02:07, David Wohlferd wrote: >> >> I'd love to. Unfortunately, my platform doesn't support valgrind. > Ah. > >> >>> Also, please include the testcase you had nlabels part. >> >> I have created the testcase for the 31 labels problem. However, not so >> much for the nclobbers part. And if I'm going to patch both, I should >> have testcases for both. > Tell you what, pass along what you've got and I'll run it under > valgrind here. I'm quite confident both need to be changed -- though > it is possible nothing will trigger with the nclobbers stuff if it is > indeed handled separately throughout the guts of GCC. > > > Jeff >
On 09/14/14 02:13, David Wohlferd wrote: > I sent you the file you requested (off list), but never heard back from > you about the valgrind results. Just haven't got back to it yet... There's always more to get done on any given day than I have the time for, so I have to prioritize and some things get put on the back burner until I can get back to them. > > In an effort to move this along, I installed ubuntu under virtualbox and > did a build of gcc. When running the output of this build with > valgrind, I saw a number of memory *leaks* reported, but no overruns, > despite having maxed out the operands + clobbers in a variety of ways. OK. Something else must be in play here. Clobbers can be somewhat special in that they may not always been an operand in the usual sense. ie, often they'll just be attached to the insn as an explicit blob of RTL, that's the only thing I can think of that would be preventing us from losing here. > > If you are also out of ideas, I can re-send the patch for the original > ninputs + noutputs + nlabels problem (along with the testcase you > requested), and we can at least fix the known ICE. Let's go with your original inputs + outputs + labels change and punt the clobbers stuff for now. jeff
Index: cfgexpand.c =================================================================== --- cfgexpand.c (revision 212900) +++ cfgexpand.c (working copy) @@ -2554,7 +2554,7 @@ } ninputs += ninout; - if (ninputs + noutputs > MAX_RECOG_OPERANDS) + if (ninputs + noutputs + nlabels > MAX_RECOG_OPERANDS) { error ("more than %d operands in %<asm%>", MAX_RECOG_OPERANDS); return;
I'm not sure which maintainer to cc for inline asm stuff? I have a release on file with the FSF, but don't have SVN write access. Problem: extract_insn() in recog.c will ICE if (noperands > MAX_RECOG_OPERANDS). Normally this isn't a problem since expand_asm_operands() in cfgexpand.c catches and reports a proper error for this condition. However, expand_asm_operands() only checks (ninputs + noutputs) instead of (ninputs + noutputs + nlabels), so you can get the ICE when using "asm goto." See the bugzilla entry for sample code. ChangeLog: 2014-07-27 David Wohlferd <dw@LimeGreenSocks.com> PR target/61692 * cfgexpand.c (expand_asm_operands): Count all inline asm parameters. dw