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.
Thanks. Aldy On Tue, Jun 29, 2021, 00:29 Martin Sebor <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 > > > >
