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

[Nick Alcock <nick dot alcock at oracle dot com>]

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.