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

["René Scharfe" <l.s.r@web.de>]

Am 05.11.22 um 23:33 schrieb Ævar Arnfjörð Bjarmason:
>
> On Sat, Nov 05 2022, René Scharfe wrote:
>
>> Am 05.11.22 um 14:52 schrieb Ævar Arnfjörð Bjarmason:
>>>
>>> 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".
>>
>> Do you mean "possible" or "workable" instead of "portable" here?  As you
>> write above, J.5.7 is an extension, not (fully) portable.
>
> I think my just-sent in the side-thread should clarify this.

AFAIU you think that J.5.7 plus POSIX make conversions between object
pointers and function pointers portable.

>>> 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.
>>
>> Conversion from object pointer to function pointer can still work if
>> function pointers are wider.

This means that a compliant implementation could lose bits when going
the other way, i.e. converting a function pointer to an object pointer.

>>> 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).
>>
>> That may not have reached unusual architectures, yet.  Let's replace
>> that cast with something boring before someone gets hurt.  Something
>> like this?

[snip]

> I think "just leave it, and see if anyone complains".
>
> If you look over config.mak.uname you can see what we're likely to be
> ported to (and some of that's probably dead). The list of potential
> targets that:
>
>  1) We know of ports to, or people would plausibly port git to
>  2) Are updated so slow that they're on a release that's getting close
>     to a year old.
>
> Are small, and it's usually easy to look up their memory model etc. are
> you concerned about any specific one?

Using implementation-defined behavior and requiring extensions when
standard code would work just as well makes no sense to me.

> I think if you're worried enough about it to push for the diff above:
> Can we just hide it behind an "#ifdef", then if we find that nobody's
> using it, we can consider it safe to use.
>
> I don't think there's any great benefit to the extension in that
> specific case, but there might be in the future (e.g. this topic would
> be one small user), so since we already have an unintentional test
> ballon, why not see if we can keep it safely?

You can't certify safety with tests.  Undefined behavior may manifest
itself in weird ways and only under certain circumstances.  Future
architectures may add new failure modes.  It's not like a syntax
extension, to which nonsupporting compilers respond with an error,
i.e. a clear signal.

René