From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21841 invoked by alias); 17 Aug 2017 15:27:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 21817 invoked by uid 89); 17 Aug 2017 15:27:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=reclaimed, H*r:sk:static., central X-HELO: mail-qk0-f180.google.com Received: from mail-qk0-f180.google.com (HELO mail-qk0-f180.google.com) (209.85.220.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 17 Aug 2017 15:27:13 +0000 Received: by mail-qk0-f180.google.com with SMTP id u139so38494423qka.1 for ; Thu, 17 Aug 2017 08:27:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=fYo8hsDbo2zw05/lh/E7Hh0JHEtLd+0YCdZjAqLDaNI=; b=W9YB3CuV4xkGgxLCg8cmwhApmrKmvn8lQhVKpKISL4nDu9mjOYuiVyzmcyxIHA8Z3/ Lj3/GCbh/29wwGskV1eQvApxy+8HYHs0qXI0Nbw9wzvR6tH6I78ECYra1I+r7jlq6qKE FvLvPzA22pJysLQgqgC/fHIpXenybf4Cn6MG0wfrOO/V1bPMGBw4O80I/BSY5Ruzoh9o qSaXHCd0G4E3m3T2OouWEbb3bC1IQwgMu4viCtPsCeodSBk/scEVQJci7WoEHWc6073n T+raiBtLkWyLvgkZK2dWMk2blJvzoSb2fMTIjz3dQILEGejG3cst3JOKqmfJVD9iaS9L CtZQ== X-Gm-Message-State: AHYfb5hYBbS1Od/LusAMQ5LrTVOyCzr12sR/8kQ0LOL4a4q2rzJRihAJ kHe7ksgFnN30Xdel X-Received: by 10.55.189.132 with SMTP id n126mr7857846qkf.89.1502983632099; Thu, 17 Aug 2017 08:27:12 -0700 (PDT) Received: from E107787-LIN (static.42.136.251.148.clients.your-server.de. [148.251.136.42]) by smtp.gmail.com with ESMTPSA id f55sm2481546qtc.18.2017.08.17.08.27.10 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 17 Aug 2017 08:27:11 -0700 (PDT) From: Yao Qi To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 4/9] Reset *THIS_CACHE in frame_unwind_try_unwinder in case of exception References: <1501539715-8049-1-git-send-email-yao.qi@linaro.org> <1501539715-8049-5-git-send-email-yao.qi@linaro.org> <742591a2-7d2e-f1fb-6cbf-174a27ed02ff@redhat.com> Date: Thu, 17 Aug 2017 15:27:00 -0000 In-Reply-To: <742591a2-7d2e-f1fb-6cbf-174a27ed02ff@redhat.com> (Pedro Alves's message of "Fri, 11 Aug 2017 17:47:16 +0100") Message-ID: <868tiimczd.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg00348.txt.bz2 Pedro Alves writes: > If we always do this on error, then why not do it in: > > static void > frame_cleanup_after_sniffer (void *arg) > { > struct frame_info *frame =3D (struct frame_info *) arg; > > /* The sniffer should not allocate a prologue cache if it did not > match this frame. */ > gdb_assert (frame->prologue_cache =3D=3D NULL); > > with the comment adjusted? I.e., replace the assertion with > frame->prologue_cache =3D NULL; Each frame sniffer still has to de-allocate frame->prologue_cache if the unwinder isn't applicable. My patch touches the patch on error/exception, and the assert is to check each unwinder's sniffer has to release frame->prologue_cache on no-error return. > > Still, there's something in the whole try-unwind mechanism that > isn't handled 100% nicely, that makes me wonder whether this > central this_cache clearing here is the right approach. > > Ant that is the fact that we clear *this_cache, but the dwarf2_frame_cache > object is left in the frame chain obstack. Sure, it won't usually We don't allocate dwarf2_frame_cache in dwarf2_frame_sniffer, so dwarf2_frame_cache object won't be left in obstack. However, - dwarf2_frame_cache created in dwarf2_frame_{unwinder_stop_reason, this_id, prev_register} may be left in obstack in case of exception. - other unwinders' sniffer may allocate frame cache, and its frame cache may be left on obstack in case of exception. > be a problem, the memory will be reclaimed at some point later, but > it still feels like a design hole. It looks to me like we should > just wipe everything at was added to the obstack if the unwinder Such design hole exists for several years. If we want to fix this design hole, as you did, we should somehow call obstack_free. On the contrary, this shows the benefit of this patch series, that is, we centralize the exception handling, so we can call obstack_free when exception is thrown in unwinder apis (see patch #5). Otherwise (we catch exceptions in each unwinder) we have to call obstack_free in each unwinder. > fails. I gave it a try, see prototype patch below, to see if something > would break. It didn't -- the patch passes regtesting on x86-64, > at least. I made get_frame_cache a template since all unwinders > would do exactly the same. > > A simpler, though maybe not as nice approach could > be to call obstack_free in frame_cleanup_after_sniffer instead: > > if (frame->prologue_cache !=3D NULL) > { > // assume=20 > obstack_free (&frame_cache_obstack, frame->prologue_cache); > frame->prologue_cache =3D NULL; > } Your patch get_frame_cache can guarantee the obstack space can be released when an exception is thrown during initialization. We need it for every unwinder. The simpler approach above looks better, because we can do it after every unwinder apis call. --=20 Yao (=E9=BD=90=E5=B0=A7)