opal/obj: add support for flexible-sized objecs

[hjelmn]

This commit adds a new macro to the opal object system:
OBJ_NEW_FLEXIBLE. This new macro allocates additional space beyond the
size of the object to support opal objects that contain a C99 flexible
array member.

Signed-off-by: Nathan Hjelm hjelmn@lanl.gov

[hjelmn]

I am proposing this PR to make it possible to use the opal object system to support C99 flexible array members. The use case I have is this class:

struct mca_btl_base_endpoint_t {
    /** opal base class */
    opal_object_t super;

    /** endpoint proc */
    opal_proc_t *ep_proc;

    /** mutex to protect this structure */
    opal_recursive_mutex_t ep_lock;

    /** cached connection endpoint */
    mca_btl_uct_connection_ep_t *conn_ep;

    /** endpoints into UCT for this BTL endpoint */
    mca_btl_uct_tl_endpoint_t uct_eps[2][MCA_BTL_UCT_MAX_WORKERS];
};

I would like to modify the class to be:

struct mca_btl_base_endpoint_t {
    /** opal base class */
    opal_object_t super;

    /** endpoint proc */
    opal_proc_t *ep_proc;

    /** mutex to protect this structure */
    opal_recursive_mutex_t ep_lock;

    /** cached connection endpoint */
    mca_btl_uct_connection_ep_t *conn_ep;

    /** endpoints into UCT for this BTL endpoint */
    mca_btl_uct_tl_endpoint_t uct_eps[];
};

to save space and avoid adding an extra dereference to get the UCT endpoint data from the BTL endpoint data structure.

[hjelmn]

@bosilca, @jsquyres, @rhc54 Comments?

[rhc54]

I'm puzzled, but maybe just ignorant. How does that differ from declaring uct_eps as a pointer? You still have to allocate space for the actual array, so how does this help?

[hjelmn]

Note that we don't have to worry about any derived classes here. It is illegal in C99 to put anything after a flexible array member. This applies to nested structs.

[hjelmn]

@rhc54 It avoids an extra pointer dereference on the critical path. The space for the array is allocated as part of the struct itself so endpoint->uct_eps[i] is just an offset calculation.

[rhc54]

Like I said, I'm just ignorant. In my old brain, int foo[] = int *foo in C-land. In both cases, there is no storage allocated because the compiler has no idea how much space you need - so at some point or another, you have to do a malloc. Yes?

[ibm-ompi]

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/d7e609116accd017f945727ec521fa1e

[ibm-ompi]

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/6a2d5dee871198169253425a0ec6e53e

[ibm-ompi]

The IBM CI (PGI Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/d5e75928eb4b9c00b2d19e66ddaf820f

[hjelmn]

@rhc54 Ah, ok. Thats what I am not explaining well enough. The space is allocated with the struct itself (which is why we need a different NEW macro). When you create a flexible structure you don't call malloc (sizeof (mca_btl_base_endpoint_t)) but instead malloc(sizeof (mca_btl_base_endpoint_t) + size) where size is the total size of the flexible array. Since it is an array and not a pointer (uintptr_t) &endpoint->uct_eps[i] == (uintptr_t) endpoint + offsetof(mca_btl_base_endpoint_t, uct_eps) + sizeof (endpoint->uct_eps[0]) * i. There is no pointer to dereference.

We use flexible array members in a couple of places in Open MPI but generally with an opal_free_list_t allocated object (ex: mca_pml_ob1_send_request_t). This PR allows them to be used more generally.

[rhc54]

Sigh - my apologies. It would have helped if I had actually looked at the files and not just your comments here. I saw the struct definition you posted here, but missed the fact that your new macro included passing the size for the array. Now it makes total sense. Thanks!

[hjelmn]

No problem

[jsquyres]

👍

[bosilca]

This extension provides no benefit compared with calloc + OBJ_CONSTRUCT, and it decreases the code readability (with the exception of the few corner cases that look like your example).

As we are now forcing C99 compliance, why not using the VA_ARGS to pass the OBJ_NEW arguments down to the first level constructor ?

[hjelmn]

@bosilca I see this as a clean way to officially support the variable sized struct case. Right now it is safe to calloc + OBJ_DESTRUCT but do we say it is ok to OBJ_RELEASE in this case? OBJ_NEW_FLEXIBLE makes support for that explicit.

Also, I do think we should have first level constructor arguments. It is something I plan on proposing once I have worked out all the cases.

[awlauria]

Ping. 5.0 branching is targeted for April 30th. If you want this in for 5.0, please target to get it in master by then. Thanks!

[lanl-ompi]

Can one of the admins verify this patch?

[jsquyres]

@bosilca Nathan made a decent point about OBJ_RELEASE. Do we still want this PR?

[bosilca]

I see little benefit compared with calloc + OBJ_CONSTRUCT, matched with OBJ_RELEASE except hiding the calloc into the macro. At the end, with the exception of static cases as in Nathan example, the constructors still have no idea what they are supposed to do because they have no context information.

[jsquyres]

Ok. Closing this PR without merging. If desired, it can always be opened / refreshed.