From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id vW9ELGqGa2C6OAAAWB0awg (envelope-from ) for ; Mon, 05 Apr 2021 17:51:38 -0400 Received: by simark.ca (Postfix, from userid 112) id A6B8D1E789; Mon, 5 Apr 2021 17:51:38 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-0.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_DYNAMIC,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 122B51E01F for ; Mon, 5 Apr 2021 17:51:38 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C456E388C034; Mon, 5 Apr 2021 21:51:37 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C456E388C034 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1617659497; bh=XsXjJtg6k1spcjtCcdKVQk9kXCrCvQGEwmDD4N2huUo=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=nBgo5csNTnoq8Rdlf17JAJvBW9+Mp+0hR/MoIXzmWrV5kxEionkehamCGD0h2ZtL1 wI6rBXgKJgi0FdCa5R9ibls8DyVhNeyEDE+SmYseZjTf6cjmXuNIS3uKopAGnVJKic 9G6ENIXpsmokKRgl4a/kN2d3t0x6YfPkFYWaV/t0= Received: from smtp.gentoo.org (mail.gentoo.org [IPv6:2001:470:ea4a:1:5054:ff:fec7:86e4]) by sourceware.org (Postfix) with ESMTP id E557C3846457 for ; Mon, 5 Apr 2021 21:51:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E557C3846457 Received: from vapier (localhost [127.0.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.gentoo.org (Postfix) with ESMTPS id F0FE4335C76; Mon, 5 Apr 2021 21:51:33 +0000 (UTC) Date: Mon, 5 Apr 2021 17:51:33 -0400 To: Simon Marchi Subject: Re: [PATCH] sim/m32c: fix memory leaks in opc2c Message-ID: Mail-Followup-To: Simon Marchi , gdb-patches@sourceware.org References: <20210405145856.3925296-1-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mike Frysinger via Gdb-patches Reply-To: Mike Frysinger Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 05 Apr 2021 14:46, Simon Marchi via Gdb-patches wrote: > On 2021-04-05 12:23 p.m., Mike Frysinger wrote:> On 05 Apr 2021 10:58, Simon Marchi via Gdb-patches wrote: > >> Fix the leak in dump_lines by free-ing the elements of varnames. > > > > i dislike stuffing a bunch of free's at the end of a program's lifetime just > > to satisfy a tool that is not normally used. which isn't to say LSAN isn't > > useful, just that i think we should do better. > > Why though? Because it adds execution time where not necessary? when the process exits, the kernel releases all the memory at once. there's no point to calling free() on all your allocated buffers before exiting. it only wastes time with the C library heap accounting & syscalls. i get that we're talking about opc2c here which is used only twice to generate two other files, so in the larger scheme of things, it's barely noise. i'm trying to define standard patterns we can apply in general for "the next one". > > in other codebases, i've done things like: > > #ifdef __SANITIZE_ADDRESS__ > > # define ENABLE_CLEANUP_ONEXIT 1 > > #else > > # define ENABLE_CLEANUP_ONEXIT 0 > > #endif > > > > then this could be written: > > > > if (ENABLE_CLEANUP_ONEXIT) { > > for (i = 0; i < vn; i++) > > free (varnames[i]); > > } > > > > wdyt ? feel free to bikeshed the name. not sure if there's prior art in > > the tree currently. > > I find this complexity a bit unnecessary, versus just free-ing the > stuff. But I don't really mind, we can do as you like, I just want to > my build to be fixed! > > Note that the igen tool doesn't free anything, so it's next on the list > of things that break the -fsanizite=address build. I started to update > it to free things, it's a bit tedious but it should be do-able. > > Another option would be to change the Makefile to call igen with the > ASAN_OPTIONS=detect_leaks=0 environment variable. ah yes, this is exactly what i mean wrt "the tip of the iceberg" and it being "a slippery slope" ;). first it's the small build tools, then the larger build tools, then the programs we actually install, ... maybe an alternative would be to convert these to C++. then it would handle stack/heap resources for us. -mike