On 05/11/2022 13:52, Ævar Arnfjörð Bjarmason wrote:
On Sat, Nov 05 2022, René Scharfe wrote:
Am 04.11.22 um 14:22 schrieb Ævar Arnfjörð Bjarmason:
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
This is a common extension, it is _not_ guaranteed by the standard and
so still undefined behavior unless your compiler happens to have
implemented that extension.
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.
How does J.5.7 bridge the gap when compilers are not required to
implement it?
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"
How is it made portable by an "unportable" extension?
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).
Saying "it works so it is fine" is not a convincing argument that it is
compliant with the standard. If it is undefined then it may work now but
not in a future compiler.
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..
Unfortunately the current code relies on being able to cast function
pointers to void* so it is not just this patch that is relying on
undefined behavior.
Best Wishes
Phillip
