{"thread":{"id":"60130","subject":"[PATCH 1/2] fsck: use enum object_type for fsck_walk callback","startedAt":"2023-08-20T00:35:36Z","lastAt":"2023-08-20T08:46:01Z","messageCount":4,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"480808","messageId":"20230819235342.GA600744@coredump.intra.peff.net","threadId":"60130","inReplyTo":"20230819235123.GA600613@coredump.intra.peff.net","subject":"[PATCH 1/2] fsck: use enum object_type for fsck_walk callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-19T23:53:42Z","receivedAt":"2023-08-20T00:35:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We switched the function interface for fsck callbacks in a1aad71601\n(fsck.h: use \"enum object_type\" instead of \"int\", 2021-03-28). However,\nwe accidentally flipped the type back to \"int\" as part of 0b4e9013f1\n(fsck: mark unused parameters in various fsck callbacks, 2023-07-03).\nThe mistake happened because that commit was written before a1aad71601\nand rebased forward, and I screwed up while resolving the conflict.\n\nCuriously, the compiler does not warn about this mismatch, at least not\nwhen using gcc and clang on Linux (nor in any of our CI environments).\nBased on 28abf260a5 (builtin/fsck.c: don't conflate \"int\" and \"enum\" in\ncallback, 2021-06-01), I'd guess that this would cause the AIX xlc\ncompiler to complain. I noticed because clang-18's UBSan now identifies\nmis-matched function calls at runtime, and does complain of this case\nwhen running the test suite.\n\nI'm not entirely clear on whether this mismatch is a problem in\npractice. Compilers are certainly free to make enums smaller than \"int\"\nif they don't need the bits, but I suspect that they have to promote\nback to int for function calls (though I didn't dig in the standard, and\nI won't be surprised if I'm simply wrong and the real-world impact would\ndepend on the ABI).\n\nRegardless, switching it back to enum is obviously the right thing to do\nhere; the switch to \"int\" was simply a mistake.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fsck.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex c1d0290026..611925905e 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -206,7 +206,7 @@ static int traverse_reachable(void)\n \treturn !!result;\n }\n \n-static int mark_used(struct object *obj, int type UNUSED,\n+static int mark_used(struct object *obj, enum object_type type UNUSED,\n \t\t     void *data UNUSED, struct fsck_options *options UNUSED)\n {\n \tif (!obj)\n-- \n2.42.0.rc2.418.g602d5859fc\n\n"},{"id":"480809","messageId":"20230819235123.GA600613@coredump.intra.peff.net","threadId":"60130","inReplyTo":null,"subject":"[PATCH 0/2] fixing a few function pointer mismatches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-19T23:51:23Z","receivedAt":"2023-08-20T00:35:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I saw that clang-18 hit Debian unstable, so I did my usual \"build and\nsee if it finds anything new to complain about\". Here's the result. :)\n\nNote that that first one is a regression in the upcoming v2.42, though I\nsuspect the fallout would not be very widespread (see comments in the\ncommit message).\n\n  [1/2]: fsck: use enum object_type for fsck_walk callback\n  [2/2]: hashmap: use expected signatures for comparison functions\n\n builtin/fsck.c    |  2 +-\n compat/terminal.c | 10 ++++++----\n range-diff.c      | 11 +++++++----\n 3 files changed, 14 insertions(+), 9 deletions(-)\n\n-Peff\n"},{"id":"480810","messageId":"20230819235530.GB600744@coredump.intra.peff.net","threadId":"60130","inReplyTo":"20230819235123.GA600613@coredump.intra.peff.net","subject":"[PATCH 2/2] hashmap: use expected signatures for comparison functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-19T23:55:30Z","receivedAt":"2023-08-20T00:36:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We prefer for callback functions to match the signature with which\nthey'll be called, rather than casting them to the correct type when\nassigning function pointers. Even though casting often works in the real\nworld, it is a violation of the standard.\n\nWe did a mass conversion in 939af16eac (hashmap_cmp_fn takes\nhashmap_entry params, 2019-10-06), but have grown a few new cases since\nthen. Because of the cast, the compiler does not complain. However, as\nof clang-18, UBSan will catch these at run-time, and the case in\nrange-diff.c triggers when running t3206.\n\nAfter seeing that one, I scanned the results of:\n\n  git grep '_fn)[^(]' '*.c' | grep -v typedef\n\nand found a similar case in compat/terminal.c (which presumably isn't\ncalled in the test suite, since it doesn't trigger UBSan). There might\nbe other cases lurking if the cast is done using a typedef that doesn't\nend in \"_fn\", but loosening it finds too many false positives. I also\nlooked for:\n\n  git grep ' = ([a-z_]*) *[a-z]' '*.c'\n\nto find assignments that cast, but nothing looked like a function.\n\nThe resulting code is unfortunately a little longer, but the bonus of\nusing container_of() is that we are no longer restricted to the\nhashmap_entry being at the start of the struct.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n compat/terminal.c | 10 ++++++----\n range-diff.c      | 11 +++++++----\n 2 files changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 83d95e8656..0afda730f2 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -479,10 +479,13 @@ struct escape_sequence_entry {\n };\n \n static int sequence_entry_cmp(const void *hashmap_cmp_fn_data UNUSED,\n-\t\t\t      const struct escape_sequence_entry *e1,\n-\t\t\t      const struct escape_sequence_entry *e2,\n+\t\t\t      const struct hashmap_entry *he1,\n+\t\t\t      const struct hashmap_entry *he2,\n \t\t\t      const void *keydata)\n {\n+\tconst struct escape_sequence_entry\n+\t\t*e1 = container_of(he1, const struct escape_sequence_entry, entry),\n+\t\t*e2 = container_of(he2, const struct escape_sequence_entry, entry);\n \treturn strcmp(e1->sequence, keydata ? keydata : e2->sequence);\n }\n \n@@ -496,8 +499,7 @@ static int is_known_escape_sequence(const char *sequence)\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tchar *p, *eol;\n \n-\t\thashmap_init(&sequences, (hashmap_cmp_fn)sequence_entry_cmp,\n-\t\t\t     NULL, 0);\n+\t\thashmap_init(&sequences, sequence_entry_cmp, NULL, 0);\n \n \t\tstrvec_pushl(&cp.args, \"infocmp\", \"-L\", \"-1\", NULL);\n \t\tif (pipe_command(&cp, NULL, 0, &buf, 0, NULL, 0))\ndiff --git a/range-diff.c b/range-diff.c\nindex 2e86063491..ca5493984a 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -230,16 +230,19 @@ static int read_patches(const char *range, struct string_list *list,\n }\n \n static int patch_util_cmp(const void *cmp_data UNUSED,\n-\t\t\t  const struct patch_util *a,\n-\t\t\t  const struct patch_util *b,\n-\t\t\t  const char *keydata)\n+\t\t\t  const struct hashmap_entry *ha,\n+\t\t\t  const struct hashmap_entry *hb,\n+\t\t\t  const void *keydata)\n {\n+\tconst struct patch_util\n+\t\t*a = container_of(ha, const struct patch_util, e),\n+\t\t*b = container_of(hb, const struct patch_util, e);\n \treturn strcmp(a->diff, keydata ? keydata : b->diff);\n }\n \n static void find_exact_matches(struct string_list *a, struct string_list *b)\n {\n-\tstruct hashmap map = HASHMAP_INIT((hashmap_cmp_fn)patch_util_cmp, NULL);\n+\tstruct hashmap map = HASHMAP_INIT(patch_util_cmp, NULL);\n \tint i;\n \n \t/* First, add the patches of a to a hash map */\n-- \n2.42.0.rc2.418.g602d5859fc\n"},{"id":"480812","messageId":"xmqqwmxqru7n.fsf@gitster.g","threadId":"60130","inReplyTo":"20230819235123.GA600613@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] fixing a few function pointer mismatches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-20T08:31:08Z","receivedAt":"2023-08-20T08:46:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I saw that clang-18 hit Debian unstable, so I did my usual \"build and\n> see if it finds anything new to complain about\". Here's the result. :)\n>\n> Note that that first one is a regression in the upcoming v2.42, though I\n> suspect the fallout would not be very widespread (see comments in the\n> commit message).\n\nBoth changes do look innocuous.  Queued.\n\nThanks.\n"}]}