From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 43958 invoked by alias); 3 May 2019 19:12:25 -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 43950 invoked by uid 89); 3 May 2019 19:12:25 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.1 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 spammy=earth, wasting, HX-Proofpoint-Spam-Details:main-1905030125 X-HELO: aserp2130.oracle.com Received: from aserp2130.oracle.com (HELO aserp2130.oracle.com) (141.146.126.79) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 03 May 2019 19:12:23 +0000 Received: from pps.filterd (aserp2130.oracle.com [127.0.0.1]) by aserp2130.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x43J9o1s061477; Fri, 3 May 2019 19:12:07 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=from : to : cc : subject : references : date : in-reply-to : message-id : mime-version : content-type; s=corp-2018-07-02; bh=7ZMo0GOZcsyKhi1rLYIhYSoHaN/5oqJz1nNkTPESmtQ=; b=XUEYAc5yjh0MhLGvY0NnqPjIxF0mRXFWzJcIoPOL6LPHGF9hMs1kO/b8mzHYDBZmuCma Qtqo9H+U1rT/zte5VcYUg626TipBf8WUkhQAl2dwo3tfm0SriiHmqmEuVYesIGWoxzs0 BR9GjnmPpVUJIe6XdsbVQ7No32eWHAbfoRuqJ98AmYq6hDQTqVPT/aSWHDEu38btL1vu mG2uyTgz871Q9dswTuVK8BEV1OErwGi8JjFCIN4KzvEJHk8g6UA9evp2EoJrO24JArpe tsB140DEqvswEIS3FpY5BVKQDMYA7jUhAMirMRhr4giONz5k3aYvV34gVxkgsuqTlXzH sg== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by aserp2130.oracle.com with ESMTP id 2s6xhyrqgk-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 03 May 2019 19:12:07 +0000 Received: from pps.filterd (userp3020.oracle.com [127.0.0.1]) by userp3020.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x43J9FrW053465; Fri, 3 May 2019 19:10:06 GMT Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by userp3020.oracle.com with ESMTP id 2s6xhhsrgs-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 03 May 2019 19:10:06 +0000 Received: from abhmp0011.oracle.com (abhmp0011.oracle.com [141.146.116.17]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id x43JA5ep008600; Fri, 3 May 2019 19:10:05 GMT Received: from loom (/81.187.191.129) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Fri, 03 May 2019 12:10:04 -0700 From: Nick Alcock To: Nick Clifton Cc: binutils@sourceware.org Subject: Re: [PATCH 03/19] libctf: lowest-level memory allocation and debug-dumping wrappers References: <20190430225706.159422-1-nick.alcock@oracle.com> <20190430225706.159422-4-nick.alcock@oracle.com> <4cda9c6e-c285-6b97-614f-4d7aaf3a036f@redhat.com> Date: Fri, 03 May 2019 19:12:00 -0000 In-Reply-To: <4cda9c6e-c285-6b97-614f-4d7aaf3a036f@redhat.com> (Nick Clifton's message of "Thu, 2 May 2019 16:29:44 +0100") Message-ID: <87woj7uzg4.fsf@esperi.org.uk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg00117.txt.bz2 On 2 May 2019, Nick Clifton spake thusly: > Hi Nick, >> (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 ? That seems like an excellent idea and I have no idea why it didn't already exist. That way the callers can look up the env var and then we don't need to bother with the ELF constructor, and one more compatibility headache for non-ELF platforms is gone. :) ... added ctf_setdebug() and ctf_getdebug(). (For compatibility with existing callers, we set the default value from the environment variable at the obvious open/create entry points, if ctf_setdebug() has not already 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 ? I was asked not to, and I thought it was current policy not to put that sort of thing in (IIRC that is current policy in GCC, but I could be wrong since this in old and crusty merely human memory). I'm quite happy to put it in otherwise :) >> +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 ? This is only used for a few large allocations of the entire CTF buffer at once: during compression and decompression, when endian-flipping, and during transparent upgrading (which you can't see yet because I unifdeffed it away, but it will return in v2 of the patch series). The only reason for this is that it lets us mprotect() it afterward using ctf_data_protect(), since after creation CTF files are always read-only so all modifications are certain to be wild pointers. Whether this particular piece of paranoia is justified, I'm not entirely sure: it might be worth it only for larger CTF sections. (In that case, we could easily arrange for ctf_data_alloc()/free() to mmap() only when the size is much larger than one page, and malloc()/free() otherwise, and for ctf_data_protect() to do nothing unless called for things above that threshold.) That should give us roughly the same degree of paranoia (since wild pointers strike roughly at random, they'll probaly hit bigger CTF files with more area to hit much more than smaller ones) while avoiding wasting memory on smaller CTF files. (Implemented that for v2, with an arbitrary limit set to one page. If you think something else is better, let me know!) >> +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 ? Mostly because I couldn't think of anything to do if there is an error. It's about as hard to deal with as an error from free(). If there is an error there has been a disaster and I can't figure out any useful recovery method, but this is a library so we surely shouldn't crash either. I could certainly emit a ctf_dprintf() or output on stderr (but that's unfriendly for a library, too). I suppose we could return it to callers, most of which will also ignore it because there is nothing they can do... :) >> +void >> +ctf_free (void *buf, size_t size _libctf_unused_) >> +{ >> + free (buf); >> +} > > Why does your free function have a size parameter at all ? I think there was an old debugging tool that needed the size at freeing time. But the size *is* annoying to keep track of as far as the free and adds pointless complexity for literally no benefit: if you don't want it either, that's it, it has passed my threshold of annoyance and it's gone. I've also marked ctf_free(), ctf_data_alloc(), ctf_mmap() and ctf_strdup() as __attribute__((__malloc__)): all four allocate new storage which cannot contain pointers to any existing storage, so they are eligible and marking them makes them better proxies for malloc(). >> +_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. ... why on earth didn't I think of that?! I've been seeing that failure now and then for ages. Thank you!!! Fixed.