From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16664 invoked by alias); 2 May 2019 15:29:48 -0000 Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org Received: (qmail 16655 invoked by uid 89); 2 May 2019 15:29:48 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 May 2019 15:29:47 +0000 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3416B81E1B; Thu, 2 May 2019 15:29:46 +0000 (UTC) Received: from [10.36.116.145] (ovpn-116-145.ams2.redhat.com [10.36.116.145]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7A59757DDA; Thu, 2 May 2019 15:29:45 +0000 (UTC) To: Nick Alcock , binutils@sourceware.org References: <20190430225706.159422-1-nick.alcock@oracle.com> <20190430225706.159422-4-nick.alcock@oracle.com> From: Nick Clifton Openpgp: preference=signencrypt Subject: Re: [PATCH 03/19] libctf: lowest-level memory allocation and debug-dumping wrappers Message-ID: <4cda9c6e-c285-6b97-614f-4d7aaf3a036f@redhat.com> Date: Thu, 02 May 2019 15:29:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190430225706.159422-4-nick.alcock@oracle.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg00102.txt.bz2 Hi Nick, > I could easily be convinced that all of these wrappers serve no purpose > and should be globally removed, but the debugging tracer is frequently > useful, and the malloc/free/mmap/munmap wrappers have proved mildly > useful in conjunction with symbol interposition for allocation debugging > in the (relatively distant) past. I see no problem with retaining these wrappers. They are not going to inflict a large overhead on the library, and if they have proved helpful in debugging then they have proved their worth. Besides having functions like these allows them to be intercepted by third parties, if for some reason that becomes necessary. > (I am amenable to replacing the environment-variable triggering of > ctf_dprintf() with something else in the binutils commit, I am not a fan of using environment variables, although in this particular case it is not so bad. There is of course the problem of documentation - you must tell the users about the variable - and also the fact that users of the library might wish to enable/disable debugging themselves. Perhaps the library could provide a function to set the value of _libctf_debug even after _libctf_init() has been called ? > +/* Miscellary. > + Copyright (C) 2003-2019 Free Software Foundation, Inc. > + Ooo - just noticed - this header, and others too, do not mention who contributed the code. It would be nice to see your efforts acknowledged don't you think ? > +void * > +ctf_data_alloc (size_t size) > +{ > + return (mmap (NULL, size, PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANON, -1, 0)); > +} I am not a memory allocation expert - but is it efficient to call mmap for every memory allocation ? Or do higher levels of libctf manage the allocated memory so that this function is only used when needed to grab large chunks of memory ? > +void > +ctf_data_free (void *buf, size_t size) > +{ > + (void) munmap (buf, size); > +} Why do you ignore and discard the return value from munmap() ? If there is an error, surely you would want to examine it or return it to the caller ? > +void > +ctf_free (void *buf, size_t size _libctf_unused_) > +{ > + free (buf); > +} Why does your free function have a size parameter at all ? > +_libctf_printflike_ (1, 2) > +void ctf_dprintf (const char *format, ...) > +{ > + if (_libctf_debug) > + { > + va_list alist; > + > + va_start (alist, format); > + (void) fputs ("libctf DEBUG: ", stderr); > + (void) vfprintf (stderr, format, alist); I would recommend calling "fflush (stdout)" before starting to print to stderr, just in order to make sure that the debug output does not appear halfway through a line of ordinary output. Cheers Nick