Julian Foad <julianfoad_at_btopenworld.com> writes:
> 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.
That's exactly the problem I described.
>> 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?
Yes, I think I am.
> 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 problem is that "svn_wc_entry_t*" isn't guaranteed to be big
enough to hold the void* value that gets written by apr_hash_this. On
most platforms all data pointers are the same size, but that's not
guaranteed by the standard. Suppose svn_wc_entry_t* is 32bits but
void* is 64bits. You are passing the address of a 32bit block of
memory into apr_hash_this but within the function the code will write
a 64bit value. It's like passing the address of a 32bit int into a
function that writes a 64bit int; casting doesn't solve the problem:
void foo(u64 *p) { *p = 0; }
u32 i;
foo(&i); /* incompatible pointer warning */
foo((u64*)&i); /* no warning, but invalid code */
> 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?
Type-punning is a separate issue, I find it hard to work out whether
it's a false alarm partly because the code has this other incompatible
pointer problem.
>
>> > +#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.
Three macros perhaps?
APR_HASH_THIS_VAL(hi,val)
APR_HASH_THIS_KEY_VAL(hi,key,val)
APR_HASH_THIS_KEY_KLEN_VAL(hi,key,klen,val)
>
> (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.)
It's not.
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2366775
Received on 2009-06-30 19:22:56 CEST