Everything

php

[PATCH] __attribute_malloc__ for malloc-like functions

php.internals

Ard Biesheuvel

23 years ago

Hello group,

After having spent some time digging into the origin of the
strict-aliasing warnings when compiling with GCC 3.3 (brought about by
Stefan Roehrich's post a couple of days ago), I came up with a couple of
things that might be of interest in the development of PHP.

First of all, GCC provides an function qualifier __attribute_malloc__,
which tells the compiler that the function in question is a malloc-like
function, and will therefore never return values that might be aliased
by other pointers in the scope of the function that called it. This is
quite a boost for the effectiveness of the optimizations, because - the
return type of malloc() being void* - the compiler has to take into
account that every pointer in scope might be an alias of the resulting
value. __attribute_malloc__ fixes that. (Disabled for non-GCC compilers)

The other thing is, that a lot of warnings are being triggered by
invocations of zend_hash_find(). This function stores its result in the
location pointed to by a void** argument, and returns an int specifying
whether the key was found or not. As the comment in the source states,
this was a conscious decision, because hashes can also be used to store
null pointers.

After looking through some of the code, I found out that in a lot of
cases, the pointers are expected not to be zero, and are being
dereferenced in the code that immediately follows it [without checking
for validity first].

Wouldn't it be more straight-forward to introduce an analogue for
zend_hash_find() (eg. 'void *zend_hash_get()') which returns the stored
pointer [or NULL on failure]. It could be used for applications where
null pointers aren't allowed, and fix current code, where the implicit
assumption is made the SUCCESS means the the pointers is non-null.

Last of all, I would like to know if we care about the warnings.
Personally I don't really mind, as GCC's capabilities of accurately
identifying violations of strict-aliasing rules are limited anyway.

Ard

Zeev Suraski

23 years ago

At 23:43 27/08/2003, Ard Biesheuvel wrote:

>The other thing is, that a lot of warnings are being triggered by
>invocations of zend_hash_find(). This function stores its result in the
>location pointed to by a void** argument, and returns an int specifying
>whether the key was found or not. As the comment in the source states,
>this was a conscious decision, because hashes can also be used to store
>null pointers.
>
>After looking through some of the code, I found out that in a lot of
>cases, the pointers are expected not to be zero, and are being
>dereferenced in the code that immediately follows it [without checking for
>validity first].
>
>Wouldn't it be more straight-forward to introduce an analogue for
>zend_hash_find() (eg. 'void *zend_hash_get()') which returns the stored
>pointer [or NULL on failure]. It could be used for applications where null
>pointers aren't allowed, and fix current code, where the implicit
>assumption is made the SUCCESS means the the pointers is non-null.
>
>Last of all, I would like to know if we care about the warnings.
>Personally I don't really mind, as GCC's capabilities of accurately
>identifying violations of strict-aliasing rules are limited anyway.

I'd just make gcc shut up about it.  Using void ** is the generic way to do
what we want to do and I see no reason to introduce another way that works
only in a subset of the cases, and will require tons of code changes.  I
have to admit I still haven't been able to figure out what gcc thinks is
wrong with casting a zval *** pointer to a void **, considering that any
pointer can be cast to a void *, and that pointers always have the same
size (i.e., any pointer is castable to void *).

Not sure about __attribute_malloc__ - it sounds reasonable but I have no
experience with it.  Were you able to measure any performance difference?

Zeev

Ard Biesheuvel

23 years ago

> I'd just make gcc shut up about it.  Using void ** is the generic way to
> do what we want to do and I see no reason to introduce another way that
> works only in a subset of the cases, and will require tons of code
> changes.  I have to admit I still haven't been able to figure out what
> gcc thinks is wrong with casting a zval *** pointer to a void **,
> considering that any pointer can be cast to a void *, and that pointers
> always have the same size (i.e., any pointer is castable to void *).

This is not about size, but about aliasing.
When compiling zend_hash.c, the compiler didn't expect void **pData to
really be a zval**, so any zval**s inside zend_hash_find() are assumed
not to alias with pData. If you cast a typed pointer function argument
to a different-typed pointer, GCC just warns you that strict-aliasing
rules are violated.

I've fixed it in interbase.c by casting to void* instead of void**. This
fix is good enough for me. It would be nice, though, to have as few
meaningless warnings as possible, so the real issues stand out.

The other issue I was pointing out is that some pieces of code blindly
dereference pointers returned by zend_hash_find(), while the comment
states that a result of SUCCESS doesn't imply that the pointer is
non-null. Instead of checking for SUCCESS, the code should really check
the validity of the pointer. I was merely suggesting a function that
returns the pointer and forgets about the result. Not that important though.

> Not sure about __attribute_malloc__ - it sounds reasonable but I have no
> experience with it.  Were you able to measure any performance difference?

I'll have to come up with a good malloc()-heavy test case.
Maybe you have a suggestion ?

I expect the effect to be significant, though, because (as I pointed
out) the void* return-type of [e]malloc() causes it to alias with every
pointer variable in scope, which severely limits the applicability of
certain GCC optimizations.

Ard

Sascha Schumann

23 years ago

> I expect the effect to be significant, though, because (as I pointed
> out) the void* return-type of [e]malloc() causes it to alias with every
> pointer variable in scope, which severely limits the applicability of
> certain GCC optimizations.

attribute((malloc)) is now enabled in CVS.  Note that
erealloc was excluded.

http://news.php.net/article.php?group=php.zend-engine.cvs&article=1711
http://news.php.net/article.php?group=php.zend-engine.cvs&article=1712

- Sascha

Ard Biesheuvel

23 years ago

>     attribute((malloc)) is now enabled in CVS.  Note that
>     erealloc was excluded.

I used __attribute_malloc__ intentionally, because it is also supported
by GCC 2.

Ard

Sascha Schumann

23 years ago

On Thu, 28 Aug 2003, Ard Biesheuvel wrote:

> >     attribute((malloc)) is now enabled in CVS.  Note that
> >     erealloc was excluded.
>
> I used __attribute_malloc__ intentionally, because it is also supported
> by GCC 2.

Nope, it does not.

$ cat m.c
void *p(void) __attribute_malloc__;
$ gcc -c m.c
m.c:1: parse error before `__attribute_malloc__'
m.c:1: warning: data definition has no type or storage class
$ gcc -v
Reading specs from /usr/lib/gcc-lib/i686-pc-linux-gnu/2.95.3/specs
gcc version 2.95.3 20010315 (release)

The same happens with the latest RedHat GCC 2.96-113.

- Sascha

Ard Biesheuvel

23 years ago

>
>     Nope, it does not.
>

You are right. Taken from <sys/cdefs.h>:

#if __GNUC_PREREQ (2,96)
# define __attribute_malloc__ __attribute__ ((__malloc__))
#else
# define __attribute_malloc__ /* Ignore */
#endif

So if you include any system library, GCC will silently ignore the
__attribute_malloc__, which is why it worked for me when I tested
compiling anything from the PHP tree with gcc-2.95.

This should be the test to include in zend_alloc.h

Ard

Ard Biesheuvel

23 years ago

>
>     Nope, it does not.
>

You are right. Taken from <sys/cdefs.h>:

#if __GNUC_PREREQ (2,96)
# define __attribute_malloc__ __attribute__ ((__malloc__))
#else
# define __attribute_malloc__ /* Ignore */
#endif

So if you include any system library, GCC will silently ignore the
__attribute_malloc__, which is why it worked for me when I tested
compiling anything from the PHP tree with gcc-2.95.

This should be the test to include in zend_alloc.h

Ard

Sascha Schumann

23 years ago

> So if you include any system library, GCC will silently ignore the
> __attribute_malloc__, which is why it worked for me when I tested
> compiling anything from the PHP tree with gcc-2.95.

Unfortunately, that is a Linux feature.  E.g. FreeBSD 4 has a
sys/cdefs.h, but does not define anything relevant.

> This should be the test to include in zend_alloc.h

A check for 2.96 has been committed a while ago.  What do you
think about adding the malloc attribute to functions which
either return NULL or an emalloc'ed pointer?

I've also added a few other attributes in a couple of places.
What do you think about the nonnull attribute?

- Sascha

Ard Biesheuvel

23 years ago

>     A check for 2.96 has been committed a while ago.  What do you
>     think about adding the malloc attribute to functions which
>     either return NULL or an emalloc'ed pointer?

If the return type is char* or void*, adding the malloc attribute would
be appropriate. Aliasing isn't an issue for null pointers, because they
can't be dereferenced anyway.

>     I've also added a few other attributes in a couple of places.
>     What do you think about the nonnull attribute?

This is merely a compile-time check, which is unnecessary if you don't
initialize pointers to NULL if NULL is an inapproriate value. In that
case (with -Wuninitialized enabled) GCC will give a warning that the
variable is not initialized.

I am not sure how nonnull is used in optimizations, though.

Ard

Ard Biesheuvel

23 years ago

>     I've also added a few other attributes in a couple of places.
>     What do you think about the nonnull attribute?

I do think the attributes ((pure)) and ((const)) could make a
difference. (For instance, hashing functions or any other function that
base their result solely on their arguments)

Ard

Rasmus Lerdorf

23 years ago

What were you saying about non-portable compiler tricks the other day?

10:15am riddler:~/php43> make
/bin/sh /home/rasmus/php43/libtool --silent --preserve-dup-deps --mode=compile gcc  -Iext/zlib/ -I/home/rasmus/php43/ext/zlib/
-DPHP_ATOM_INC -I/home/rasmus/php43/include -I/home/rasmus/php43/main -I/home/rasmus/php43 -I/home/rasmus/php43/Zend -I/usr/local/include
-I/usr/local/include/freetype2  -I/home/rasmus/php43/TSRM  -DSKIP_PATH_CHECKS -g -O2 -Wall  -prefer-non-pic -c
/home/rasmus/php43/ext/zlib/zlib.c -o ext/zlib/zlib.lo
In file included from /home/rasmus/php43/main/php.h:34,
from /home/rasmus/php43/ext/zlib/zlib.c:28:
/home/rasmus/php43/Zend/zend.h:305: argument format specified for non-function `error_function'
/home/rasmus/php43/Zend/zend.h:306: argument format specified for non-function `printf_function'
/home/rasmus/php43/Zend/zend.h:438: argument format specified for non-function `zend_printf'
/home/rasmus/php43/Zend/zend.h:445: argument format specified for non-function `zend_error_cb'
/home/rasmus/php43/ext/zlib/zlib.c: In function `zif_gzcompress':
/home/rasmus/php43/ext/zlib/zlib.c:440: warning: implicit declaration of function `compress2'
/home/rasmus/php43/ext/zlib/zlib.c:451: warning: implicit declaration of function `zError'
/home/rasmus/php43/ext/zlib/zlib.c:451: warning: format argument is not a pointer (arg 4)
/home/rasmus/php43/ext/zlib/zlib.c: In function `zif_gzuncompress':
/home/rasmus/php43/ext/zlib/zlib.c:508: warning: format argument is not a pointer (arg 4)
/home/rasmus/php43/ext/zlib/zlib.c: In function `zif_gzdeflate':
/home/rasmus/php43/ext/zlib/zlib.c:578: warning: format argument is not a pointer (arg 4)
/home/rasmus/php43/ext/zlib/zlib.c: In function `zif_gzinflate':
/home/rasmus/php43/ext/zlib/zlib.c:660: warning: format argument is not a pointer (arg 4)
/home/rasmus/php43/ext/zlib/zlib.c: In function `zif_gzencode':
/home/rasmus/php43/ext/zlib/zlib.c:821: warning: int format, long int arg (arg 4)
/home/rasmus/php43/ext/zlib/zlib.c:858: warning: format argument is not a pointer (arg 4)
/home/rasmus/php43/ext/zlib/zlib.c:865: warning: format argument is not a pointer (arg 4)
/home/rasmus/php43/ext/zlib/zlib.c:903: warning: format argument is not a pointer (arg 4)
*** Error code 1

Stop in /home/rasmus/php43.
10:15am riddler:~/php43> gcc --version
2.95.3

Sascha Schumann

23 years ago

On Thu, 28 Aug 2003, Ard Biesheuvel wrote:

> >     attribute((malloc)) is now enabled in CVS.  Note that
> >     erealloc was excluded.
>
> I used __attribute_malloc__ intentionally, because it is also supported
> by GCC 2.

A helpful comment from ansidecl.h:

Attribute __malloc__ on functions was valid as of gcc 2.96.

I've confirmed that __attribute__((malloc)) is accepted
whereas __attribute_malloc__ is not.  The inconsistency does
not surprise me considering the overall quality of GCC 2.96.
Anyway, I'll refine the version check later.

- Sascha

Sascha Schumann

23 years ago

> account that every pointer in scope might be an alias of the resulting
> value. __attribute_malloc__ fixes that. (Disabled for non-GCC compilers)

What about erealloc?  It can return the same pointer as
passed to it.

> After looking through some of the code, I found out that in a lot of
> cases, the pointers are expected not to be zero, and are being
> dereferenced in the code that immediately follows it [without checking
> for validity first].

Segfaulting early is a good thing.  Whenever a crash occurs a
defect has been found and needs to be fixed.  If the NULL would
just be silently ignored, the real bug (where NULL is
erroneously inserted) could slip by and make it into a
release.

> Wouldn't it be more straight-forward to introduce an analogue for
> zend_hash_find() (eg. 'void *zend_hash_get()') which returns the stored
> pointer [or NULL on failure]. It could be used for applications where
> null pointers aren't allowed, and fix current code, where the implicit
> assumption is made the SUCCESS means the the pointers is non-null.

Then you would also need a _set function which rejects NULL
values.  Considering that the existing APIs have worked quite
well in the past, I don't see a need for adding such a class
of APIs.

> Last of all, I would like to know if we care about the warnings.
> Personally I don't really mind, as GCC's capabilities of accurately
> identifying violations of strict-aliasing rules are limited anyway.

I think we should address them.  Many warnings provide
information about potential defects, but the flood of aliasing
warnings deters developers from considering the relevant
ones.

- Sascha

Ard Biesheuvel

23 years ago

>>account that every pointer in scope might be an alias of the resulting
>>value. __attribute_malloc__ fixes that. (Disabled for non-GCC compilers)
>
>
>     What about erealloc?  It can return the same pointer as
>     passed to it.

GNU C realloc() does have the __attribute__((malloc)) itself. This seems
reasonable, because, from a strict-aliasing point of view, the result of
the function is either a new 'fresh' pointer, or equal to the first
argument passed to realloc(), to which the strict-aliasing rules apply
already.

Ard