{"thread":{"id":"49603","subject":"[PATCH] config.mak.dev: enable -Wunused-function","startedAt":"2018-10-18T07:05:26Z","lastAt":"2018-10-27T08:17:28Z","messageCount":13,"participants":["Jeff King","Duy Nguyen","Ramsay Jones","Nguyễn Thái Ngọc Duy","Junio C Hamano","Ben Peart"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"360823","messageId":"20181018070522.GA29499@sigill.intra.peff.net","threadId":"49603","inReplyTo":null,"subject":"[PATCH] config.mak.dev: enable -Wunused-function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-18T07:05:22Z","receivedAt":"2018-10-18T07:05:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We explicitly omitted -Wunused-function when we added\n-Wextra, but there is no need: the current code compiles\ncleanly with it. And it's worth having, since it can let you\nknow when there are cascading effects from a cleanup (e.g.,\ndeleting one function lets you delete its static helpers).\n\nThere are cases where we may need an unused function to\nexist, but we can handle these easily:\n\n - macro-generated code like commit-slab; there we have the\n MAYBE_UNUSED annotation to silence the compiler\n\n - conditional compilation, where we may or may not need a\n static helper. These generally fall into one of two\n categories:\n\n - the call should not be conditional, but rather the\n\tfunction body itself should be (and may just be a\n\tno-op on one side of the #if). That keeps the\n\tconditional pollution out of the main code.\n\n - call-chains of static helpers should all be in the\n same #if block, so they are all-or-nothing\n\n And if there's some case that doesn't cover, we still\n have MAYBE_UNUSED as a fallback.\n\nSigned-off-by: Jeff King \n---\n config.mak.dev | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 92d268137f..bbeeff44fe 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -34,7 +34,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),)\n CFLAGS += -Wno-empty-body\n CFLAGS += -Wno-missing-field-initializers\n CFLAGS += -Wno-sign-compare\n-CFLAGS += -Wno-unused-function\n CFLAGS += -Wno-unused-parameter\n endif\n endif\n-- \n2.19.1.823.g84a1dd67bf\n"},{"id":"360824","messageId":"20181018070831.GA29542@sigill.intra.peff.net","threadId":"49603","inReplyTo":"20181018070522.GA29499@sigill.intra.peff.net","subject":"Re: [PATCH] config.mak.dev: enable -Wunused-function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-18T07:08:32Z","receivedAt":"2018-10-18T07:08:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 18, 2018 at 03:05:22AM -0400, Jeff King wrote:\n\n> diff --git a/config.mak.dev b/config.mak.dev\n> index 92d268137f..bbeeff44fe 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -34,7 +34,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),)\n> CFLAGS += -Wno-empty-body\n> CFLAGS += -Wno-missing-field-initializers\n> CFLAGS += -Wno-sign-compare\n> -CFLAGS += -Wno-unused-function\n> CFLAGS += -Wno-unused-parameter\n\nBy the way, I wondered how close we were to being able to use\n-Wunused-parameter. The answer is \"not very\".\n\nHowever, I've been digging into the results, and it does find a number\nof bugs. I'm 168 (rough) patches deep right now, and I have it compiling\ncleanly. Most of those are just annotations, but I'll start posting\nfixes as I organize and clean them up.\n\n-Peff\n"},{"id":"360849","messageId":"CACsJy8BOL+QOb3Vdh8RKbra-DDbJVyznTbCJgjK5h8L2y0H7yg@mail.gmail.com","threadId":"49603","inReplyTo":"20181018070522.GA29499@sigill.intra.peff.net","subject":"Re: [PATCH] config.mak.dev: enable -Wunused-function","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-18T15:48:16Z","receivedAt":"2018-10-18T15:48:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 18, 2018 at 9:05 AM Jeff King wrote:\n>\n> We explicitly omitted -Wunused-function when we added\n> -Wextra, but there is no need: the current code compiles\n> cleanly with it. And it's worth having, since it can let you\n> know when there are cascading effects from a cleanup (e.g.,\n> deleting one function lets you delete its static helpers).\n>\n> There are cases where we may need an unused function to\n> exist, but we can handle these easily:\n>\n> - macro-generated code like commit-slab; there we have the\n> MAYBE_UNUSED annotation to silence the compiler\n>\n> - conditional compilation, where we may or may not need a\n> static helper. These generally fall into one of two\n> categories:\n>\n> - the call should not be conditional, but rather the\n> function body itself should be (and may just be a\n> no-op on one side of the #if). That keeps the\n> conditional pollution out of the main code.\n>\n> - call-chains of static helpers should all be in the\n> same #if block, so they are all-or-nothing\n\nGrouping is not always desired because it could break better function\nlayout. Have a look at read-cache.c where _ieot_extension functions\nare #if'd because the only call sites are from pthread code (#if'd far\naway).\n\nIn this particular case though I think we should be able to avoid so\nmuch #if if we make a wrapper for pthread api that would return an\nerror or something when pthread is not available. But similar\nsituation may happen elsewhere too.\n\nHaving said that, if people do consider MAYBE_UNUSED before #if'ing\neverywhere (and opening up more conditional build problems in future),\nI think this change is fine.\n-- \nDuy\n"},{"id":"360851","messageId":"3295c759-473b-f279-5443-f915fd0b1061@ramsayjones.plus.com","threadId":"49603","inReplyTo":"20181018070522.GA29499@sigill.intra.peff.net","subject":"Re: [PATCH] config.mak.dev: enable -Wunused-function","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-10-18T17:01:44Z","receivedAt":"2018-10-18T17:01:49Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 18/10/2018 08:05, Jeff King wrote:\n> We explicitly omitted -Wunused-function when we added\n> -Wextra, but there is no need: the current code compiles\n> cleanly with it. And it's worth having, since it can let you\n> know when there are cascading effects from a cleanup (e.g.,\n> deleting one function lets you delete its static helpers).\n> \n> There are cases where we may need an unused function to\n> exist, but we can handle these easily:\n> \n> - macro-generated code like commit-slab; there we have the\n> MAYBE_UNUSED annotation to silence the compiler\n> \n> - conditional compilation, where we may or may not need a\n> static helper. These generally fall into one of two\n> categories:\n> \n> - the call should not be conditional, but rather the\n> \tfunction body itself should be (and may just be a\n> \tno-op on one side of the #if). That keeps the\n> \tconditional pollution out of the main code.\n> \n> - call-chains of static helpers should all be in the\n> same #if block, so they are all-or-nothing\n> \n> And if there's some case that doesn't cover, we still\n> have MAYBE_UNUSED as a fallback.\n> \n> Signed-off-by: Jeff King \n> ---\n> config.mak.dev | 1 -\n> 1 file changed, 1 deletion(-)\n> \n> diff --git a/config.mak.dev b/config.mak.dev\n> index 92d268137f..bbeeff44fe 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -34,7 +34,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),)\n> CFLAGS += -Wno-empty-body\n> CFLAGS += -Wno-missing-field-initializers\n> CFLAGS += -Wno-sign-compare\n> -CFLAGS += -Wno-unused-function\n> CFLAGS += -Wno-unused-parameter\n> endif\n> endif\n> \n\nHeh, as luck would have it, tonight I had an -Wunused-function\nwarning on cygwin, but not Linux, when building the 'pu' branch.\n\n[On linux, I am using DEVELOPER=1 in my config.mak etc., whereas\non cygwin I am still using an explicit (but very similar) list\nof -W args.]\n\nI haven't looked too deeply, but this seems to be caused by\nJunio's commit 42c89ea70a (\"SQUASH??? - convert the other user of\nstring-list as db\", 2018-10-17) which removes a call to the\nadd_existing() function - the subject of the warning.\n\n[BTW there is another 'static add_existing()' in builtin/show_ref.c]\n\nATB,\nRamsay Jones\n\n"},{"id":"360852","messageId":"20181018170934.GA21138@sigill.intra.peff.net","threadId":"49603","inReplyTo":"CACsJy8BOL+QOb3Vdh8RKbra-DDbJVyznTbCJgjK5h8L2y0H7yg@mail.gmail.com","subject":"Re: [PATCH] config.mak.dev: enable -Wunused-function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-18T17:09:34Z","receivedAt":"2018-10-18T17:09:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 18, 2018 at 05:48:16PM +0200, Duy Nguyen wrote:\n\n> > - conditional compilation, where we may or may not need a\n> > static helper. These generally fall into one of two\n> > categories:\n> >\n> > - the call should not be conditional, but rather the\n> > function body itself should be (and may just be a\n> > no-op on one side of the #if). That keeps the\n> > conditional pollution out of the main code.\n> >\n> > - call-chains of static helpers should all be in the\n> > same #if block, so they are all-or-nothing\n> \n> Grouping is not always desired because it could break better function\n> layout. Have a look at read-cache.c where _ieot_extension functions\n> are #if'd because the only call sites are from pthread code (#if'd far\n> away).\n\nTrue, though as long as they are triggered by the same set of #if\nconditions, that is fine. Putting them in the same block is just an\neasy way to make sure that is the case. ;)\n\n> In this particular case though I think we should be able to avoid so\n> much #if if we make a wrapper for pthread api that would return an\n> error or something when pthread is not available. But similar\n> situation may happen elsewhere too.\n\nYeah, I think that is generally the preferred method anyway, just\nbecause of readability and simplicity.\n\n> Having said that, if people do consider MAYBE_UNUSED before #if'ing\n> everywhere (and opening up more conditional build problems in future),\n> I think this change is fine.\n\nI'd like to use it as a last resort, certainly. Mostly the fact that we\ncompile cleanly _now_ makes me think that it probably won't be that hard\nto keep it going.\n\nI think the biggest potential problem with this is going to be obscure\nconfigurations where some functions are used or not used. So somebody\nsilencing a compiler warning may inadvertently break another case if\nthey're not careful. But that's already a problem to some degree (and\npart of why we try to push the conditionality out to the whole-function\nlevel).\n\n-Peff\n"},{"id":"360856","messageId":"20181018180522.17642-1-pclouds@gmail.com","threadId":"49603","inReplyTo":"20181018170934.GA21138@sigill.intra.peff.net","subject":"[PATCH/RFC] thread-utils: better wrapper to avoid #ifdef NO_PTHREADS","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-18T18:05:22Z","receivedAt":"2018-10-18T18:05:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 18, 2018 at 7:09 PM Jeff King wrote:\n> > In this particular case though I think we should be able to avoid so\n> > much #if if we make a wrapper for pthread api that would return an\n> > error or something when pthread is not available. But similar\n> > situation may happen elsewhere too.\n>\n> Yeah, I think that is generally the preferred method anyway, just\n> because of readability and simplicity.\n\nI've wanted to do this for a while, so let's test the water and see if\nit's well received.\n\nThis patch is a proof of concept that adds just enough macros so that\nI can build index-pack.c on a single thread mode with zero #ifdef\nrelated to NO_PTHREADS.\n\nBesides readability and simplicity, it reduces the chances of breaking\nconditional builds (e.g. you rename a variable name but forgot that\nthe variable is in #if block that is not used by your\ncompiler/platform).\n\nPerformance-wise I don't think there is any loss for single thread\nmode. I rely on compilers recognizing HAVE_THREADS being a constant\nand remove dead code or at least optimize in favor of non-dead code.\n\nMemory-wise, yes we use some more memory in single thread mode. But we\ndon't have zillions of mutexes or thread id, so a bit extra memory\ndoes not worry me so much.\n\nHmm?\n---\n Makefile | 2 +-\n builtin/index-pack.c | 68 ++++++++++++--------------------------------\n thread-utils.c | 30 +++++++++++++++++++\n thread-utils.h | 38 +++++++++++++++++++++++--\n 4 files changed, 84 insertions(+), 54 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 5bf1af369e..ef852031bd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -981,6 +981,7 @@ LIB_OBJS += sub-process.o\n LIB_OBJS += symlinks.o\n LIB_OBJS += tag.o\n LIB_OBJS += tempfile.o\n+LIB_OBJS += thread-utils.o\n LIB_OBJS += tmp-objdir.o\n LIB_OBJS += trace.o\n LIB_OBJS += trailer.o\n@@ -1664,7 +1665,6 @@ ifdef NO_PTHREADS\n else\n \tBASIC_CFLAGS += $(PTHREAD_CFLAGS)\n \tEXTLIBS += $(PTHREAD_LIBS)\n-\tLIB_OBJS += thread-utils.o\n endif\n \n ifdef HAVE_PATHS_H\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 2004e25da2..bbd66ca025 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -42,9 +42,7 @@ struct base_data {\n };\n \n struct thread_local {\n-#ifndef NO_PTHREADS\n \tpthread_t thread;\n-#endif\n \tstruct base_data *base_cache;\n \tsize_t base_cache_used;\n \tint pack_fd;\n@@ -98,8 +96,6 @@ static uint32_t input_crc32;\n static int input_fd, output_fd;\n static const char *curr_pack;\n \n-#ifndef NO_PTHREADS\n-\n static struct thread_local *thread_data;\n static int nr_dispatched;\n static int threads_active;\n@@ -179,26 +175,6 @@ static void cleanup_thread(void)\n \tfree(thread_data);\n }\n \n-#else\n-\n-#define read_lock()\n-#define read_unlock()\n-\n-#define counter_lock()\n-#define counter_unlock()\n-\n-#define work_lock()\n-#define work_unlock()\n-\n-#define deepest_delta_lock()\n-#define deepest_delta_unlock()\n-\n-#define type_cas_lock()\n-#define type_cas_unlock()\n-\n-#endif\n-\n-\n static int mark_link(struct object *obj, int type, void *data, struct fsck_options *options)\n {\n \tif (!obj)\n@@ -364,22 +340,20 @@ static NORETURN void bad_object(off_t offset, const char *format, ...)\n \n static inline struct thread_local *get_thread_data(void)\n {\n-#ifndef NO_PTHREADS\n-\tif (threads_active)\n-\t\treturn pthread_getspecific(key);\n-\tassert(!threads_active &&\n-\t \"This should only be reached when all threads are gone\");\n-#endif\n+\tif (HAVE_THREADS) {\n+\t\tif (threads_active)\n+\t\t\treturn pthread_getspecific(key);\n+\t\tassert(!threads_active &&\n+\t\t \"This should only be reached when all threads are gone\");\n+\t}\n \treturn ¬hread_data;\n }\n \n-#ifndef NO_PTHREADS\n static void set_thread_data(struct thread_local *data)\n {\n \tif (threads_active)\n \t\tpthread_setspecific(key, data);\n }\n-#endif\n \n static struct base_data *alloc_base_data(void)\n {\n@@ -1092,7 +1066,6 @@ static void resolve_base(struct object_entry *obj)\n \tfind_unresolved_deltas(base_obj);\n }\n \n-#ifndef NO_PTHREADS\n static void *threaded_second_pass(void *data)\n {\n \tset_thread_data(data);\n@@ -1116,7 +1089,6 @@ static void *threaded_second_pass(void *data)\n \t}\n \treturn NULL;\n }\n-#endif\n \n /*\n * First pass:\n@@ -1213,7 +1185,6 @@ static void resolve_deltas(void)\n \t\tprogress = start_progress(_(\"Resolving deltas\"),\n \t\t\t\t\t nr_ref_deltas + nr_ofs_deltas);\n \n-#ifndef NO_PTHREADS\n \tnr_dispatched = 0;\n \tif (nr_threads > 1 || getenv(\"GIT_FORCE_THREADS\")) {\n \t\tinit_thread();\n@@ -1229,7 +1200,6 @@ static void resolve_deltas(void)\n \t\tcleanup_thread();\n \t\treturn;\n \t}\n-#endif\n \n \tfor (i = 0; i < nr_objects; i++) {\n \t\tstruct object_entry *obj = &objects[i];\n@@ -1531,11 +1501,11 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)\n \t\tif (nr_threads < 0)\n \t\t\tdie(_(\"invalid number of threads specified (%d)\"),\n \t\t\t nr_threads);\n-#ifdef NO_PTHREADS\n-\t\tif (nr_threads != 1)\n-\t\t\twarning(_(\"no threads support, ignoring %s\"), k);\n-\t\tnr_threads = 1;\n-#endif\n+\t\tif (!HAVE_THREADS) {\n+\t\t\tif (nr_threads != 1)\n+\t\t\t\twarning(_(\"no threads support, ignoring %s\"), k);\n+\t\t\tnr_threads = 1;\n+\t\t}\n \t\treturn 0;\n \t}\n \treturn git_default_config(k, v, cb);\n@@ -1723,12 +1693,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tnr_threads = strtoul(arg+10, &end, 0);\n \t\t\t\tif (!arg[10] || *end || nr_threads < 0)\n \t\t\t\t\tusage(index_pack_usage);\n-#ifdef NO_PTHREADS\n-\t\t\t\tif (nr_threads != 1)\n-\t\t\t\t\twarning(_(\"no threads support, \"\n-\t\t\t\t\t\t \"ignoring %s\"), arg);\n-\t\t\t\tnr_threads = 1;\n-#endif\n+\t\t\t\tif (!HAVE_THREADS) {\n+\t\t\t\t\tif (nr_threads != 1)\n+\t\t\t\t\t\twarning(_(\"no threads support, \"\n+\t\t\t\t\t\t\t \"ignoring %s\"), arg);\n+\t\t\t\t\tnr_threads = 1;\n+\t\t\t\t}\n \t\t\t} else if (starts_with(arg, \"--pack_header=\")) {\n \t\t\t\tstruct pack_header *hdr;\n \t\t\t\tchar *c;\n@@ -1791,14 +1761,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tif (strict)\n \t\topts.flags |= WRITE_IDX_STRICT;\n \n-#ifndef NO_PTHREADS\n-\tif (!nr_threads) {\n+\tif (HAVE_THREADS && !nr_threads) {\n \t\tnr_threads = online_cpus();\n \t\t/* An experiment showed that more threads does not mean faster */\n \t\tif (nr_threads > 3)\n \t\t\tnr_threads = 3;\n \t}\n-#endif\n \n \tcurr_pack = open_pack_file(pack_name);\n \tparse_pack_header();\ndiff --git a/thread-utils.c b/thread-utils.c\nindex a2135e0743..d205a474e0 100644\n--- a/thread-utils.c\n+++ b/thread-utils.c\n@@ -20,6 +20,9 @@\n \n int online_cpus(void)\n {\n+#ifdef NO_PTHREADS\n+\treturn 1;\n+#else\n #ifdef _SC_NPROCESSORS_ONLN\n \tlong ncpus;\n #endif\n@@ -59,10 +62,12 @@ int online_cpus(void)\n #endif\n \n \treturn 1;\n+#endif\n }\n \n int init_recursive_mutex(pthread_mutex_t *m)\n {\n+#ifndef NO_PTHREADS\n \tpthread_mutexattr_t a;\n \tint ret;\n \n@@ -74,4 +79,29 @@ int init_recursive_mutex(pthread_mutex_t *m)\n \t\tpthread_mutexattr_destroy(&a);\n \t}\n \treturn ret;\n+#else\n+\treturn ENOSYS;\n+#endif\n+}\n+\n+#ifdef NO_PTHREADS\n+int dummy_pthread_create(pthread_t *pthread, const void *attr,\n+\t\t\t void *(*fn)(void *), void *data)\n+{\n+\treturn ENOSYS;\n }\n+\n+int dummy_pthread_init(void *data)\n+{\n+\t/*\n+\t * Do nothing.\n+\t *\n+\t * The main purpose of this function is to break compiler's\n+\t * flow analysis or it may realize that functions like\n+\t * pthread_mutex_init() is no-op, which means the (static)\n+\t * variable is not used/initialized at all and trigger\n+\t * -Wunused-variable\n+\t */\n+\treturn ENOSYS;\n+}\n+#endif\ndiff --git a/thread-utils.h b/thread-utils.h\nindex d9a769d190..b8c6500c42 100644\n--- a/thread-utils.h\n+++ b/thread-utils.h\n@@ -4,12 +4,44 @@\n #ifndef NO_PTHREADS\n #include \n \n-extern int online_cpus(void);\n-extern int init_recursive_mutex(pthread_mutex_t*);\n+#define HAVE_THREADS 1\n \n #else\n \n-#define online_cpus() 1\n+#define HAVE_THREADS 0\n+\n+/*\n+ * macros instead of typedefs because pthread definitions may have\n+ * been pulled in by some system dependencies even though the user\n+ * wants to disable pthread.\n+ */\n+#define pthread_t int\n+#define pthread_mutex_t int\n+\n+#define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)\n+#define pthread_mutex_lock(mutex)\n+#define pthread_mutex_unlock(mutex)\n+#define pthread_mutex_destroy(mutex)\n+\n+#define pthread_key_create(key, attr) dummy_pthread_init(key)\n+#define pthread_key_delete(key)\n+\n+#define pthread_create(thread, attr, fn, data) \\\n+\tdummy_pthread_create(thread, attr, fn, data)\n+#define pthread_join(thread, reval) ENOSYS\n+\n+#define pthread_setspecific(key, data)\n+#define pthread_getspecific(key) NULL\n+\n+int dummy_pthread_create(pthread_t *pthread, const void *attr,\n+\t\t\t void *(*fn)(void *), void *data);\n+\n+int dummy_pthread_init(void *);\n \n #endif\n+\n+int online_cpus(void);\n+int init_recursive_mutex(pthread_mutex_t*);\n+\n+\n #endif /* THREAD_COMPAT_H */\n-- \n2.19.1.647.g708186aaf9\n\n"},{"id":"360889","messageId":"xmqqftx24tlc.fsf@gitster-ct.c.googlers.com","threadId":"49603","inReplyTo":"3295c759-473b-f279-5443-f915fd0b1061@ramsayjones.plus.com","subject":"Re: [PATCH] config.mak.dev: enable -Wunused-function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-19T01:23:59Z","receivedAt":"2018-10-19T01:24:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> I haven't looked too deeply, but this seems to be caused by\n> Junio's commit 42c89ea70a (\"SQUASH??? - convert the other user of\n> string-list as db\", 2018-10-17) which removes a call to the\n> add_existing() function - the subject of the warning.\n\nThat is very understandable and it is immediately obvious to me that\nthe unused-function warning is being useful there ;-)\n"},{"id":"361325","messageId":"20181023202842.GA17371@sigill.intra.peff.net","threadId":"49603","inReplyTo":"20181018180522.17642-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC] thread-utils: better wrapper to avoid #ifdef NO_PTHREADS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-23T20:28:43Z","receivedAt":"2018-10-23T20:28:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 18, 2018 at 08:05:22PM +0200, Nguyễn Thái Ngọc Duy wrote:\n\n> On Thu, Oct 18, 2018 at 7:09 PM Jeff King wrote:\n> > > In this particular case though I think we should be able to avoid so\n> > > much #if if we make a wrapper for pthread api that would return an\n> > > error or something when pthread is not available. But similar\n> > > situation may happen elsewhere too.\n> >\n> > Yeah, I think that is generally the preferred method anyway, just\n> > because of readability and simplicity.\n> \n> I've wanted to do this for a while, so let's test the water and see if\n> it's well received.\n> \n> This patch is a proof of concept that adds just enough macros so that\n> I can build index-pack.c on a single thread mode with zero #ifdef\n> related to NO_PTHREADS.\n> \n> Besides readability and simplicity, it reduces the chances of breaking\n> conditional builds (e.g. you rename a variable name but forgot that\n> the variable is in #if block that is not used by your\n> compiler/platform).\n\nYes, I love this. We're already halfway there with things like\nread_lock() in index-pack and elsewhere, which are conditionally no-ops.\nThe resulting code is much easier to read, I think.\n\n> Performance-wise I don't think there is any loss for single thread\n> mode. I rely on compilers recognizing HAVE_THREADS being a constant\n> and remove dead code or at least optimize in favor of non-dead code.\n> \n> Memory-wise, yes we use some more memory in single thread mode. But we\n> don't have zillions of mutexes or thread id, so a bit extra memory\n> does not worry me so much.\n\nYeah, I don't think carrying around a handful of ints is going to be a\nbig deal.\n\nI also think we may want to make a fundamental shift in our view of\nthread support. In the early days, it was \"well, this is a thing that\nmodern systems can take advantage of for certain commands\". But these\ndays I suspect it is more like \"there are a handful of legacy systems\nthat do not even support threads\".\n\nI don't think we should break the build on those legacy systems, but\nit's probably OK to stop thinking of it as \"non-threaded platforms are\nthe default and must pay zero cost\" and more as \"threaded platforms are\nthe default, and non-threaded ones are OK to pay a small cost as long as\nthey still work\".\n\n> @@ -74,4 +79,29 @@ int init_recursive_mutex(pthread_mutex_t *m)\n> \t\tpthread_mutexattr_destroy(&a);\n> \t}\n> \treturn ret;\n> +#else\n> +\treturn ENOSYS;\n> +#endif\n> +}\n\nI suspect some of these ENOSYS could just become a silent success.\n(\"yep, I initialized your dummy mutex\"). But it probably doesn't matter\nmuch either way, as we would not generally even bother checking this\nreturn.\n\n> +#ifdef NO_PTHREADS\n> +int dummy_pthread_create(pthread_t *pthread, const void *attr,\n> +\t\t\t void *(*fn)(void *), void *data)\n> +{\n> +\treturn ENOSYS;\n> }\n\nWhereas for this one, ENOSYS makes a lot of sense (we should avoid the\nthreaded code-path anyway when we see that online_cpus()==1, and this\nwould let us know when we mess that up).\n\n> +int dummy_pthread_init(void *data)\n> +{\n> +\t/*\n> +\t * Do nothing.\n> +\t *\n> +\t * The main purpose of this function is to break compiler's\n> +\t * flow analysis or it may realize that functions like\n> +\t * pthread_mutex_init() is no-op, which means the (static)\n> +\t * variable is not used/initialized at all and trigger\n> +\t * -Wunused-variable\n> +\t */\n> +\treturn ENOSYS;\n> +}\n\nIt might be worth marking the dummy variables as MAYBE_UNUSED, exactly\nto avoid this kind of compiler complaint.\n\n-Peff\n"},{"id":"361346","messageId":"xmqq1s8gqcd8.fsf@gitster-ct.c.googlers.com","threadId":"49603","inReplyTo":"20181023202842.GA17371@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] thread-utils: better wrapper to avoid #ifdef NO_PTHREADS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T02:58:43Z","receivedAt":"2018-10-24T02:58:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I also think we may want to make a fundamental shift in our view of\n> thread support. In the early days, it was \"well, this is a thing that\n> modern systems can take advantage of for certain commands\". But these\n> days I suspect it is more like \"there are a handful of legacy systems\n> that do not even support threads\".\n>\n> I don't think we should break the build on those legacy systems, but\n> it's probably OK to stop thinking of it as \"non-threaded platforms are\n> the default and must pay zero cost\" and more as \"threaded platforms are\n> the default, and non-threaded ones are OK to pay a small cost as long as\n> they still work\".\n\nGood suggestion.\n\n"},{"id":"361625","messageId":"852ad281-09df-c980-790c-df25e82b3331@gmail.com","threadId":"49603","inReplyTo":"20181023202842.GA17371@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] thread-utils: better wrapper to avoid #ifdef NO_PTHREADS","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-10-26T14:09:46Z","receivedAt":"2018-10-26T14:09:54Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 10/23/2018 4:28 PM, Jeff King wrote:\n> On Thu, Oct 18, 2018 at 08:05:22PM +0200, Nguyễn Thái Ngọc Duy wrote:\n> \n>> On Thu, Oct 18, 2018 at 7:09 PM Jeff King wrote:\n>>>> In this particular case though I think we should be able to avoid so\n>>>> much #if if we make a wrapper for pthread api that would return an\n>>>> error or something when pthread is not available. But similar\n>>>> situation may happen elsewhere too.\n>>>\n>>> Yeah, I think that is generally the preferred method anyway, just\n>>> because of readability and simplicity.\n>>\n>> I've wanted to do this for a while, so let's test the water and see if\n>> it's well received.\n>>\n>> This patch is a proof of concept that adds just enough macros so that\n>> I can build index-pack.c on a single thread mode with zero #ifdef\n>> related to NO_PTHREADS.\n>>\n>> Besides readability and simplicity, it reduces the chances of breaking\n>> conditional builds (e.g. you rename a variable name but forgot that\n>> the variable is in #if block that is not used by your\n>> compiler/platform).\n> \n> Yes, I love this. We're already halfway there with things like\n> read_lock() in index-pack and elsewhere, which are conditionally no-ops.\n> The resulting code is much easier to read, I think.\n> \n\nI am also very much in favor of this. I updated a couple of places \nthreading is being used that I've been working in (preload-index and \nread-cache) and both are much simplified using your proof of concept patch.\n\n>> Performance-wise I don't think there is any loss for single thread\n>> mode. I rely on compilers recognizing HAVE_THREADS being a constant\n>> and remove dead code or at least optimize in favor of non-dead code.\n>>\n>> Memory-wise, yes we use some more memory in single thread mode. But we\n>> don't have zillions of mutexes or thread id, so a bit extra memory\n>> does not worry me so much.\n> \n> Yeah, I don't think carrying around a handful of ints is going to be a\n> big deal.\n> \n\nJust to be complete, there _is_ an additional cost. Today, code paths \nthat are only executed when there are pthreads available are excluded \nfrom the binary (via #ifdef). With this change, those code paths would \nnow be included causing some code bloat to NO_PTHREAD threaded images.\n\nOne example of this is in read-cache.c where the ieot read/write \nfunctions aren't included for NO_PTHREAD but now would be.\n\n> I also think we may want to make a fundamental shift in our view of\n> thread support. In the early days, it was \"well, this is a thing that\n> modern systems can take advantage of for certain commands\". But these\n> days I suspect it is more like \"there are a handful of legacy systems\n> that do not even support threads\".\n> \n> I don't think we should break the build on those legacy systems, but\n> it's probably OK to stop thinking of it as \"non-threaded platforms are\n> the default and must pay zero cost\" and more as \"threaded platforms are\n> the default, and non-threaded ones are OK to pay a small cost as long as\n> they still work\".\n> \n\nI agree though I'm still curious if there are still no-threaded \nplatforms taking new versions of git. Perhaps we should do the \ndepreciation warning you suggested elsewhere and see how much push back \nwe get. It's unlikely we'd get lucky and be able to stop supporting \nthem completely but it's worth asking!\n\n>> @@ -74,4 +79,29 @@ int init_recursive_mutex(pthread_mutex_t *m)\n>> \t\tpthread_mutexattr_destroy(&a);\n>> \t}\n>> \treturn ret;\n>> +#else\n>> +\treturn ENOSYS;\n>> +#endif\n>> +}\n> \n> I suspect some of these ENOSYS could just become a silent success.\n> (\"yep, I initialized your dummy mutex\"). But it probably doesn't matter\n> much either way, as we would not generally even bother checking this\n> return.\n> \n>> +#ifdef NO_PTHREADS\n>> +int dummy_pthread_create(pthread_t *pthread, const void *attr,\n>> +\t\t\t void *(*fn)(void *), void *data)\n>> +{\n>> +\treturn ENOSYS;\n>> }\n> \n> Whereas for this one, ENOSYS makes a lot of sense (we should avoid the\n> threaded code-path anyway when we see that online_cpus()==1, and this\n> would let us know when we mess that up).\n> \n\nThis highlights something anyone writing multi-threaded code will need \nto pay attention to that wasn't an issue before. If you attempt to \ncreate more threads than online_cpus(), the pthread_create() call will \nfail and needs to be handled gracefully.\n\nOne example of this is in preload-index.c where (up to) 20 threads are \ncreated irrespective of what online_cpus() returns and if \npthread_create() fails, it just dies. The logic would need to be \nupdated for this to work correctly.\n\nI still think this is a much simpler issue to deal with than what we \nhave today with having to write/debug multiple code paths but I did want \nto point it out for completeness.\n\n>> +int dummy_pthread_init(void *data)\n>> +{\n>> +\t/*\n>> +\t * Do nothing.\n>> +\t *\n>> +\t * The main purpose of this function is to break compiler's\n>> +\t * flow analysis or it may realize that functions like\n>> +\t * pthread_mutex_init() is no-op, which means the (static)\n>> +\t * variable is not used/initialized at all and trigger\n>> +\t * -Wunused-variable\n>> +\t */\n>> +\treturn ENOSYS;\n>> +}\n> \n> It might be worth marking the dummy variables as MAYBE_UNUSED, exactly\n> to avoid this kind of compiler complaint.\n> \n> -Peff\n> \n"},{"id":"361761","messageId":"20181027071220.GB17203@sigill.intra.peff.net","threadId":"49603","inReplyTo":"852ad281-09df-c980-790c-df25e82b3331@gmail.com","subject":"can we deprecate NO_PTHREADS?, was: better wrapper to avoid #ifdef NO_PTHREADS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-27T07:12:21Z","receivedAt":"2018-10-27T07:12:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 26, 2018 at 10:09:46AM -0400, Ben Peart wrote:\n\n> > Yeah, I don't think carrying around a handful of ints is going to be a\n> > big deal.\n> \n> Just to be complete, there _is_ an additional cost. Today, code paths that\n> are only executed when there are pthreads available are excluded from the\n> binary (via #ifdef). With this change, those code paths would now be\n> included causing some code bloat to NO_PTHREAD threaded images.\n> \n> One example of this is in read-cache.c where the ieot read/write functions\n> aren't included for NO_PTHREAD but now would be.\n\nYeah. I think that is also an OK cost. It may bloat the binary a little,\nbut if we're not running those instructions, they're probably not\nbloating CPU cache, etc.\n\n> > I don't think we should break the build on those legacy systems, but\n> > it's probably OK to stop thinking of it as \"non-threaded platforms are\n> > the default and must pay zero cost\" and more as \"threaded platforms are\n> > the default, and non-threaded ones are OK to pay a small cost as long as\n> > they still work\".\n> \n> I agree though I'm still curious if there are still no-threaded platforms\n> taking new versions of git. Perhaps we should do the depreciation warning\n> you suggested elsewhere and see how much push back we get. It's unlikely\n> we'd get lucky and be able to stop supporting them completely but it's worth\n> asking!\n\nI'm trying to think how that might look. I think putting it into the\nbinary and warning at runtime is probably a little _too_ obnoxious. Here\nare some other options ranging from less to more annoying:\n\n - mention it in the release notes (guaranteed not to hurt anybody, but\n also likely to be missed)\n\n - a build-time warning when the NO_PTHREADS is set. Also easy to miss,\n but at least hits the people who are using it.\n\n - rename NO_PTHREADS to NO_PTHREADS_REALLY, and error out the build\n when NO_PTHREADS is set. That would certainly get people's\n attention.\n\nI guess it would make sense to do them in ascending order. I.e., maybe\nstart with something like:\n\ndiff --git a/Makefile b/Makefile\nindex b08d5ea258..8ac234e4c3 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1670,6 +1670,11 @@ ifdef RUNTIME_PREFIX\n endif\n \n ifdef NO_PTHREADS\n+$(warning The NO_PTHREADS flag is being considered for deprecation,)\n+$(warning which would require all platforms supported by Git to have)\n+$(warning some kind of threading support. If your platform does not)\n+$(warning support threading, please report it by sending an email to)\n+$(warning git@vger.kernel.org.)\n \tBASIC_CFLAGS += -DNO_PTHREADS\n else\n \tBASIC_CFLAGS += $(PTHREAD_CFLAGS)\n\nIf we get no takers, that doesn't prove much, but it builds confidence\nin moving to the \"REALLY\" variant. And if we do get a report, we can be\ntold this is a bad idea before moving further.\n\nAlso, the posting of the patch itself may gather some feedback. ;)\n\n-Peff\n"},{"id":"361765","messageId":"CACsJy8D8a-0R=J6nCPwBAypYF=rejgT-3QFnuK1h3vMcJGG7_A@mail.gmail.com","threadId":"49603","inReplyTo":"852ad281-09df-c980-790c-df25e82b3331@gmail.com","subject":"Re: [PATCH/RFC] thread-utils: better wrapper to avoid #ifdef NO_PTHREADS","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-27T07:26:28Z","receivedAt":"2018-10-27T07:26:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Oct 26, 2018 at 4:09 PM Ben Peart wrote:\n> I agree though I'm still curious if there are still no-threaded\n> platforms taking new versions of git. Perhaps we should do the\n> depreciation warning you suggested elsewhere and see how much push back\n> we get. It's unlikely we'd get lucky and be able to stop supporting\n> them completely but it's worth asking!\n\nNO_PTHREADS can also be used even though the platform supports\nmultithread: to make keep git execution in a single core/thread. It\nmight matter on hosted systems with limited cpu power and you don't\nwant git to hog it all. Yes it can also be achieved by setting a\nzillion config keys to \"1\", this way is just simpler.\n-- \nDuy\n"},{"id":"361777","messageId":"20181027081724.GH26685@sigill.intra.peff.net","threadId":"49603","inReplyTo":"CACsJy8D8a-0R=J6nCPwBAypYF=rejgT-3QFnuK1h3vMcJGG7_A@mail.gmail.com","subject":"Re: [PATCH/RFC] thread-utils: better wrapper to avoid #ifdef NO_PTHREADS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-27T08:17:25Z","receivedAt":"2018-10-27T08:17:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 27, 2018 at 09:26:28AM +0200, Duy Nguyen wrote:\n\n> On Fri, Oct 26, 2018 at 4:09 PM Ben Peart wrote:\n> > I agree though I'm still curious if there are still no-threaded\n> > platforms taking new versions of git. Perhaps we should do the\n> > depreciation warning you suggested elsewhere and see how much push back\n> > we get. It's unlikely we'd get lucky and be able to stop supporting\n> > them completely but it's worth asking!\n> \n> NO_PTHREADS can also be used even though the platform supports\n> multithread: to make keep git execution in a single core/thread. It\n> might matter on hosted systems with limited cpu power and you don't\n> want git to hog it all. Yes it can also be achieved by setting a\n> zillion config keys to \"1\", this way is just simpler.\n\nYeah, I wondered about that use case (also with your patches, and\nwhether they might run into problems on systems that _do_ have pthreads,\nbut just don't want to compile with them).\n\nBut I think that is pretty easily solved by just having a single runtime\noption (e.g., to just pretend that oneline_cpus is always 1 by default).\n\n-Peff\n"}]}