| Message ID | 47244405-9a99-a24b-f2f1-6b18343ec18d@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 12/14/2016 03:56 PM, Martin Sebor wrote: > The -Wnonnull warning improvement (PR c/17308 - nonnull attribute > not as useful as it could be) causes a couple of false positives > in a powerpc64le bootstrap. The attached fix suppresses them. > It passes bootstrap on powerpc64le but tests are still running. > > I couldn't reproduce the bootstrap comparison failure that Segher > mentioned in his comment on the bug. I've gone over the patch > again to see if I could spot what could possibly be behind it > but couldn't really see anything. Jeff, you mentioned attribute > nonnull is exploited by the null pointer optimization. Do you > think it could possibly have this effect in GCC? It's certainly possible. It would be an indicator that something in GCC is passing a NULL pointer into a routine where it shouldn't at runtime and that GCC is using its new knowledge to optimize the code assuming that can't happen. ie, it's an indicator of a latent bug in GCC. Depending on the difficulty in tracking down, you may need to revert the change. This is exactly the kind of scenario I was concerned about in that approval message. > > Martin > > gcc-78817.diff > > > PR bootstrap/78817 - stage2 bootstrap failure in vec.h:1613:5: error: argument 1 null where non-null expected after r243661 > > gcc/ChangeLog: > > PR bootstrap/78817 > * vec.h (vec<T, A, vl_embed>::quick_grow_cleared): Assert postcondition. > ( vec<T, va_heap, vl_ptr>::safe_grow_cleared): Ditto. I'm pretty sure the vec<T, A, vl_embed> change is not needed. The address method for that type can't ever return NULL AFAICT. So there should be no way to see a NULL flowing into the memset call for that case. In the vec<T, va_heap, vl_ptr> case the address method can clearly return NULL: const T *address (void) const { return m_vec ? m_vec->m_vecdata : NULL; } I've only lightly tried to follow things down through the vec implementation. But it seems that if create a vec with no elements we can start with m_vec NULL. We can call safe_grow_cleared on such a vector. If we did and LEN was zero, then ISTM that m_vec will continue to be NULL if I follow the callgraph maze correctly. So there is a potential path to the address() call where m_vec is NULL and thus address() could return NULL. We happen to know that can't happen at runtime due to the if (sz != 0) check. That's a good indicator that jump threading has missed an opportunity. I'd probably need to look at the .i file as well as the dumps to be sure, but as is usually the case, there appears to be a missed optimization here leading to the false positive warning. The change to the vec<T, va_heap, vl_ptr> is OK. Can you please verify that if you install just that change that ppc bootstraps are restored and if so, please install. Thanks, Jeff
PR bootstrap/78817 - stage2 bootstrap failure in vec.h:1613:5: error: argument 1 null where non-null expected after r243661 gcc/ChangeLog: PR bootstrap/78817 * vec.h (vec<T, A, vl_embed>::quick_grow_cleared): Assert postcondition. ( vec<T, va_heap, vl_ptr>::safe_grow_cleared): Ditto. diff --git a/gcc/vec.h b/gcc/vec.h index aa93411..80a6ef0 100644 --- a/gcc/vec.h +++ b/gcc/vec.h @@ -1092,10 +1092,16 @@ inline void vec<T, A, vl_embed>::quick_grow_cleared (unsigned len) { unsigned oldlen = length (); - size_t sz = sizeof (T) * (len - oldlen); - quick_grow (len); - if (sz != 0) - memset (&(address ()[oldlen]), 0, sz); + gcc_checking_assert (oldlen <= len); + + if (size_t sz = sizeof (T) * (len - oldlen)) + { + quick_grow (len); + + T *p = address (); + gcc_assert (p != NULL); + memset (p + oldlen, 0, sz); + } } @@ -1607,10 +1613,16 @@ inline void vec<T, va_heap, vl_ptr>::safe_grow_cleared (unsigned len MEM_STAT_DECL) { unsigned oldlen = length (); - size_t sz = sizeof (T) * (len - oldlen); - safe_grow (len PASS_MEM_STAT); - if (sz != 0) - memset (&(address ()[oldlen]), 0, sz); + gcc_checking_assert (oldlen <= len); + + if (size_t sz = sizeof (T) * (len - oldlen)) + { + safe_grow (len PASS_MEM_STAT); + + T *p = address (); + gcc_assert (p != NULL); + memset (p + oldlen, 0, sz); + } }