From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61901 invoked by alias); 15 May 2015 22:35:14 -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 61889 invoked by uid 89); 15 May 2015 22:35:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail-oi0-f45.google.com Received: from mail-oi0-f45.google.com (HELO mail-oi0-f45.google.com) (209.85.218.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 15 May 2015 22:35:12 +0000 Received: by oica37 with SMTP id a37so93340612oic.0 for ; Fri, 15 May 2015 15:35:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=CXjhSVPPPx/d3dEfE+aKcJaeSM0AKmfMNcUBGG8pVyg=; b=HZ7Wk+xu6LzTeZdTl5Vx7qWbfZlB+ELBRvHYoHclg6RwSCGFNSa1XNcAy9944OttpK bgmptHOgzYicMdt6hdA/AbdpGqtDWj/HIVDAZAOvc8bMampEDi7vUwOAi0veWvPcW7MZ pUDD2CJuf9WHxTUN/O9uILLw7zbmxVa4jgdENWAutVLlvm5SNv5KsXvI2xz856TuupWy +cLmDnSVg7iwbU0dS5qkb60imD3SHRLCin8tCPHb7AwgEDJTBv2FfwGSSnCpNH98/qUT yfECt6fzCTFVH0BnT4AVIcVJ/RggUBwHnXB7au8xzwelGPSF6wxcKNaC3qEsJsKJ2yg/ AjEw== X-Gm-Message-State: ALoCoQmUcnBG/1WCa4juPOSbhMfAuqu9khY84HqkvD9D8tCKoZo/APRDYYKsd6L5mHXj0Ub3juxt MIME-Version: 1.0 X-Received: by 10.202.217.196 with SMTP id q187mr9869672oig.64.1431729310971; Fri, 15 May 2015 15:35:10 -0700 (PDT) Received: by 10.182.89.99 with HTTP; Fri, 15 May 2015 15:35:10 -0700 (PDT) In-Reply-To: <20150515155823.GL4767@adacore.com> References: <1431100524-7793-1-git-send-email-brobecker@adacore.com> <55508A83.3060605@redhat.com> <20150511205312.GE4767@adacore.com> <5551CB20.4090104@redhat.com> <20150515155823.GL4767@adacore.com> Date: Fri, 15 May 2015 22:35:00 -0000 Message-ID: Subject: Re: [RFA/commit] Memory leak in on reading frame register From: Doug Evans To: Joel Brobecker Cc: Pedro Alves , gdb-patches , Jerome Guitton Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg00420.txt.bz2 On Fri, May 15, 2015 at 8:58 AM, Joel Brobecker wrote: >> >> Not sure about this. >> >> >> >> How come this in bpstat_check_breakpoint_conditions didn't >> >> handle this issue already? : >> >> >> >> ... >> >> /* We use value_mark and value_free_to_mark because it could >> >> be a long time before we return to the command level and >> >> call free_all_values. We can't call free_all_values >> >> because we might be in the middle of evaluating a >> >> function call. */ >> >> struct value *mark = value_mark (); >> >> >> >> ... >> >> value_free_to_mark (mark); >> > >> > An excellent question, which I will try to research in the next >> > couple of days! >> >> Thanks. I wonder whether the leaks come from constructing the >> current frame at each stop, instead of from evaluating >> breakpoint conditions. E.g.., if we do a "step" over: >> >> while (1); >> >> ... are we constantly leaking values until the user does >> ctrl-c? >> >> That would suggest to me to that we should be doing >> value_mark/value_free_to_mark around each >> handle_inferior_event. > > A very accurate guess, as it turns out. Condition evaluation > is not the problem, here, but indeed, we a couple of calls to > handle_inferior in addition to each call to > bpstat_check_breakpoint_conditions. The former are responsible > for the leak. > > How about the attached patch? > > gdb/ChangeLog: > > * infrun.c (handle_inferior_event_1): Renames handle_inferior_event. > (handle_inferior_event): New function. > > Tested on x86_64-linux. No regression. Not that this has to be changed here, but I'm wondering why all value mark/frees aren't done via cleanups. I can imagine sometimes it's not, technically, necessary, and I can imagine there's some history/inertia here, but having two ways to do this (using a cleanup or not) leaves the reader having to wonder if using a cleanup was errantly skipped.