Re: [PATCH 09/13] parse-options API: don't restrict OPT_SUBCOMMAND() to one *_fn type

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Nov 05 2022, René Scharfe wrote:

> Am 04.11.22 um 14:22 schrieb Ævar Arnfjörð Bjarmason:
>> When the OPT_SUBCOMMAND() API was implemented in [1] it did so by
>> adding a new "subcommand_fn" member to "struct option", rather than
>> allowing the user of the API to pick the type of the function.
>>
>> An advantage of mandating that "parse_opt_subcommand_fn" must be used
>> is that we'll get type checking for the function we're passing in, a
>> disadvantage is that we can't convert e.g. "builtin/bisect--helper.c"
>> easily to it, as its callbacks need their own argument.
>>
>> Let's generalize this interface, while leaving in place a small hack
>> to give the existing API users their type safety. We assign to
>> "typecheck_subcommand_fn", but don't subsequently use it for
>> anything. Instead we use the "defval" and "value" members.
>>
>> A subsequent commit will add a OPT_SUBCOMMAND() variant where the
>> "callback" isn't our default "parse_options_pick_subcommand" (and that
>> caller won't be able to use the type checking).
>>
>> 1. fa83cc834da (parse-options: add support for parsing subcommands,
>>    2022-08-19)
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  parse-options.c |  9 ++++++---
>>  parse-options.h | 25 +++++++++++++++++++++----
>>  2 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/parse-options.c b/parse-options.c
>> index a1ec932f0f9..1d9e46c9dc7 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -427,7 +427,8 @@ static enum parse_opt_result parse_subcommand(const char *arg,
>>  	for (; options->type != OPTION_END; options++)
>>  		if (options->type == OPTION_SUBCOMMAND &&
>>  		    !strcmp(options->long_name, arg)) {
>> -			*(parse_opt_subcommand_fn **)options->value = options->subcommand_fn;
>> +			if (options->callback(options, arg, 0))
>> +				BUG("OPT_SUBCOMMAND callback returning non-zero");
>>  			return PARSE_OPT_SUBCOMMAND;
>>  		}
>>
>> @@ -506,8 +507,10 @@ static void parse_options_check(const struct option *opts)
>>  			       "That case is not supported yet.");
>>  			break;
>>  		case OPTION_SUBCOMMAND:
>> -			if (!opts->value || !opts->subcommand_fn)
>> -				optbug(opts, "OPTION_SUBCOMMAND needs a value and a subcommand function");
>> +			if (!opts->value || !opts->callback)
>> +				optbug(opts, "OPTION_SUBCOMMAND needs a value and a callback function");
>> +			if (opts->ll_callback)
>> +				optbug(opts, "OPTION_SUBCOMMAND uses callback, not ll_callback");
>>  			if (!subcommand_value)
>>  				subcommand_value = opts->value;
>>  			else if (subcommand_value != opts->value)
>> diff --git a/parse-options.h b/parse-options.h
>> index b6ef86e0d15..61e3016c3fc 100644
>> --- a/parse-options.h
>> +++ b/parse-options.h
>> @@ -128,19 +128,24 @@ typedef int parse_opt_subcommand_fn(int argc, const char **argv,
>>   *			 the option takes optional argument.
>>   *
>>   * `callback`::
>> - *   pointer to the callback to use for OPTION_CALLBACK
>> + *   pointer to the callback to use for OPTION_CALLBACK and OPTION_SUBCOMMAND.
>>   *
>>   * `defval`::
>>   *   default value to fill (*->value) with for PARSE_OPT_OPTARG.
>>   *   OPTION_{BIT,SET_INT} store the {mask,integer} to put in the value when met.
>> + *   OPTION_SUBCOMMAND stores the pointer the function selected for
>> + *   the subcommand.
>> + *
>>   *   CALLBACKS can use it like they want.
>>   *
>>   * `ll_callback`::
>>   *   pointer to the callback to use for OPTION_LOWLEVEL_CALLBACK
>>   *
>>   * `subcommand_fn`::
>> - *   pointer to a function to use for OPTION_SUBCOMMAND.
>> - *   It will be put in value when the subcommand is given on the command line.
>> + *   pointer to the callback used with OPT_SUBCOMMAND() and
>> + *   OPT_SUBCOMMAND_F(). Internally we store the same value in
>> + *   `defval`. This is only here to give the OPT_SUBCOMMAND{,_F}()
>> + *   common case type safety.
>>   */
>>  struct option {
>>  	enum parse_opt_type type;
>> @@ -217,12 +222,24 @@ struct option {
>>  #define OPT_ALIAS(s, l, source_long_name) \
>>  	{ OPTION_ALIAS, (s), (l), (source_long_name) }
>>
>> +static inline int parse_options_pick_subcommand_cb(const struct option *option,
>> +						   const char *arg UNUSED,
>> +						   int unset UNUSED)
>> +{
>> +	parse_opt_subcommand_fn *fn = (parse_opt_subcommand_fn *)option->defval;
>> +	*(parse_opt_subcommand_fn **)option->value = fn;
>
> ->defval is of type intptr_t and ->value is a void pointer.  The result
> of converting a void pointer value to an intptr_t and back is a void
> pointer equal to the original pointer if I read 6.3.2.3 (Pointers,
> paragraphs 5 and 6) and 7.18.1.4 (Integer types capable of holding
> object pointers) in C99 correctly.
>
> 6.3.2.3 paragraph 8 says that casting between function pointers of
> different type is OK and you can get your original function pointer
> back and use it in a call if you convert it back to the right type.
>
> Casting between a function pointer and an object pointer is undefined,
> though.  They don't have to be of the same size, so a function pointer
> doesn't have to fit into an intptr_t.  I wouldn't be surprised if CHERI
> (https://www.cl.cam.ac.uk/research/security/ctsrd/cheri/) was an actual
> example of that.

I should have called this out explicitly. I think you're right as far as
what you're summarizing goes.

To elaborate on it, paragraph 8 of 6.3.2.3 says:

	A pointer to a function of one type may be converted to a
	pointer to a function of another type and back again; the result
	shall compare equal to the original pointer. If a converted
	pointer is used to call a function whose type is not compatible
	with the pointed-to type, the behavior is undefined.

And 7.18.1.4 says, when discussing (among other things) "intptr_t"
("[such" added for clarity:

	[...]any valid [such] pointer to void can be converted to this
	type, then converted back to pointer to void, and the result
	will compare equal to the original pointer:

But as you point out that doesn't say anything about whether a pointer
to a function is a "valid .. pointer to void".

I think that's an "unportable" extension covered in "J.5 Common
extensions", specifically "J.5.7 Function pointer casts":

	A pointer to an object or to void may be cast to a pointer to a
	function, allowing data to be invoked as a function

Thus, since the standard already establishes that valid "void *" and
"intptr_t" pointers can be cast'd back & forth, the J.5.7 bridges the
gap between the two saying a function pointer can be converted to
either.

Now, I may be missing something here, but I was under the impression
that "intptr_t" wasn't special in any way here, and that any casting of
a function pointer to either it or a "void *" was what was made portable
by "J.5.7". We're only discussing "intptr_t" here, so it's just a point
of clarification.

Anyway, like ssize_t and a few other things this is extended upon and
made standard by POSIX. I.e. we're basically talking about whether this
passes:

	assert(sizeof(void (*)(void)) == sizeof(void*))

And per POSIX
(https://pubs.opengroup.org/onlinepubs/9699919799/functions/dlsym.html):

	Note that conversion from a void * pointer to a function pointer
	as in:

		fptr = (int (*)(int))dlsym(handle, "my_function");

	is not defined by the ISO C standard. This standard requires
	this conversion to work correctly on conforming implementations.

So I think aside from other concerns this should be safe to use, as
real-world data backing that up we've had a intptr_t converted to a
function pointer since v2.35.0: 5cb28270a1f (pack-objects: lazily set up
"struct rev_info", don't leak, 2022-03-28).

> Why is this trickery needed?  Above you write that callbacks in
> builtin/bisect--helper.c can't use subcommand_fn because they need
> their own argument.  Can we extend subcommand_fn or use a global
> variable to pass that extra thing instead?  The latter may be ugly, but
> at least it's valid C..

Yeah, there's ways around it. Less uglier in this case would probably be
to just have the callback set a function pointer in your own custom
struct (which we'd point to with "defval).

I.e. if our callabck is the one to populate the "fn" even without J.5.7
there's no portability issue, and that's just a convenience.

>> +	return 0;
>> +}
>> +
>>  #define OPT_SUBCOMMAND_F(l, v, fn, f) { \
>>  	.type = OPTION_SUBCOMMAND, \
>>  	.long_name = (l), \
>>  	.value = (v), \
>>  	.flags = (f), \
>> -	.subcommand_fn = (fn) }
>> +	.defval = (intptr_t)(fn), \
>> +	.subcommand_fn = (fn), \
>> +	.callback = parse_options_pick_subcommand_cb, \
>
> Getting the address of an inline function feels weird, but the compiler
> is free to emit to ignore that keyword and will provide an addressable
> function object here.

*nod*, I thought about adding this to parse-options-cb.c, but it seemed
 small enough...