[03/19] libctf: lowest-level memory allocation and debug-dumping wrappers

Message ID 20190430225706.159422-4-nick.alcock@oracle.com
State New
Headers show
Series
  • libctf, and CTF support for objdump and readelf
Related show

Commit Message

Nick Alcock April 30, 2019, 10:56 p.m.
Note for reviewers:

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.

(The debugging dumper is initialized via an ELF constructor in a different
TU, ctf-lib.c, because this ELF constructor also does other things, mostly
specific ctf-lib.c, which will be introduced in later commits, and we wanted
to keep the number of ELF constructors down.)

(I am amenable to replacing the environment-variable triggering of
ctf_dprintf() with something else in the binutils commit, and dropping
the other wrappers entirely, if you prefer.  If some other way of triggering
debugging trace output is preferred, or if the tracer should be replaced with
a do-nothing wrapper in binutils, that is fine too.)

libctf/
	* ctf-impl.h: New file.
	* ctf-lib.c: New file.
	* ctf-subr.c: New file.
---
 libctf/ctf-impl.h | 71 +++++++++++++++++++++++++++++++++++++++++++++
 libctf/ctf-lib.c  | 26 +++++++++++++++++
 libctf/ctf-subr.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 171 insertions(+)
 create mode 100644 libctf/ctf-impl.h
 create mode 100644 libctf/ctf-lib.c
 create mode 100644 libctf/ctf-subr.c

-- 
2.21.0.237.gd0cfaa883d

Comments

Nick Clifton May 2, 2019, 3:29 p.m. | #1
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
Nick Alcock May 3, 2019, 7:10 p.m. | #2
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.

Patch

diff --git a/libctf/ctf-impl.h b/libctf/ctf-impl.h
new file mode 100644
index 0000000000..108c89d2c5
--- /dev/null
+++ b/libctf/ctf-impl.h
@@ -0,0 +1,71 @@ 
+/* Implementation header.
+   Copyright (C) 2006-2019 Free Software Foundation, Inc.
+
+   This file is part of libctf.
+
+   libctf is free software; you can redistribute it and/or modify it under
+   the terms of the GNU General Public License as published by the Free
+   Software Foundation; either version 2, or (at your option) any later
+   version.
+
+   This program is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+   See the GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; see the file COPYING.  If not see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef	_CTF_IMPL_H
+#define	_CTF_IMPL_H
+
+#include "config.h"
+#include <sys/errno.h>
+#include <ctf-api.h>
+#include <sys/types.h>
+
+#ifdef	__cplusplus
+extern "C"
+  {
+#endif
+
+/* Compiler attributes.  */
+
+#if defined (__GNUC__)
+
+/* GCC.  We assume that all compilers claiming to be GCC support sufficiently
+   many GCC attributes that the code below works.  If some non-GCC compilers
+   masquerading as GCC in fact do not implement these attributes, version checks
+   may be required.  */
+
+/* We use the _libctf_*_ pattern to avoid clashes with any future attribute
+   macros glibc may introduce, which have names of the pattern
+   __attribute_blah__.  */
+
+#define _libctf_constructor_(x) __attribute__ ((__constructor__))
+#define _libctf_destructor_(x) __attribute__ ((__destructor__))
+#define _libctf_printflike_(string_index,first_to_check) \
+    __attribute__ ((__format__ (__printf__, (string_index), (first_to_check))))
+#define _libctf_unlikely_(x) __builtin_expect ((x), 0)
+#define _libctf_unused_ __attribute__ ((__unused__))
+
+#endif
+
+extern void *ctf_data_alloc (size_t);
+extern void ctf_data_free (void *, size_t);
+extern void ctf_data_protect (void *, size_t);
+
+extern void *ctf_alloc (size_t);
+extern void ctf_free (void *, size_t);
+
+_libctf_printflike_ (1, 2)
+extern void ctf_dprintf (const char *, ...);
+
+extern int _libctf_debug;	/* debugging messages enabled */
+
+#ifdef	__cplusplus
+}
+#endif
+
+#endif /* _CTF_IMPL_H */
diff --git a/libctf/ctf-lib.c b/libctf/ctf-lib.c
new file mode 100644
index 0000000000..1e81f3d20e
--- /dev/null
+++ b/libctf/ctf-lib.c
@@ -0,0 +1,26 @@ 
+/* Miscellary.
+   Copyright (C) 2003-2019 Free Software Foundation, Inc.
+
+   This file is part of libctf.
+
+   libctf is free software; you can redistribute it and/or modify it under
+   the terms of the GNU General Public License as published by the Free
+   Software Foundation; either version 2, or (at your option) any later
+   version.
+
+   This program is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+   See the GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; see the file COPYING.  If not see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <ctf-impl.h>
+
+_libctf_constructor_(_libctf_init)
+static void _libctf_init (void)
+{
+  _libctf_debug = getenv ("LIBCTF_DEBUG") != NULL;
+}
diff --git a/libctf/ctf-subr.c b/libctf/ctf-subr.c
new file mode 100644
index 0000000000..f358298b7d
--- /dev/null
+++ b/libctf/ctf-subr.c
@@ -0,0 +1,74 @@ 
+/* Simple subrs.
+   Copyright (C) 2003-2019 Free Software Foundation, Inc.
+
+   This file is part of libctf.
+
+   libctf is free software; you can redistribute it and/or modify it under
+   the terms of the GNU General Public License as published by the Free
+   Software Foundation; either version 2, or (at your option) any later
+   version.
+
+   This program is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+   See the GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; see the file COPYING.  If not see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <ctf-impl.h>
+#include <sys/mman.h>
+#include <stdarg.h>
+#include <string.h>
+
+void *
+ctf_data_alloc (size_t size)
+{
+  return (mmap (NULL, size, PROT_READ | PROT_WRITE,
+		MAP_PRIVATE | MAP_ANON, -1, 0));
+}
+
+void
+ctf_data_free (void *buf, size_t size)
+{
+  (void) munmap (buf, size);
+}
+
+void
+ctf_data_protect (void *buf, size_t size)
+{
+  (void) mprotect (buf, size, PROT_READ);
+}
+
+void *
+ctf_alloc (size_t size)
+{
+  return (malloc (size));
+}
+
+void
+ctf_free (void *buf, size_t size _libctf_unused_)
+{
+  free (buf);
+}
+
+const char *
+ctf_strerror (int err)
+{
+  return (const char *) (strerror (err));
+}
+
+_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);
+      va_end (alist);
+    }
+}