UCS/CONFIG/PARSER: Handle NULL entries in sparse arrays

[michal-shalev]

What?

Following #10172

• Changed parser.c to support sparse parsing of configuration environment variables.
• Removed unnecessary flags from UCX_EXTRA_OP_ATTR_FLAGS.
• Removed [*_LAST] = NULL null entries in configuration arrays.
• Introduced UCS_CONFIG_DECLARE_ALLOWED_VALUES and UCS_CONFIG_DEFINE_ALLOWED_VALUES macros for cleaner handling of configuration arrays and their associated allowed values.
• Added UCS_CONFIG_GET_ALLOWED_VALUES macro to retrieve allowed values consistently.
• Changed UCS_CONFIG_TYPE_ENUM, UCS_CONFIG_TYPE_UINT_ENUM, and UCS_CONFIG_TYPE_BITMAP to work seamlessly with the new macros.

Why?

• The previous implementation relied on the last entry in configuration arrays being NULL to determine the end of the array. This required unnecessary placeholder entries like [*_LAST] = NULL.
• The previous implementation of UCS_CONFIG_TYPE_ENUM, UCS_CONFIG_TYPE_UINT_ENUM, and UCS_CONFIG_TYPE_BITMAP required contiguous indices, leading to core dumps when accessing NULL values due to gaps in the array.

How?

• Introduced ucs_config_allowed_values_t to explicitly pass both the array pointer and the number of valid entries to the parser.
• Updated parser.c to use the size specified in ucs_config_allowed_values_t instead of relying on NULL-terminated arrays.
• Added the following macros:
  • UCS_CONFIG_DECLARE_ALLOWED_VALUES: For declaring allowed values arrays in headers when they are defined elsewhere.
  • UCS_CONFIG_DEFINE_ALLOWED_VALUES: For defining allowed values arrays in source files.
  • UCS_CONFIG_GET_ALLOWED_VALUES: For retrieving the allowed values object associated with an array.
• Modified UCS_CONFIG_TYPE_ENUM, UCS_CONFIG_TYPE_UINT_ENUM, and UCS_CONFIG_TYPE_BITMAP to leverage the new macros and handle sparse arrays properly.

[iyastreb]

I would propose to create a variable to avoid repetitive casts:
uint64_t *flags = dest;

[michal-shalev]

Outdated?

[iyastreb]

consider using ucs_string_split

[michal-shalev]

Outdated?

[tvegas1]

is it possible to fix UCS_CONFIG_TYPE_BITMAP for sparse, instead of introducing new UCS_CONFIG_TYPE_FLAGS?

[michal-shalev]

is it possible to fix UCS_CONFIG_TYPE_BITMAP for sparse, instead of introducing new UCS_CONFIG_TYPE_FLAGS?

@tvegas1
The alternative is to change UCS_CONFIG_TYPE_BITMAP instances unrelated to the flags, but it is not critical for me.

[yosefe]

is it possible to fix UCS_CONFIG_TYPE_BITMAP for sparse, instead of introducing new UCS_CONFIG_TYPE_FLAGS?

We could do it nicely by adding another "unsigned long" field to ucs_config_parser_t to hold the count, something like

typedef struct ucs_config_parser {
    ...
    void                     (*help)(char *buf, size_t max, const void *arg);
    void                     (*doc)(ucs_string_buffer_t *strb, const void *arg);
    const void               *arg_ptr;
    unsigned long            arg_ulong;
};

And setting arg_ulong by ucs_static_array_size

[michal-shalev]

is it possible to fix UCS_CONFIG_TYPE_BITMAP for sparse, instead of introducing new UCS_CONFIG_TYPE_FLAGS?

We could do it nicely by adding another "unsigned long" field to ucs_config_parser_t to hold the count, something like

typedef struct ucs_config_parser {
    ...
    void                     (*help)(char *buf, size_t max, const void *arg);
    void                     (*doc)(ucs_string_buffer_t *strb, const void *arg);
    const void               *arg_ptr;
    unsigned long            arg_ulong;
};

And setting arg_ulong by ucs_static_array_size

We could but it will require changes in all the parsers, which is more than the scope of the flags, LMK WDYT @yosefe

[yosefe]

is it possible to fix UCS_CONFIG_TYPE_BITMAP for sparse, instead of introducing new UCS_CONFIG_TYPE_FLAGS?

We could do it nicely by adding another "unsigned long" field to ucs_config_parser_t to hold the count, something like

typedef struct ucs_config_parser {
    ...
    void                     (*help)(char *buf, size_t max, const void *arg);
    void                     (*doc)(ucs_string_buffer_t *strb, const void *arg);
    const void               *arg_ptr;
    unsigned long            arg_ulong;
};

And setting arg_ulong by ucs_static_array_size

We could but it will require changes in all the parsers, which is more than the scope of the flags, LMK WDYT @yosefe

I think it's worth it , but would appreciate @gleon99 @brminich @tvegas1 opinions

[tvegas1]

We could but it will require changes in all the parsers, which is more than the scope of the flags, LMK WDYT @yosefe

I think it's worth it , but would appreciate @gleon99 @brminich @tvegas1 opinions

I would try to have only one UCS_CONFIG_TYPE_BITMAP that supports holes because _TYPE_FLAGS seems to be more or less the same, if that's possible.

[iyastreb]

I think you can get rid of all DECLARE and DEFINE macros by using slightly more advanced macro here, with static initializer and compound literal:

#define UCS_CONFIG_TYPE_ENUM(t)    {ucs_config_sscanf_enum,      ucs_config_sprintf_enum, \
                                    ucs_config_clone_uint,       ucs_config_release_nop, \
                                    ucs_config_help_enum,        ucs_config_doc_nop, \
        (const ucs_config_allowed_values_t[]) { \
            { .values=t, .count = ucs_static_array_size(t)} \
        } \
}

[michal-shalev]

The proposed approach using will not work for cases where the array is declared as extern in a header file and defined elsewhere, which is common in our codebase.

For example, arrays like ucs_memory_type_names are declared as extern in the header file memory_type.h and defined in the source file memory_type.c.
When sizeof is used in a file where only the extern declaration is visible, the compiler treats the array as an incomplete type, resulting in the following error:
error: invalid application of ‘sizeof’ to incomplete type ‘const char *[]’

[iyastreb]

Ok, I see
Maybe we declare these extern vars with array size?
E.g:

extern const char *ucm_mmap_hook_modes[UCM_MMAP_HOOK_LAST + 1];

[iyastreb]

I'm not sure we need to remove all those LAST fields. Does it harm if we keep them?
If not, I would keep them and reduce the PR size

[michal-shalev]

I removed the LAST fields that were no longer used anywhere and were not needed anymore

[rakhmets]

I think the line can be remove, it is already included in the help's output.

[michal-shalev]

Other UCS_CONFIG_TYPE_BITMAP help messages in ucp_context_config_table show the options as well, see reg_whole_alloc_bitmap for example

[rakhmets]

I mean the same information is repeated twice in the output of ucx_info -cf:

# Additional send/receive operation flags that are added for each requestin addition to what is set explicitly by the user. 
# Possible values are: fast_cmpl, multi_send.
# 
#
# syntax:    comma-separated list of: [fast_cmpl|multi_send]

Some other messages have the same issue.

[rakhmets]

I think we can make it const char *const, since all related function signatures have been changed in the PR.

[michal-shalev]

Do you mean in parser.h? const char *buf?

[rakhmets]

I mean it is not just an array of constant strings, but a constant-size array of constant strings.

[michal-shalev]

@gleon99 @iyastreb
After a discussion with @yosefe, he requested to move forward with this code by replacing the macro-based approach with a new data structure that maps numbers to strings. Since most arrays in the config parser are based on enums, this structure should also store the size, addressing both the sparse array issue and cases where different parts of the code need to retrieve a string based on an enum offset.
However, since this task is low priority, it will take some time before I get back to it. Moving this back to draft for now.