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

[Ben Pfaff]

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 <jrajaha...@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;
    }
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev