You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Joe Orton <jo...@manyfish.co.uk> on 2004/02/05 18:11:29 UTC
[PATCH] optional hook macros
gcc 3.4 gives a warning for all (valid or invalid) uses of
APR_REGISTER_OPTIONAL_FN and APR_OPTIONAL_HOOK: e.g.:
mpm_common.c:894: warning: function called through a non-compatible type
(this is emitted even without -Wall)
>From delving through C99, this appears to be a valid warning, since the
code has undefined behaviour by section 6.3.2.3 para 8. (which says, to
try and paraphrase: behaviour is undefined if you call a function
through a function pointer where the function and the function pointer
don't have compatible types)
Given that, I don't see how these macros can be written to be both
conformant C and type-safe. It can be be made type-safe using GCC
extensions, for instance, as below: any better ideas?
It might be prudent to also make the !__GNUC__ case valid C but not
type-safe, by casting the pfn argument rather than casting
apr_dynamic_fn_register itself.
Index: include/apr_optional.h
===================================================================
RCS file: /home/cvs/apr-util/include/apr_optional.h,v
retrieving revision 1.12
diff -u -r1.12 apr_optional.h
--- include/apr_optional.h 16 Nov 2003 01:50:10 -0000 1.12
+++ include/apr_optional.h 5 Feb 2004 16:36:19 -0000
@@ -105,9 +105,18 @@
* confusingly but correctly, the function itself can be static!
* @param name The name of the function
*/
+#ifdef __GNUC__
+#define APR_REGISTER_OPTIONAL_FN(name) do { \
+ APR_OPTIONAL_FN_TYPE(name) apu__opt1; \
+ typeof(name) apu__opt2; \
+ (void) (&apu__opt1 == &apu__opt2); \
+ apr_dynamic_fn_register(#name,(apr_opt_fn_t *)name); \
+} while (0)
+#else
#define APR_REGISTER_OPTIONAL_FN(name) \
(((void (*)(const char *, APR_OPTIONAL_FN_TYPE(name) *)) \
&apr_dynamic_fn_register)(#name,name))
+#endif
/** @internal
* Private function! DO NOT USE!
Index: include/apr_optional_hooks.h
===================================================================
RCS file: /home/cvs/apr-util/include/apr_optional_hooks.h,v
retrieving revision 1.9
diff -u -r1.9 apr_optional_hooks.h
--- include/apr_optional_hooks.h 1 Jan 2003 00:02:20 -0000 1.9
+++ include/apr_optional_hooks.h 5 Feb 2004 16:36:19 -0000
@@ -99,10 +99,20 @@
* @param nOrder an integer determining order before honouring aszPre and aszSucc (for example HOOK_MIDDLE)
*/
+#ifdef __GNUC__
+#define APR_OPTIONAL_HOOK(ns,name,pfn,aszPre,aszSucc,nOrder) \
+do { \
+ ns##_HOOK_##name##_t apu__opt1; \
+ typeof(pfn) apu__opt2; \
+ (void) (&apu__opt1 == &apu__opt2); \
+ apr_optional_hook_add(#name,(apr_opt_fn_t *)pfn,aszPre, aszSucc, nOrder); \
+} while (0)
+#else
#define APR_OPTIONAL_HOOK(ns,name,pfn,aszPre,aszSucc,nOrder) \
((void (APR_THREAD_FUNC *)(const char *,ns##_HOOK_##name##_t *,const char * const *, \
const char * const *,int))&apr_optional_hook_add)(#name,pfn,aszPre, \
aszSucc, nOrder)
+#endif
/**
* @internal
Re: [PATCH] optional hook macros
Posted by Ben Laurie <be...@algroup.co.uk>.
Joe Orton wrote:
> On Fri, Feb 06, 2004 at 10:06:14AM +0000, Ben Laurie wrote:
>>Surely the point is that the function and the function pointer actually
>>_do_ have compatible types, but we hold the pointer in a variable that
>>doesn't. That is, we cast it to an incompatible type for storage, then
>>cast it back.
>
> The issue is the call to apr_dynamic_fn_register, here's how I read it:
>
> (((void (*)(const char *, APR_OPTIONAL_FN_TYPE(name) *))
> &apr_dynamic_fn_register)(#name,name))
>
> this takes a function pointer (&apr_dynamic_fn_register), then casts it
> to a different type (void (*)(const blah blah)), then calls the function
> using this type. That has undefined behaviour, since the type of
> apr_dynamic_fn_register is not compatible with the (void (*)(const blah
> blah) type.
I misunderstood, sorry.
Cheers,
Ben.
--
http://www.apache-ssl.org/ben.html http://www.thebunker.net/
"There is no limit to what a man can do or how far he can go if he
doesn't mind who gets the credit." - Robert Woodruff
Re: [PATCH] optional hook macros
Posted by Ben Laurie <be...@algroup.co.uk>.
Joe Orton wrote:
> On Fri, Feb 06, 2004 at 10:31:07AM +0000, Joe Orton wrote:
>
>>Here's a simpler version of the patch
>
> ...
>
> which doesn't use GCC extensions! Here's my final attempt:
Looks OK to me, but you have to wonder about the warning if this is legal!
Cheers,
Ben.
>
> --- apr-util-0.9.4/include/apr_optional.h.gcc34
> +++ apr-util-0.9.4/include/apr_optional.h
> @@ -109,9 +109,10 @@
> * confusingly but correctly, the function itself can be static!
> * @param name The name of the function
> */
> -#define APR_REGISTER_OPTIONAL_FN(name) \
> - (((void (*)(const char *, APR_OPTIONAL_FN_TYPE(name) *)) \
> - &apr_dynamic_fn_register)(#name,name))
> +#define APR_REGISTER_OPTIONAL_FN(name) do { \
> + APR_OPTIONAL_FN_TYPE(name) *apu__opt = name; \
> + apr_dynamic_fn_register(#name,(apr_opt_fn_t *)apu__opt); \
> +} while (0)
>
> /** @internal
> * Private function! DO NOT USE!
> --- apr-util-0.9.4/include/apr_optional_hooks.h.gcc34
> +++ apr-util-0.9.4/include/apr_optional_hooks.h
> @@ -99,10 +99,10 @@
> * @param nOrder an integer determining order before honouring aszPre and aszSucc (for example HOOK_MIDDLE)
> */
>
> -#define APR_OPTIONAL_HOOK(ns,name,pfn,aszPre,aszSucc,nOrder) \
> - ((void (APR_THREAD_FUNC *)(const char *,ns##_HOOK_##name##_t *,const char * const *, \
> - const char * const *,int))&apr_optional_hook_add)(#name,pfn,aszPre, \
> - aszSucc, nOrder)
> +#define APR_OPTIONAL_HOOK(ns,name,pfn,aszPre,aszSucc,nOrder) do { \
> + ns##_HOOK_##name##_t *apu__hook = pfn; \
> + apr_optional_hook_add(#name,(void (*)(void))apu__hook,aszPre, aszSucc, nOrder); \
> +} while (0)
>
> /**
> * @internal
>
--
http://www.apache-ssl.org/ben.html http://www.thebunker.net/
"There is no limit to what a man can do or how far he can go if he
doesn't mind who gets the credit." - Robert Woodruff
Re: [PATCH] optional hook macros
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Feb 06, 2004 at 10:31:07AM +0000, Joe Orton wrote:
> Here's a simpler version of the patch
...
which doesn't use GCC extensions! Here's my final attempt:
--- apr-util-0.9.4/include/apr_optional.h.gcc34
+++ apr-util-0.9.4/include/apr_optional.h
@@ -109,9 +109,10 @@
* confusingly but correctly, the function itself can be static!
* @param name The name of the function
*/
-#define APR_REGISTER_OPTIONAL_FN(name) \
- (((void (*)(const char *, APR_OPTIONAL_FN_TYPE(name) *)) \
- &apr_dynamic_fn_register)(#name,name))
+#define APR_REGISTER_OPTIONAL_FN(name) do { \
+ APR_OPTIONAL_FN_TYPE(name) *apu__opt = name; \
+ apr_dynamic_fn_register(#name,(apr_opt_fn_t *)apu__opt); \
+} while (0)
/** @internal
* Private function! DO NOT USE!
--- apr-util-0.9.4/include/apr_optional_hooks.h.gcc34
+++ apr-util-0.9.4/include/apr_optional_hooks.h
@@ -99,10 +99,10 @@
* @param nOrder an integer determining order before honouring aszPre and aszSucc (for example HOOK_MIDDLE)
*/
-#define APR_OPTIONAL_HOOK(ns,name,pfn,aszPre,aszSucc,nOrder) \
- ((void (APR_THREAD_FUNC *)(const char *,ns##_HOOK_##name##_t *,const char * const *, \
- const char * const *,int))&apr_optional_hook_add)(#name,pfn,aszPre, \
- aszSucc, nOrder)
+#define APR_OPTIONAL_HOOK(ns,name,pfn,aszPre,aszSucc,nOrder) do { \
+ ns##_HOOK_##name##_t *apu__hook = pfn; \
+ apr_optional_hook_add(#name,(void (*)(void))apu__hook,aszPre, aszSucc, nOrder); \
+} while (0)
/**
* @internal
Re: [PATCH] optional hook macros
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Fri, Feb 06, 2004 at 10:03:46AM -0500, Joe Schaefer wrote:
> Joe Orton <jo...@redhat.com> writes:
>
> > On Fri, Feb 06, 2004 at 10:06:14AM +0000, Ben Laurie wrote:
> > > Joe Orton wrote:
> > > >From delving through C99, this appears to be a valid warning, since the
> > > >code has undefined behaviour by section 6.3.2.3 para 8. (which says, to
> > > >try and paraphrase: behaviour is undefined if you call a function
> > > >through a function pointer where the function and the function pointer
> > > >don't have compatible types)
> > >
> > > Sigh. OK, how do I get hold of C99?
> >
> > Only by buying a copy from ISO I think.
>
> However, the N869 draft is free (lots of links to it on google).
Ah, thanks. The paragraph in question is in the same place in that
draft, for reference.
joe
Re: [PATCH] optional hook macros
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Joe Orton <jo...@redhat.com> writes:
> On Fri, Feb 06, 2004 at 10:06:14AM +0000, Ben Laurie wrote:
> > Joe Orton wrote:
> > >From delving through C99, this appears to be a valid warning, since the
> > >code has undefined behaviour by section 6.3.2.3 para 8. (which says, to
> > >try and paraphrase: behaviour is undefined if you call a function
> > >through a function pointer where the function and the function pointer
> > >don't have compatible types)
> >
> > Sigh. OK, how do I get hold of C99?
>
> Only by buying a copy from ISO I think.
However, the N869 draft is free (lots of links to it on google).
--
Joe Schaefer
Re: [PATCH] optional hook macros
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Feb 06, 2004 at 10:06:14AM +0000, Ben Laurie wrote:
> Joe Orton wrote:
> >From delving through C99, this appears to be a valid warning, since the
> >code has undefined behaviour by section 6.3.2.3 para 8. (which says, to
> >try and paraphrase: behaviour is undefined if you call a function
> >through a function pointer where the function and the function pointer
> >don't have compatible types)
>
> Sigh. OK, how do I get hold of C99?
Only by buying a copy from ISO I think.
> Surely the point is that the function and the function pointer actually
> _do_ have compatible types, but we hold the pointer in a variable that
> doesn't. That is, we cast it to an incompatible type for storage, then
> cast it back.
The issue is the call to apr_dynamic_fn_register, here's how I read it:
(((void (*)(const char *, APR_OPTIONAL_FN_TYPE(name) *))
&apr_dynamic_fn_register)(#name,name))
this takes a function pointer (&apr_dynamic_fn_register), then casts it
to a different type (void (*)(const blah blah)), then calls the function
using this type. That has undefined behaviour, since the type of
apr_dynamic_fn_register is not compatible with the (void (*)(const blah
blah) type.
Here's a simpler version of the patch: httpd-2.0 builds with "gcc
version 3.4.0 20040121" at -Wall -Werror and handles requests fine with
this applied. Deliberately introducing a type-unsafe use of either
macro gets a warning like:
mpm_common.c: In function `ap_mpm_rewrite_args':
mpm_common.c:884: warning: initialization from incompatible pointer type
--- apr-util-0.9.4/include/apr_optional.h.gcc34
+++ apr-util-0.9.4/include/apr_optional.h
@@ -109,9 +109,16 @@
* confusingly but correctly, the function itself can be static!
* @param name The name of the function
*/
+#ifdef __GNUC__
+#define APR_REGISTER_OPTIONAL_FN(name) do { \
+ APR_OPTIONAL_FN_TYPE(name) *apu__opt = name; \
+ apr_dynamic_fn_register(#name,(apr_opt_fn_t *)apu__opt); \
+} while (0)
+#else
#define APR_REGISTER_OPTIONAL_FN(name) \
(((void (*)(const char *, APR_OPTIONAL_FN_TYPE(name) *)) \
&apr_dynamic_fn_register)(#name,name))
+#endif
/** @internal
* Private function! DO NOT USE!
--- apr-util-0.9.4/include/apr_optional_hooks.h.gcc34
+++ apr-util-0.9.4/include/apr_optional_hooks.h
@@ -99,10 +99,17 @@
* @param nOrder an integer determining order before honouring aszPre and aszSucc (for example HOOK_MIDDLE)
*/
+#ifdef __GNUC__
+#define APR_OPTIONAL_HOOK(ns,name,pfn,aszPre,aszSucc,nOrder) do { \
+ ns##_HOOK_##name##_t *apu__hook = pfn; \
+ apr_optional_hook_add(#name,(apr_opt_fn_t *)apu__hook,aszPre, aszSucc, nOrder); \
+} while (0)
+#else
#define APR_OPTIONAL_HOOK(ns,name,pfn,aszPre,aszSucc,nOrder) \
((void (APR_THREAD_FUNC *)(const char *,ns##_HOOK_##name##_t *,const char * const *, \
const char * const *,int))&apr_optional_hook_add)(#name,pfn,aszPre, \
aszSucc, nOrder)
+#endif
/**
* @internal
Re: [PATCH] optional hook macros
Posted by Ben Laurie <be...@algroup.co.uk>.
Joe Orton wrote:
> From delving through C99, this appears to be a valid warning, since the
> code has undefined behaviour by section 6.3.2.3 para 8. (which says, to
> try and paraphrase: behaviour is undefined if you call a function
> through a function pointer where the function and the function pointer
> don't have compatible types)
Sigh. OK, how do I get hold of C99?
Surely the point is that the function and the function pointer actually
_do_ have compatible types, but we hold the pointer in a variable that
doesn't. That is, we cast it to an incompatible type for storage, then
cast it back. So, the warning, though correct in theory, is wrong in
practice. Can we fix it by casting the storage instead?
> Given that, I don't see how these macros can be written to be both
> conformant C and type-safe. It can be be made type-safe using GCC
> extensions, for instance, as below: any better ideas?
>
> It might be prudent to also make the !__GNUC__ case valid C but not
> type-safe, by casting the pfn argument rather than casting
> apr_dynamic_fn_register itself.
> Index: include/apr_optional.h
> ===================================================================
> RCS file: /home/cvs/apr-util/include/apr_optional.h,v
> retrieving revision 1.12
> diff -u -r1.12 apr_optional.h
> --- include/apr_optional.h 16 Nov 2003 01:50:10 -0000 1.12
> +++ include/apr_optional.h 5 Feb 2004 16:36:19 -0000
> @@ -105,9 +105,18 @@
> * confusingly but correctly, the function itself can be static!
> * @param name The name of the function
> */
> +#ifdef __GNUC__
> +#define APR_REGISTER_OPTIONAL_FN(name) do { \
> + APR_OPTIONAL_FN_TYPE(name) apu__opt1; \
> + typeof(name) apu__opt2; \
> + (void) (&apu__opt1 == &apu__opt2); \
> + apr_dynamic_fn_register(#name,(apr_opt_fn_t *)name); \
> +} while (0)
> +#else
> #define APR_REGISTER_OPTIONAL_FN(name) \
> (((void (*)(const char *, APR_OPTIONAL_FN_TYPE(name) *)) \
> &apr_dynamic_fn_register)(#name,name))
> +#endif
>
> /** @internal
> * Private function! DO NOT USE!
> Index: include/apr_optional_hooks.h
> ===================================================================
> RCS file: /home/cvs/apr-util/include/apr_optional_hooks.h,v
> retrieving revision 1.9
> diff -u -r1.9 apr_optional_hooks.h
> --- include/apr_optional_hooks.h 1 Jan 2003 00:02:20 -0000 1.9
> +++ include/apr_optional_hooks.h 5 Feb 2004 16:36:19 -0000
> @@ -99,10 +99,20 @@
> * @param nOrder an integer determining order before honouring aszPre and aszSucc (for example HOOK_MIDDLE)
> */
>
> +#ifdef __GNUC__
> +#define APR_OPTIONAL_HOOK(ns,name,pfn,aszPre,aszSucc,nOrder) \
> +do { \
> + ns##_HOOK_##name##_t apu__opt1; \
> + typeof(pfn) apu__opt2; \
> + (void) (&apu__opt1 == &apu__opt2); \
> + apr_optional_hook_add(#name,(apr_opt_fn_t *)pfn,aszPre, aszSucc, nOrder); \
> +} while (0)
> +#else
> #define APR_OPTIONAL_HOOK(ns,name,pfn,aszPre,aszSucc,nOrder) \
> ((void (APR_THREAD_FUNC *)(const char *,ns##_HOOK_##name##_t *,const char * const *, \
> const char * const *,int))&apr_optional_hook_add)(#name,pfn,aszPre, \
> aszSucc, nOrder)
> +#endif
>
> /**
> * @internal
>
>
--
http://www.apache-ssl.org/ben.html http://www.thebunker.net/
"There is no limit to what a man can do or how far he can go if he
doesn't mind who gets the credit." - Robert Woodruff