Git Mailing List Archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "René Scharfe" <l.s.r@web.de>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Jeff King" <peff@peff.net>, "Taylor Blau" <me@ttaylorr.com>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Lukáš Doktor" <ldoktor@redhat.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 09/13] parse-options API: don't restrict OPT_SUBCOMMAND() to one *_fn type
Date: Sat, 05 Nov 2022 14:52:59 +0100	[thread overview]
Message-ID: <221105.861qqhxz0o.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <df855ba1-52b1-1007-68e8-2e28e85b6822@web.de>


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...

  parent reply	other threads:[~2022-11-05 14:22 UTC|newest]

Thread overview: 106+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-04  6:31 "git bisect run" strips "--log" from the list of arguments Lukáš Doktor
2022-11-04  9:45 ` Jeff King
2022-11-04 11:10   ` Đoàn Trần Công Danh
2022-11-04 12:51     ` Jeff King
2022-11-04 11:36   ` Ævar Arnfjörð Bjarmason
2022-11-04 12:45     ` Jeff King
2022-11-04 13:07       ` Ævar Arnfjörð Bjarmason
2022-11-04 12:37   ` SZEDER Gábor
2022-11-04 12:44     ` Jeff King
2022-11-04 11:40 ` [PATCH 0/3] Convert git-bisect--helper to OPT_SUBCOMMAND Đoàn Trần Công Danh
2022-11-04 11:40   ` [PATCH 1/3] bisect--helper: remove unused options Đoàn Trần Công Danh
2022-11-04 12:53     ` Jeff King
2022-11-04 11:40   ` [PATCH 2/3] bisect--helper: move all subcommands into their own functions Đoàn Trần Công Danh
2022-11-04 12:55     ` Jeff King
2022-11-04 13:32     ` Ævar Arnfjörð Bjarmason
2022-11-04 14:03       ` Đoàn Trần Công Danh
2022-11-04 11:40   ` [PATCH 3/3] bisect--helper: parse subcommand with OPT_SUBCOMMAND Đoàn Trần Công Danh
2022-11-04 13:00     ` Jeff King
2022-11-04 13:46     ` Ævar Arnfjörð Bjarmason
2022-11-04 14:07       ` Đoàn Trần Công Danh
2022-11-04 13:55   ` [PATCH 0/3] Convert git-bisect--helper to OPT_SUBCOMMAND Ævar Arnfjörð Bjarmason
2022-11-05 17:03   ` [PATCH v2 " Đoàn Trần Công Danh
2022-11-05 17:03     ` [PATCH v2 1/3] bisect--helper: remove unused options Đoàn Trần Công Danh
2022-11-05 17:03     ` [PATCH v2 2/3] bisect--helper: move all subcommands into their own functions Đoàn Trần Công Danh
2022-11-05 17:13       ` Đoàn Trần Công Danh
2022-11-05 17:03     ` [PATCH v2 3/3] bisect--helper: parse subcommand with OPT_SUBCOMMAND Đoàn Trần Công Danh
2022-11-05 17:07     ` [PATCH 00/13] Turn git-bisect to be builtin Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 01/13] bisect tests: test for v2.30.0 "bisect run" regressions Đoàn Trần Công Danh
2022-11-07 21:31         ` Ævar Arnfjörð Bjarmason
2022-11-08  1:17           ` Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 02/13] bisect: refactor bisect_run() to match CodingGuidelines Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 03/13] bisect--helper: pass arg[cv] down to do_bisect_run Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 04/13] bisect: fix output regressions in v2.30.0 Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 05/13] bisect run: keep some of the post-v2.30.0 output Đoàn Trần Công Danh
2022-11-07 21:40         ` Ævar Arnfjörð Bjarmason
2022-11-08  1:26           ` Đoàn Trần Công Danh
2022-11-08  3:11             ` Ævar Arnfjörð Bjarmason
2022-11-05 17:07       ` [PATCH 06/13] bisect--helper: remove unused arguments from do_bisect_run Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 07/13] bisect--helper: pretend we're real bisect when report error Đoàn Trần Công Danh
2022-11-07 21:29         ` Ævar Arnfjörð Bjarmason
2022-11-05 17:07       ` [PATCH 08/13] bisect test: test exit codes on bad usage Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 09/13] bisect--helper: emit usage for "git bisect" Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 10/13] bisect--helper: make `state` optional Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 11/13] bisect--helper: remove subcommand state Đoàn Trần Công Danh
2022-11-07 21:45         ` Ævar Arnfjörð Bjarmason
2022-11-08  1:27           ` Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 12/13] bisect--helper: log: allow arbitrary number of arguments Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 13/13] Turn `git bisect` into a full built-in Đoàn Trần Công Danh
2022-11-05 23:18     ` [PATCH v2 0/3] Convert git-bisect--helper to OPT_SUBCOMMAND Taylor Blau
2022-11-10 16:36   ` [PATCH v3 " Đoàn Trần Công Danh
2022-11-10 16:36     ` [PATCH v3 1/3] bisect--helper: remove unused options Đoàn Trần Công Danh
2022-11-11 12:42       ` Ævar Arnfjörð Bjarmason
2022-11-10 16:36     ` [PATCH v3 2/3] bisect--helper: move all subcommands into their own functions Đoàn Trần Công Danh
2022-11-11 13:51       ` Ævar Arnfjörð Bjarmason
2022-11-10 16:36     ` [PATCH v3 3/3] bisect--helper: parse subcommand with OPT_SUBCOMMAND Đoàn Trần Công Danh
2022-11-10 16:36     ` [PATCH v2 00/11] Turn git-bisect to be builtin Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 01/11] bisect tests: test for v2.30.0 "bisect run" regressions Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 02/11] bisect: refactor bisect_run() to match CodingGuidelines Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 03/11] bisect: fix output regressions in v2.30.0 Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 04/11] bisect run: keep some of the post-v2.30.0 output Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 05/11] bisect-run: verify_good: account for non-negative exit status Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 06/11] bisect--helper: identify as bisect when report error Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 07/11] bisect test: test exit codes on bad usage Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 08/11] bisect--helper: emit usage for "git bisect" Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 09/11] bisect--helper: handle states directly Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 10/11] bisect--helper: log: allow arbitrary number of arguments Đoàn Trần Công Danh
2022-11-11 14:01         ` Ævar Arnfjörð Bjarmason
2022-11-10 16:36       ` [PATCH v2 11/11] Turn `git bisect` into a full built-in Đoàn Trần Công Danh
2022-11-11 13:53         ` Ævar Arnfjörð Bjarmason
2022-11-11 15:37           ` Jeff King
2022-11-11 21:09             ` Ævar Arnfjörð Bjarmason
2022-11-11 22:07       ` [PATCH v2 00/11] Turn git-bisect to be builtin Taylor Blau
2022-11-15 19:18         ` Taylor Blau
2022-11-15 19:36           ` Jeff King
2022-11-15 19:40             ` Taylor Blau
2022-11-11 12:32     ` [PATCH v3 0/3] Convert git-bisect--helper to OPT_SUBCOMMAND Ævar Arnfjörð Bjarmason
2022-11-04 13:22 ` [PATCH 00/13] bisect: v2.30.0 "run" regressions + make it built-in Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 01/13] bisect tests: test for v2.30.0 "bisect run" regressions Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 02/13] bisect: refactor bisect_run() to match CodingGuidelines Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 03/13] bisect: fix output regressions in v2.30.0 Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 04/13] bisect run: fix "--log" eating regression " Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 05/13] bisect run: keep some of the post-v2.30.0 output Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 06/13] bisect test: test exit codes on bad usage Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 07/13] bisect--helper: emit usage for "git bisect" Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 08/13] bisect--helper: have all functions take state, argc, argv, prefix Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 09/13] parse-options API: don't restrict OPT_SUBCOMMAND() to one *_fn type Ævar Arnfjörð Bjarmason
2022-11-05  8:32     ` René Scharfe
2022-11-05 11:34       ` Đoàn Trần Công Danh
2022-11-05 21:32         ` Phillip Wood
2022-11-05 13:52       ` Ævar Arnfjörð Bjarmason [this message]
2022-11-05 16:36         ` Phillip Wood
2022-11-05 21:59           ` Ævar Arnfjörð Bjarmason
2022-11-05 17:26         ` René Scharfe
2022-11-05 22:33           ` Ævar Arnfjörð Bjarmason
2022-11-06  8:25             ` René Scharfe
2022-11-06 13:28               ` Ævar Arnfjörð Bjarmason
2022-11-12 10:42                 ` René Scharfe
2022-11-12 16:34                   ` Jeff King
2022-11-12 16:55                     ` Ævar Arnfjörð Bjarmason
2022-11-13 17:31                       ` René Scharfe
2022-11-04 13:22   ` [PATCH 10/13] bisect--helper: remove dead --bisect-{next-check,autostart} code Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 11/13] bisect--helper: convert to OPT_SUBCOMMAND_CB() Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 12/13] bisect--helper: make `state` optional Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 13/13] Turn `git bisect` into a full built-in Ævar Arnfjörð Bjarmason
2022-11-05  0:13   ` [PATCH 00/13] bisect: v2.30.0 "run" regressions + make it built-in Taylor Blau
2022-11-10 12:50   ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=221105.861qqhxz0o.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=congdanhqx@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --cc=ldoktor@redhat.com \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).