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

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

Am 12.11.22 um 17:55 schrieb Ævar Arnfjörð Bjarmason:
>
> On Sat, Nov 12 2022, Jeff King wrote:
>
>> On Sat, Nov 12, 2022 at 11:42:09AM +0100, René Scharfe wrote:
>>
>>>> E.g. we have plenty of code that assumes ASCII, instead of catering to
>>>> EBCDIC, and assuming NULL is (void *)0, not (void *)123456 or whatever.
>>>
>>> NULL is defined as "0" or "(void *)0" by C99 6.3.2.3 Pointers paragraph
>>> 3 and 7.17 Common definitions <stddef.h> paragraph 3.
>>
>> I think he is alluding to the fact that while the standard requires that
>> a "0" constant refers to a NULL pointer, the representation does not
>> have to be all-bits-zero. So:
>>
>>   char *foo = 0;
>>
>> is fine, but:
>>
>>   char *foo;
>>   memset(foo, 0, sizeof(&foo));
>
> Yes, to elaborate: the "null pointer constant" referred to in 6.3.2.3
> deliberately leaves room for the representation being unequal to the
> "all zero bits". And as you point out the former example is portable,
> but not the latter.
>
>> is not. And we absolutely do the latter in our code base anyway, because
>> it's convenient and unlikely to be a problem on practical platforms. And
>> I think it has always been our attitude in this community to let
>> engineering practicality trump strict adherence to the standard. But
>> "practicality" there should be measuring the tradeoff of how useful
>> something is versus how likely it is to bite us.

For me the usefulness so far is negative: The code is more complicated
than necessary.  Adding a context pointer to the callback function
signature here and keeping the extra code outside the callback function
in builtin/pack-objects.c is simpler.

> All I've been trying to get across in this sub-thread is that there's an
> interesting empirical question here: Are we in fact targeting an
> architecture where J.5.7 isn't implemented, or likely to have one sneak
> up on us?

How would you measure this?  Undefined behavior can manifest itself
differently e.g. based on compiler version and options, or in this
case pointer value and perhaps even function calling convention.  And
of course in the form of the famous nasal demons..

> I don't think so, and timing-wise deciding to be paranoid about this
> particular thing would leave that question unanswered, when all we have
> to do is wait a bit (some of the slower platforms tend to be a few
> releases behind).
>
> The argument for the change[1] (further articulated upthread) hasn't
> answered the "do we target such an arch?", but seems to just fall back
> to general standards paranoia.

I mentioned CHERI (Arm Morello) as a candidate, but can't tell you for
sure.

> Which isn't an invalid argument in itself. But doesn't really address
> why we'd be worried about *this* particular thing, but not e.g. those
> sort of memsets, assuming ASCII ordering for 'A'..'z' etc.

You can keep worrying about them if you like.  Replacing memset calls
with _INIT macros has been going on for while already.  Using isalpha()
instead of character range comparisons etc. is probably a good idea
anyway.

>> In the case under discussion, my gut feeling agrees with you, though.
>> I'm skeptical that equivalence of object and function pointers is all
>> that useful in practice. And your mention of CHERI seems like a
>> plausible way it could bite us.
>
> I think the post-image of [1] looks nicer when reviewed stand-alone, so
> I'm not against the change per-se, I actually like it.
>
> And I don't have a use-case for using that feature further, in a way
> that isn't easy to do differently.
>
> But e.g. now we're having a parallel discussion about using some 3rd
> party bitmap library. We might e.g. want to incorporate some 3rd party
> JIT or whatever in the future. If we run into this question again it
> would be nice to have it answered already.

Why would a bitmap library require function pointer casts?

A JIT library probably comes with a list of supported systems and
requires a fallback for anyone else.  I'd expect the system-specific
parts to be encapsulated in that library.

> And if we didn't have this J.5.7 reliance in that code already I don't
> think it would be worth the effort to introduce one as a test
> balloon. I'm only saying this in the context that we already have one.
>
> 1. https://lore.kernel.org/git/c64e4fa5-62c2-2a93-a4ef-bd84407ea570@web.de/

If it's not worth adding then it's probably not worth keeping.

René