Re: proposed new module careadlinkat (renamed from breadlinkat)

[Paul Eggert]

Thanks for your careful review, Ben.

On 04/01/2011 09:35 AM, Ben Pfaff wrote:

> It might be a good idea to mark each function with appropriate
> GCC attributes, e.g. __attribute__((malloc)) for malloc and
> realloc and __attribute__((noreturn)) for die.

I tried that, and it doesn't work, at least not with GCC 4.6.0.  GCC
warns that the attributes are ignored when they're on function types
within pointer types.  I don't know why that is.

Here are some examples of the problem.  (I tried all the places I
could think of, with that somewhat-squirrelly syntax.)

   $ cat t.c
   #include <stddef.h>
   void *(*f) (size_t) __attribute__((malloc));
   void __attribute__ ((malloc)) *(*f) (size_t);
   void *__attribute__ ((malloc)) (*g) (size_t);
   void *(__attribute__ ((malloc)) *h) (size_t);
   void *(*__attribute__ ((malloc)) i) (size_t);
   void *(*j) (size_t) __attribute__((malloc));
   $ gcc -Wall -S t.c
   t.c:2:1: warning: 'malloc' attribute ignored [-Wattributes]
   t.c:3:1: warning: 'malloc' attribute ignored [-Wattributes]
   t.c:4:1: warning: 'malloc' attribute ignored [-Wattributes]
   t.c:5:1: warning: 'malloc' attribute does not apply to types [-Wattributes]
   t.c:6:1: warning: 'malloc' attribute ignored [-Wattributes]
   t.c:7:1: warning: 'malloc' attribute ignored [-Wattributes]

> I was a little surprised to see the first proposed use of this
> actually copy out all of the pointers into local variables.  If
> that's the way it's going to be used, I guess that would make
> the attributes useless, unless they were applied to the local
> variables too.

Yes, that's right, but they don't work on local variables
either, as far as I know.

> I'm not sure why die is a separate member.  Couldn't xmalloc and
> xrealloc simply be used as the malloc and realloc functions?

No, because sometimes functions detect that there is memory
overflow while calculating sizes, before those sizes are
passed to malloc and realloc.  In that case we need a
separate 'die' function.  This happens in careadlinkat,
if a symbolic link contains SIZE_MAX or more bytes.
Admittedly this is just a theoretical possibility here,
but it'll be a practical problem in some other applications.

> I would have guessed that there would be a global instance of
> this allocator, something like:
>
> struct allocator standard_allocator = { malloc, realloc, free, NULL };

I tried that at first, but it was more awkward for the common case
where the standard functions are being used, because users of
careadlinkat had to say "&standard_allocator" and they had to
#include <allocator.h>.  Under the proposal they just say "NULL" and
don't need to include <allocator.h>.  It's a minor point and I could
easily be persuaded to switch back.


(I suppose I should add some comments to the code explaining all
the above....)