On 6/28/21 6:32 PM, Aldy Hernandez wrote:
I had changed my approach to passing -Wno-free-nonheap-object in the
Makefile. Can you try disabling the Makefile bit and bootstrapping,
cause that was still failing.
I see. I just tested it and my patch does let the existing #pragma
suppress the warning. I just pinged the core patch yesterday so
once it and the rest of the series gets approved there will be no
need for the Makefile workaround.
Martin
Thanks.
Aldy
On Tue, Jun 29, 2021, 00:29 Martin Sebor <mse...@gmail.com
<mailto:mse...@gmail.com>> wrote:
On 6/28/21 12:17 AM, Aldy Hernandez wrote:
>
>
> On 6/25/21 7:19 PM, Martin Sebor wrote:
>> On 6/25/21 10:20 AM, Aldy Hernandez via Gcc wrote:
>>> Hi folks.
>>>
>>> I'm done with benchmarking, testing and cleanups, so I'd like
to post
>>> my patchset for review. However, before doing so, I'd like to
>>> address a handful of meta-issues that may affect how I post these
>>> patches.
>>>
>>> Trapping on differences
>>> =======================
>>>
>>> Originally I wanted to contribute verification code that would
trap
>>> if the legacy code threaded any edges the new code couldn't (to be
>>> removed after a week). However, after having tested on various
>>> architectures and only running once into a missing thread, I'm
>>> leaning towards omitting the verification code, since it's
fragile,
>>> time consuming, and quite hacky.
>>>
>>> For the record, I have tested on x86-64, aarch64, ppc64 and
ppc64le.
>>> There is only one case, across bootstrap and regression tests
where
>>> the verification code is ever tripped (discussed below).
>>>
>>> Performance
>>> ===========
>>>
>>> I re-ran benchmarks as per our callgrind suite, and the penalty
with
>>> the current pipeline is 1.55% of overall compilation time. As is
>>> being discussed, we should be able to mitigate this
significantly by
>>> removing other threading passes.
>>>
>>> Failing testcases
>>> =================
>>>
>>> I have yet to run into incorrect code being generated, but I
have had
>>> to tweak a considerable number of tests. I have verified every
>>> single discrepancy and documented my changes in the testsuite
when it
>>> merited doing so. However, there are a couple tests that trigger
>>> regressions and I'd like to ask for guidance on how to address
them.
>>>
>>> 1. gcc.c-torture/compile/pr83510.c
>>>
>>> I would like to XFAIL this.
>>>
>>> What happens here is that thread1 threads a switch statement such
>>> that the various cases have been split into different independent
>>> blocks. One of these blocks exposes an arr[i_27] access which is
>>> later propagated by VRP to be arr[10]. This is an invalid access,
>>> but the array bounds code doesn't know it is an unreachable path.
>>
>> The test has a bunch of loops that iterate over the 10 array
elements.
>> There have been bug reports about loop unrolling causing false
positives
>> -Warray-bounds (e.g., PR 92539, 92110, or 86341) so this could be
>> the same issue.
>>
>>>
>>> However, it is not until dom2 that we "know" that the value of the
>>> switch index is such that the path to arr[10] is unreachable. For
>>> that matter, it is not until dom3 that we remove the
unreachable path.
>>
>> If you do XFAIL it can you please isolate a small test case and open
>> a bug and make it a -Warray-bounds blocker?
>
> Will do.
>
>>
>>>
>>> 2. -Wfree-nonheap-object
>>>
>>> This warning is triggered while cleaning up an auto_vec. I see
that
>>> the va_heap::release() inline is wrapped with a pragma ignore
>>> "-Wfree-nonheap-object", but this is not sufficient because jump
>>> threading may alter uses in such a way that
may_emit_free_warning()
>>> will warn on the *inlined* location, thus bypassing the pragma.
>>>
>>> I worked around this with a mere:
>>>
>>> > @@ -13839,6 +13839,7 @@ maybe_emit_free_warning (tree exp)
>>>> location_t loc = tree_inlined_location (exp);
>>>> + loc = EXPR_LOCATION (exp);
>>>
>>> but this causes a ton of Wfree-nonheap* tests to fail. I think
>>> someone more knowledgeable should address this (msebor??).
>>
>> This sounds like the same problem as PR 98871. Does the patch below
>> fix it?
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572515.html
>> If so, I suggest getting that patch in first to avoid testsuite
>> failures. If it doesn't fix it I'll look into it before you commit
>> your changes.
>
> The latest patch in the above thread does not apply. Do you have
a more
> recent patch?
The first patch in the series applies cleanly for me:
https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572516.html
but patch 2/4 does need a few adjustments. With those and your
latest changes applied I don't see any -Wfree-nonheap-object test
failures.
Martin
>
>>> 3. uninit-pred-9_b.c
>>>
>>> The uninit code is getting confused with the threading and the
bogus
>>> warning in line 24 is back. I looked at the thread, and it is
correct.
>>>
>>> I'm afraid all these warnings are quite fragile in the presence of
>>> more aggressive optimizations, and I suspect it will only get
worse.
>>
>> From my recent review of open -Wmaybe-uninitialized bugs (and
>> the code) it does seem to be both fragile and getting worse. I've
>> only found a few simple problems so far in the code but nothing that
>> would make a dramatic difference so I can't say if it's possible to
>> do much better, but I'm not done or ready to give up. If you XFAIL
>> this too please open a bug for it and make it a blocker for
>> -Wuninitialized?
>
> Will do.
>
> Good to know these are somewhat known issues.
>
> Thanks.
> Aldy
>
