| Message ID | 1520a6f9-6773-b971-5111-de6c588fdff9@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed] prune warning from test output to avoid arm-none-eabi failure (PR 83303) | expand |
On 12/06/2017 12:27 PM, Martin Sebor wrote: > For reference, I committed r255450 to avoid the test failure > reported on the arm-none-eabi target. The warning is valid > but unrelated to the purpose of the test so I pruned it from > its output. > > Martin > > Index: gcc/testsuite/ChangeLog > =================================================================== > --- gcc/testsuite/ChangeLog (revision 255449) > +++ gcc/testsuite/ChangeLog (working copy) > @@ -9,6 +9,9 @@ > > 2017-12-06 Martin Sebor <msebor@redhat.com> > > + PR testsuite/83303 > + * g++.dg/opt/new1.C: Prune warning from test output. The path to the erroneous behavior should be caught and eliminated by DOM. That's the whole point behind one of the recent changes. Instead we need to figure out why it wasn't caught on arm-none-eabi. jeff
On 12/06/2017 12:41 PM, Jeff Law wrote: > On 12/06/2017 12:27 PM, Martin Sebor wrote: >> For reference, I committed r255450 to avoid the test failure >> reported on the arm-none-eabi target. The warning is valid >> but unrelated to the purpose of the test so I pruned it from >> its output. >> >> Martin >> >> Index: gcc/testsuite/ChangeLog >> =================================================================== >> --- gcc/testsuite/ChangeLog (revision 255449) >> +++ gcc/testsuite/ChangeLog (working copy) >> @@ -9,6 +9,9 @@ >> >> 2017-12-06 Martin Sebor <msebor@redhat.com> >> >> + PR testsuite/83303 >> + * g++.dg/opt/new1.C: Prune warning from test output. > The path to the erroneous behavior should be caught and eliminated by > DOM. That's the whole point behind one of the recent changes. > > Instead we need to figure out why it wasn't caught on arm-none-eabi. Yes, I should have suggested to look into the failure to do that, independently of the test. I can open a new bug for it unless it's already on your to-do list. Martin
On 12/06/2017 12:58 PM, Martin Sebor wrote: > On 12/06/2017 12:41 PM, Jeff Law wrote: >> On 12/06/2017 12:27 PM, Martin Sebor wrote: >>> For reference, I committed r255450 to avoid the test failure >>> reported on the arm-none-eabi target. The warning is valid >>> but unrelated to the purpose of the test so I pruned it from >>> its output. >>> >>> Martin >>> >>> Index: gcc/testsuite/ChangeLog >>> =================================================================== >>> --- gcc/testsuite/ChangeLog (revision 255449) >>> +++ gcc/testsuite/ChangeLog (working copy) >>> @@ -9,6 +9,9 @@ >>> >>> 2017-12-06 Martin Sebor <msebor@redhat.com> >>> >>> + PR testsuite/83303 >>> + * g++.dg/opt/new1.C: Prune warning from test output. >> The path to the erroneous behavior should be caught and eliminated by >> DOM. That's the whole point behind one of the recent changes. >> >> Instead we need to figure out why it wasn't caught on arm-none-eabi. > > Yes, I should have suggested to look into the failure to do > that, independently of the test. I can open a new bug for > it unless it's already on your to-do list. I think the right thing to do is revert the change and keep 83303 open until we see what's different about arm-none-eabi. I only noticed this because I'd debugged that test on x86_64 and fixed DOM to eliminate the problem path over the weekend... So something is clearly different on that platform. jeff
On 12/06/2017 12:41 PM, Jeff Law wrote: > On 12/06/2017 12:27 PM, Martin Sebor wrote: >> For reference, I committed r255450 to avoid the test failure >> reported on the arm-none-eabi target. The warning is valid >> but unrelated to the purpose of the test so I pruned it from >> its output. >> >> Martin >> >> Index: gcc/testsuite/ChangeLog >> =================================================================== >> --- gcc/testsuite/ChangeLog (revision 255449) >> +++ gcc/testsuite/ChangeLog (working copy) >> @@ -9,6 +9,9 @@ >> >> 2017-12-06 Martin Sebor <msebor@redhat.com> >> >> + PR testsuite/83303 >> + * g++.dg/opt/new1.C: Prune warning from test output. > The path to the erroneous behavior should be caught and eliminated by > DOM. That's the whole point behind one of the recent changes. > > Instead we need to figure out why it wasn't caught on arm-none-eabi. Martin, can you look into why on x86_64 the initial code we generate for QScript::Buffer<T>::reserve looks like this: struct QScriptValueImpl * new_data; <<cleanup_point <<< Unknown tree: expr_stmt (void) (new_data = TARGET_EXPR <D.2456, operator new [] (SAVE_EXPR <(sizetype) ((struct Buffer *) this)->m_capacity> <= 2305843009213693950 ? (long unsigned int) (SAVE_EXPR <(sizetype) ((struct Buffer *) this)->m_capacity> * 4) : (long unsigned int) __cxa_throw_bad_array_new_length ())>;, try Note the test against capacity. Yet on arm-none-eabi we have: (void) (new_data = TARGET_EXPR <D.4742, operator new [] ((unsigned int) NON_LVALUE_EXPR <SAVE_EXPR <(sizetype) ((struct Buffer *) this)->m_capacity>>)>;, try This is important because after inlining the test that you see in the x86 version is guarded by a test (of the same object) for < 0. Naturally no object can satisfy both of those conditions. It may be easier to see it in the .gimple dump. For x86_64: QScript::Buffer<QScriptValueImpl>::reserve (struct Buffer * const this, int x) { void * D.2456; long unsigned int iftmp.0; sizetype D.2481; struct QScriptValueImpl * retval.1; struct QScriptValueImpl * D.2457; struct QScriptValueImpl * D.2458; long int D.2459; struct QScriptValueImpl * new_data; _1 = this->m_capacity; D.2481 = (sizetype) _1; if (D.2481 <= 2305843009213693950) goto <D.2482>; else goto <D.2483>; <D.2482>: iftmp.0 = D.2481 * 4; goto <D.2484>; <D.2483>: __cxa_throw_bad_array_new_length (); <D.2484>: D.2456 = operator new [] (iftmp.0); In contrast on arm we have the following: QScript::Buffer<QScriptValueImpl>::reserve (struct Buffer * const this, int x) { void * D.4742; sizetype D.4769; struct QScriptValueImpl * retval.0; struct QScriptValueImpl * D.4743; struct QScriptValueImpl * D.4744; int D.4745; struct QScriptValueImpl * new_data; _1 = this->m_capacity; D.4769 = (sizetype) _1; D.4742 = operator new [] (D.4769); This feels like something the front-end is doing to me. jeff
On 12/06/2017 03:58 PM, Jeff Law wrote: > On 12/06/2017 12:41 PM, Jeff Law wrote: >> On 12/06/2017 12:27 PM, Martin Sebor wrote: >>> For reference, I committed r255450 to avoid the test failure >>> reported on the arm-none-eabi target. The warning is valid >>> but unrelated to the purpose of the test so I pruned it from >>> its output. >>> >>> Martin >>> >>> Index: gcc/testsuite/ChangeLog >>> =================================================================== >>> --- gcc/testsuite/ChangeLog (revision 255449) >>> +++ gcc/testsuite/ChangeLog (working copy) >>> @@ -9,6 +9,9 @@ >>> >>> 2017-12-06 Martin Sebor <msebor@redhat.com> >>> >>> + PR testsuite/83303 >>> + * g++.dg/opt/new1.C: Prune warning from test output. >> The path to the erroneous behavior should be caught and eliminated by >> DOM. That's the whole point behind one of the recent changes. >> >> Instead we need to figure out why it wasn't caught on arm-none-eabi. > Martin, can you look into why on x86_64 the initial code we generate for > QScript::Buffer<T>::reserve looks like this: There's a difference between the underlying type and size of the QScript::Type enum in the ABIs. On arm-non-eabi the size of its underlying type is 1. On arm-linux-gnueabi and x86_64 the size is 4. The C++ front end only inserts a length check for the new expression when the element size is more than 1. This is to avoid unsigned wrapping when computing the number of bytes to allocate, as N * sizeof (T). With the arm-non-eabi cross compiler the warning disappears when the size of the enum is more than 1. Similarly, changing the size of the enum is changed to 1 makes the warning appear on arm-linux-gnueabi as well as a native x86_64 compiler. I think this confirms that the test is not useful to validate the range work (it's meant to verify the absence of an ICE) and warnings emitted for it aren't necessarily reliable indicators one way or the other. As I said in the PR, I think issuing a warning for the test would actually be helpful given that the code will unavoidably abort at runtime. > This feels like something the front-end is doing to me. Yes, the C++ front end has conspired with the ABI to bite us both. Martin
Index: gcc/testsuite/ChangeLog =================================================================== --- gcc/testsuite/ChangeLog (revision 255449) +++ gcc/testsuite/ChangeLog (working copy) @@ -9,6 +9,9 @@ 2017-12-06 Martin Sebor <msebor@redhat.com> + PR testsuite/83303 + * g++.dg/opt/new1.C: Prune warning from test output. + PR tree-optimization/82646 * gcc.dg/builtin-stringop-chk-1.c: Adjust. * gcc.dg/builtin-stringop-chk-9.c: New test. Index: gcc/testsuite/g++.dg/opt/new1.C =================================================================== --- gcc/testsuite/g++.dg/opt/new1.C (revision 255449) +++ gcc/testsuite/g++.dg/opt/new1.C (working copy) @@ -1,4 +1,4 @@ -// PR c++/39367 +// PR c++/39367 - ICE at tree-inline.c:1042 with -O // { dg-options "-O" } class QScriptEnginePrivate; @@ -37,6 +37,11 @@ template <typename T> void QScript::Buffer<T>::res reserve (s << 1); } template <typename T> void QScript::Buffer<T>::reserve(int x) { + /* The following may be optimized into a trap because the function + is called from resize(0) and so with m_capacity < 0. When not + optimized it may trigger -Walloc-size-larger-than= since + operator new() is called with an excessively large value. + The warning is pruned from the test output below. */ T *new_data = new T[m_capacity]; for (int i=0; i<m_size; ++i) new_data[i] = m_data[i]; @@ -69,3 +74,5 @@ namespace QScript { } } } + +// { dg-prune-output "\\\[-Walloc-size-larger-than=]" }