[ovs-dev] [PATCH 2/5] lib/cmap, lib/classifier: Avoid aliasing pointers.

[Jarno Rajahalme]

Sent from my iPhone

> On Jul 19, 2014, at 10:03 AM, Ben Pfaff <blp at nicira.com> wrote:
> 
>> On Fri, Jul 18, 2014 at 11:33:38PM -0700, Ben Pfaff wrote:
>>> On Fri, Jul 18, 2014 at 09:05:49PM -0700, Jarno Rajahalme wrote:
>>> Older GCC (e.g. 4.1.2) did not like the pointer casts used for
>>> initializing the iteration cursors.  This patch changes the code to
>>> avoid the void casts for GCC, and makes the classifier interface more
>>> similar to that of the cmap.  These changes make the code work with
>>> GCC 4.1.2 strict aliasing rules.
>>> 
>>> VMware-BZ: #1287651
>>> 
>>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
>> 
>> I see why GCC didn't like this code.
>> 
>> There are lots of tricky constraints here.  We don't want to violate ISO
>> aliasing rules, we have to stick to exactly one declaration in the first
>> clause of the for statement, and we don't want to have different code
>> for GCC and other compilers if we don't have to.  The proposed solution
>> here compromises on the last one.
>> 
>> I spent a bunch of time experimenting and came up with what I think is
>> the outline of another solution.  How about adding a 'node' member to
>> cmap_cursor, like this:
>> 
>> struct cmap_cursor {
>>    const struct cmap_impl *impl;
>>    uint32_t bucket_idx;
>>    int entry_idx;
>>    struct cmap_node *node;
>> };
>> 
>> struct cmap_cursor cmap_cursor_start(const struct cmap *);
>> void cmap_cursor_advance(struct cmap_cursor *);
>> 
>> #define CMAP_FOR_EACH(NODE, MEMBER, CMAP)                               \
>>    for (struct cmap_cursor cursor__ = cmap_cursor_start(CMAP); \
>>         (cursor__.node                                                 \
>>          ? (ASSIGN_CONTAINER(NODE, cursor__.node, MEMBER), true)       \
>>          : false);                                                     \
>>         cmap_cursor_advance(&cursor__))
>> 
>> #define CMAP_FOR_EACH_SAFE(NODE, MEMBER, CMAP)                          \
>>    for (struct cmap_cursor cursor__ = cmap_cursor_start(CMAP); \
>>         (cursor__.node                                                 \
>>          ? (ASSIGN_CONTAINER(NODE, cursor__.node, MEMBER),             \
>>             cmap_cursor_advance(&cursor__), \
>>             true)                                                      \
>>          : false);                                                     \
>>        )
>> 
>> with corresponding support functions:
>> 
>> struct cmap_cursor
>> cmap_cursor_start(const struct cmap *cmap)
>> {
>>    struct cmap_cursor cursor;
>> 
>>    cursor.impl = cmap_get_impl(cmap);
>>    cursor.bucket_idx = 0;
>>    cursor.entry_idx = 0;
>>    cursor.node = NULL;
>>    cmap_cursor_advance(&cursor);
>> 
>>    return cursor;
>> }
>> 
>> void
>> cmap_cursor_advance(struct cmap_cursor *cursor)
>> {
>>    const struct cmap_impl *impl = cursor->impl;
>> 
>>    if (cursor->node) {
>>        cursor->node = cmap_node_next(cursor->node);
>>        if (cursor->node) {
>>            return;
>>        }
>>    }
>> 
>>    while (cursor->bucket_idx <= impl->mask) {
>>        const struct cmap_bucket *b = &impl->buckets[cursor->bucket_idx];
>> 
>>        while (cursor->entry_idx < CMAP_K) {
>>            cursor->node = cmap_node_next(&b->nodes[cursor->entry_idx++]);
>>            if (cursor->node) {
>>                return;
>>            }
>>        }
>> 
>>        cursor->bucket_idx++;
>>        cursor->entry_idx = 0;
>>    }
>> }
> 
> Here's a fully worked out version of this idea that compiles and passes
> all the tests (at least with GCC 4.7.2).  With this, the _SAFE variants
> don't take an extra named variable or really have any extra cost, so one
> thinks about just getting making the normal variant the same as _SAFE:
> 

Just a quick note, more later: Corresponding changes to classifier would be more complex, and there is a locking difference between the normal and safe cases. Did you consider if classifier should use this same pattern?

  Jarno