Philip Martin wrote:
> Julian Foad <julianfoad_at_btopenworld.com> writes:
>
> This is gcc's "type-punned pointer will break strict-aliasing"
> warning;
Thanks for the response.
The warnings I'm talking about (that I get from gcc if I use neither
casts nor temp vars with apr_hash_this()) are:
warning: passing argument 2 [or 4] of 'apr_hash_this' from
incompatible pointer type
I'm not sure if you're looking at the same issue.
> I've never been able to decide whether this is a false alarm
> or not. The gcc documentation states that false alarms may occur but
> that there should be very few of them. However even if it is a false
> alarm the code is suspect for another reason.
>
> > which is what we normally do, or add explicit type casts:
> >
> > for (hi = apr_hash_first(pool, entries); hi; hi = apr_hash_next(hi))
> > {
> > const char *item;
> > svn_wc_entry_t *entry;
> >
> > apr_hash_this(hi, (void *)&item, NULL, (void *)&entry);
> >
> > ...
> > }
>
> I don't think that's valid, although it will work on most common
> platforms today. The problem is that in C a void* pointer can be
> bigger than any particular data pointer. When the compiler assigns
> to/from void* it adjusts for size as necessary, but the cast above
> defeats that. So inside apr_hash_this a void* value will get written
> into the space occupied by entry and that space might not be big
> enough.
Are you sure you're looking at the right level of indirection?
apr_hash_this() is a function whose prototype ensures its last argument
gets converted to type (void **). All we're doing here is casting
(&entry) to type (void *) before it gets converted automatically to
(void **), instead of letting the conversion happen in a single step. I
assume that the result of this pair of type conversions would be the
same as the single step, on all platforms. The result is the same, but,
by making an intermediate explicit cast, we avoid the "converting
between incompatible pointer target types" warning.
The issue of whether (void **) is compatible with (&entry) is a separate
issue that has always existed with this APR API. Is that the
"type-punned" issue that you're talking about?
> > +#if 1
> > +#define APR_HASH_THIS(hi,key,klen,val) \
> > + apr_hash_this((hi), (void *)(key), (klen), (void *)(val))
>
> This is not valid for the reason above.
>
> > +#else
> > +#define APR_HASH_THIS(hi,key,klen,val) \
> > + do { \
> > + const void *__svn_key; \
> > + apr_ssize_t __svn_klen; \
> > + void *__svn_val; \
> > + apr_hash_this((hi), &__svn_key, &__svn_klen, &__svn_val); \
> > + if (key) *((const void **)key) = __svn_key; \
> > + if (klen) *((apr_ssize_t *)klen) = __svn_klen; \
> > + if (val) *((void **)val) = __svn_val; \
> > + } while (0)
> > +#endif
>
> That's not valid because the casts in the three assignment lines have
> the same problem. It should be OK without the casts.
Here I agree that these casts are unsafe for systems on which pointers
are not all the same size. I added these casts only to be able to
compile this code without warnings when the "calling" code passes NULL
for key/klen/val. If I need to use an implementation like this one I
will look for a different way to achieve that.
(Why have I even shown you this particular implementation? Because I
wrote it before I discovered the simple "#if 1" solution, and could
hardly believe that the simple solution will be OK.)
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2366763
Received on 2009-06-30 18:31:25 CEST