{"thread":{"id":"61607","subject":"[PATCH 0/4] varargs functions with __attributes__(())","startedAt":"2024-06-08T18:37:55Z","lastAt":"2024-06-11T15:17:11Z","messageCount":9,"participants":["Junio C Hamano","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"496714","messageId":"20240608183747.2084294-1-gitster@pobox.com","threadId":"61607","inReplyTo":null,"subject":"[PATCH 0/4] varargs functions with __attributes__(())","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-08T18:37:43Z","receivedAt":"2024-06-08T18:37:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"There are several varargs functions that take either NULL-terminated\nlist of parameters, or printf-like format followed by list of\nparameters, that are not declared as such with __attributes__(()).\n\nAdding such a missing attribute to trace2_region_enter_printf()\nrevealed that an existing call to it was trying to format a value of\ntype size_t using \"%d\", which is not such an excellent idea. Other\nfunctions that were lacking attributes fortunately did not have any\nbroken existing callers.\n\nJunio C Hamano (4):\n __attribute__: trace2_region_enter_printf() is like \"printf\"\n __attribute__: remove redundant __attribute__ declaration for\n git_die_config()\n __attribute__: mark some functions with LAST_ARG_MUST_BE_NULL\n __attribute__: add a few missing format attributes\n\n add-patch.c | 1 +\n attr.h | 2 ++\n config.c | 1 -\n hook.h | 1 +\n mem-pool.h | 1 +\n run-command.c | 3 ++-\n scalar.c | 2 ++\n trace2.h | 1 +\n wt-status.c | 1 +\n 9 files changed, 11 insertions(+), 2 deletions(-)\n\n-- \n2.45.2-445-g1b76f06508\n\n"},{"id":"496715","messageId":"20240608183747.2084294-2-gitster@pobox.com","threadId":"61607","inReplyTo":"20240608183747.2084294-1-gitster@pobox.com","subject":"[PATCH 1/4] __attribute__: trace2_region_enter_printf() is like \"printf\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-08T18:37:44Z","receivedAt":"2024-06-08T18:37:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The last part of the parameter list the function takes is like\nparameters to printf. mark it as such.\n\nAn existing call that formats a value of type size_t using \"%d\" was\nfound by the compiler with the help with this annotation; fix it.\n\nSigned-off-by: Junio C Hamano \n---\n run-command.c | 3 ++-\n trace2.h | 1 +\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1b821042b4..ec2c12e98b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1753,7 +1753,8 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)\n \n \tif (do_trace2)\n \t\ttrace2_region_enter_printf(tr2_category, tr2_label, NULL,\n-\t\t\t\t\t \"max:%d\", opts->processes);\n+\t\t\t\t\t \"max:%\"PRIuMAX,\n+\t\t\t\t\t (uintmax_t)opts->processes);\n \n \tpp_init(&pp, opts, &pp_sig);\n \twhile (1) {\ndiff --git a/trace2.h b/trace2.h\nindex 1f0669bbd2..19e04bf040 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -390,6 +390,7 @@ void trace2_region_enter_printf_va_fl(const char *file, int line,\n \ttrace2_region_enter_printf_va_fl(__FILE__, __LINE__, (category), \\\n \t\t\t\t\t (label), (repo), (fmt), (ap))\n \n+__attribute__((format (printf, 6, 7)))\n void trace2_region_enter_printf_fl(const char *file, int line,\n \t\t\t\t const char *category, const char *label,\n \t\t\t\t const struct repository *repo,\n-- \n2.45.2-445-g1b76f06508\n\n"},{"id":"496716","messageId":"20240608183747.2084294-3-gitster@pobox.com","threadId":"61607","inReplyTo":"20240608183747.2084294-1-gitster@pobox.com","subject":"[PATCH 2/4] __attribute__: remove redundant attribute declaration for git_die_config()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-08T18:37:45Z","receivedAt":"2024-06-08T18:38:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The convention is to declare the function attribute to an extern\nfunction together with its declaration in the header file, without\nrepeating the attribute declaration with its definition in the .c\nsource file (a file-scope static function declares its attribute\ntogether with its definition in the .c file it is defined, as there\nis no other place to do so).\n\nThe definition of git_die_config() in config.c did not follow the\nconvention and had its attribute declared with both its declaration\nin the header and its definition in the .c source file.\n\nRemove the one in the config.c to match everybody else.\n\nSigned-off-by: Junio C Hamano \n---\n config.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex abce05b774..ea355f28ec 100644\n--- a/config.c\n+++ b/config.c\n@@ -2844,7 +2844,6 @@ void git_die_config_linenr(const char *key, const char *filename, int linenr)\n \t\t key, filename, linenr);\n }\n \n-NORETURN __attribute__((format(printf, 2, 3)))\n void git_die_config(const char *key, const char *err, ...)\n {\n \tconst struct string_list *values;\n-- \n2.45.2-445-g1b76f06508\n\n"},{"id":"496717","messageId":"20240608183747.2084294-4-gitster@pobox.com","threadId":"61607","inReplyTo":"20240608183747.2084294-1-gitster@pobox.com","subject":"[PATCH 3/4] __attribute__: mark some functions with LAST_ARG_MUST_BE_NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-08T18:37:46Z","receivedAt":"2024-06-08T18:38:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Some varargs functions that use NULL-terminated parameter list were\nmissing __attributes__ ((sentinel)) aka LAST_ARG_MUST_BE_NULL.\n\nAdd them.\n\nSigned-off-by: Junio C Hamano \n---\n add-patch.c | 1 +\n attr.h | 2 ++\n hook.h | 1 +\n scalar.c | 1 +\n 4 files changed, 5 insertions(+)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 814de57c4a..d8ea05ff10 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -299,6 +299,7 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n \tva_end(args);\n }\n \n+LAST_ARG_MUST_BE_NULL\n static void setup_child_process(struct add_p_state *s,\n \t\t\t\tstruct child_process *cp, ...)\n {\ndiff --git a/attr.h b/attr.h\nindex e7cc318b0c..bb33b60880 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -190,6 +190,8 @@ struct attr_check {\n };\n \n struct attr_check *attr_check_alloc(void);\n+\n+LAST_ARG_MUST_BE_NULL\n struct attr_check *attr_check_initl(const char *, ...);\n struct attr_check *attr_check_dup(const struct attr_check *check);\n \ndiff --git a/hook.h b/hook.h\nindex 19ab9a5806..6511525aeb 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -86,5 +86,6 @@ int run_hooks(const char *hook_name);\n * argument. These things will be used as positional arguments to the\n * hook. This function behaves like the old run_hook_le() API.\n */\n+LAST_ARG_MUST_BE_NULL\n int run_hooks_l(const char *hook_name, ...);\n #endif\ndiff --git a/scalar.c b/scalar.c\nindex 331b91dbdb..62dd77aaec 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -70,6 +70,7 @@ static void setup_enlistment_directory(int argc, const char **argv,\n \tstrbuf_release(&path);\n }\n \n+LAST_ARG_MUST_BE_NULL\n static int run_git(const char *arg, ...)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n-- \n2.45.2-445-g1b76f06508\n\n"},{"id":"496718","messageId":"20240608183747.2084294-5-gitster@pobox.com","threadId":"61607","inReplyTo":"20240608183747.2084294-1-gitster@pobox.com","subject":"[PATCH 4/4] __attribute__: add a few missing format attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-08T18:37:47Z","receivedAt":"2024-06-08T18:38:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A public function mem_pool_strfmt() takes printf like parameters,\nbut is not given an attribute as such. Also a few file-scope static\nfunctions were missing their format attribute.\n\nAdd them.\n\nSigned-off-by: Junio C Hamano \n---\n mem-pool.h | 1 +\n scalar.c | 1 +\n wt-status.c | 1 +\n 3 files changed, 3 insertions(+)\n\ndiff --git a/mem-pool.h b/mem-pool.h\nindex d1c66413ec..321d86a63c 100644\n--- a/mem-pool.h\n+++ b/mem-pool.h\n@@ -50,6 +50,7 @@ char *mem_pool_strndup(struct mem_pool *pool, const char *str, size_t len);\n /*\n * Allocate memory from the memory pool and format a string into it.\n */\n+__attribute__((format (printf, 2, 3)))\n char *mem_pool_strfmt(struct mem_pool *pool, const char *fmt, ...);\n \n /*\ndiff --git a/scalar.c b/scalar.c\nindex 62dd77aaec..a8318078c9 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -289,6 +289,7 @@ static int unregister_dir(void)\n }\n \n /* printf-style interface, expects `=` argument */\n+__attribute__((format (printf, 1, 2)))\n static int set_config(const char *fmt, ...)\n {\n \tstruct strbuf buf = STRBUF_INIT;\ndiff --git a/wt-status.c b/wt-status.c\nindex ff4be071ca..c4dac01999 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -126,6 +126,7 @@ void status_printf(struct wt_status *s, const char *color,\n \tva_end(ap);\n }\n \n+__attribute__((format (printf, 3, 4)))\n static void status_printf_more(struct wt_status *s, const char *color,\n \t\t\t const char *fmt, ...)\n {\n-- \n2.45.2-445-g1b76f06508\n\n"},{"id":"496756","messageId":"ZmakwSVBXbjfRpUi@tanuki","threadId":"61607","inReplyTo":"20240608183747.2084294-2-gitster@pobox.com","subject":"Re: [PATCH 1/4] __attribute__: trace2_region_enter_printf() is like \"printf\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-10T07:01:21Z","receivedAt":"2024-06-10T07:01:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Jun 08, 2024 at 11:37:44AM -0700, Junio C Hamano wrote:\n> The last part of the parameter list the function takes is like\n> parameters to printf. mark it as such.\n\ns/mark/Mark, or convert the dot to a semicolon.\n\n> An existing call that formats a value of type size_t using \"%d\" was\n> found by the compiler with the help with this annotation; fix it.\n\nMakes sense, as do all the other patches in this series.\n\nPatrick\n"},{"id":"496779","messageId":"xmqqy17c4vw7.fsf@gitster.g","threadId":"61607","inReplyTo":"ZmakwSVBXbjfRpUi@tanuki","subject":"Re: [PATCH 1/4] __attribute__: trace2_region_enter_printf() is like \"printf\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-10T16:15:04Z","receivedAt":"2024-06-10T16:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Sat, Jun 08, 2024 at 11:37:44AM -0700, Junio C Hamano wrote:\n>> The last part of the parameter list the function takes is like\n>> parameters to printf. mark it as such.\n>\n> s/mark/Mark, or convert the dot to a semicolon.\n>\n>> An existing call that formats a value of type size_t using \"%d\" was\n>> found by the compiler with the help with this annotation; fix it.\n>\n> Makes sense, as do all the other patches in this series.\n\nThanks, will amend.\n"},{"id":"496843","messageId":"20240611081735.GI3248245@coredump.intra.peff.net","threadId":"61607","inReplyTo":"20240608183747.2084294-1-gitster@pobox.com","subject":"Re: [PATCH 0/4] varargs functions with __attributes__(())","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-11T08:17:35Z","receivedAt":"2024-06-11T08:17:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 08, 2024 at 11:37:43AM -0700, Junio C Hamano wrote:\n\n> There are several varargs functions that take either NULL-terminated\n> list of parameters, or printf-like format followed by list of\n> parameters, that are not declared as such with __attributes__(()).\n> \n> Adding such a missing attribute to trace2_region_enter_printf()\n> revealed that an existing call to it was trying to format a value of\n> type size_t using \"%d\", which is not such an excellent idea. Other\n> functions that were lacking attributes fortunately did not have any\n> broken existing callers.\n\nGreat, I am happy to see these. I assume you found them all by grepping?\n\nI wonder if there is a way to convince the compiler (or coccinelle) to\ncomplain about any varargs function that does not have one of our\nusual annotations. It's possible to have other conventions (e.g., an\n\"int\" up front specifying the number of entries) but in practice I doubt\nwe would ever use one.\n\nStill, I suspect the answer is probably \"no\", there is not an easy way\nto do it.\n\n-Peff\n"},{"id":"496927","messageId":"xmqqcyony0ej.fsf@gitster.g","threadId":"61607","inReplyTo":"20240611081735.GI3248245@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] varargs functions with __attributes__(())","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-11T15:17:08Z","receivedAt":"2024-06-11T15:17:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Jun 08, 2024 at 11:37:43AM -0700, Junio C Hamano wrote:\n>\n>> There are several varargs functions that take either NULL-terminated\n>> list of parameters, or printf-like format followed by list of\n>> parameters, that are not declared as such with __attributes__(()).\n>> \n>> Adding such a missing attribute to trace2_region_enter_printf()\n>> revealed that an existing call to it was trying to format a value of\n>> type size_t using \"%d\", which is not such an excellent idea. Other\n>> functions that were lacking attributes fortunately did not have any\n>> broken existing callers.\n>\n> Great, I am happy to see these. I assume you found them all by grepping?\n>\n> I wonder if there is a way to convince the compiler (or coccinelle) to\n> complain about any varargs function that does not have one of our\n> usual annotations. It's possible to have other conventions (e.g., an\n> \"int\" up front specifying the number of entries) but in practice I doubt\n> we would ever use one.\n>\n> Still, I suspect the answer is probably \"no\", there is not an easy way\n> to do it.\n\nI just went from grepping for fixed \"...)\" in *.[ch] files. I do\nadmit I wished some form of automation, but didn't come up with one.\n\nI was happy that the exercise found a real bug ;-) I started it\nonly as a clean-up topic.\n"}]}