{"thread":{"id":"53150","subject":"[PATCH] Fix -Wmaybe-uninitialized warnings under -O0","startedAt":"2020-04-01T07:30:26Z","lastAt":"2021-05-21T09:34:21Z","messageCount":31,"participants":["Denton Liu","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano","Johannes Schindelin","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"394468","messageId":"33984eeaabbfbcfd4b9d3903549d8b7d6c4ced7e.1585726172.git.liu.denton@gmail.com","threadId":"53150","inReplyTo":null,"subject":"[PATCH] Fix -Wmaybe-uninitialized warnings under -O0","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-01T07:30:16Z","receivedAt":"2020-04-01T07:30:26Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When compiling Git under -O0, gcc (Arch Linux 9.3.0-1) 9.3.0 produces\nmany -Wmaybe-uninitialized warnings. These are false positives since\nwhen Git is compiled under -O2, gcc is smart enough to see that the\ncode paths that use these variables all initialise them beforehand.\nNonetheless, these warnings block the compilation process when\nDEVELOPER=1 is enabled (which enables -Werror).\n\nFix these warnings by initializing these variables with dummy values (0,\n-1 or NULL as appropriate).\n\nSigned-off-by: Denton Liu \n---\n builtin/branch.c | 2 +-\n diff.c | 2 +-\n fast-import.c | 4 ++--\n http-backend.c | 2 +-\n ref-filter.c | 2 +-\n refs/packed-backend.c | 2 +-\n t/helper/test-ref-store.c | 2 +-\n trace2/tr2_dst.c | 2 +-\n trailer.c | 2 +-\n 9 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d8297f80ff..3669fba546 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -455,7 +455,7 @@ static void print_current_branch_name(void)\n {\n \tint flags;\n \tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n-\tconst char *shortname;\n+\tconst char *shortname = NULL;\n \tif (!refname)\n \t\tdie(_(\"could not resolve HEAD\"));\n \telse if (!(flags & REF_ISSYMREF))\ndiff --git a/diff.c b/diff.c\nindex f2cfbf2214..99a35774d7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3263,7 +3263,7 @@ static void emit_binary_diff_body(struct diff_options *o,\n \tvoid *delta;\n \tvoid *deflated;\n \tvoid *data;\n-\tunsigned long orig_size;\n+\tunsigned long orig_size = 0;\n \tunsigned long delta_size;\n \tunsigned long deflate_size;\n \tunsigned long data_size;\ndiff --git a/fast-import.c b/fast-import.c\nindex b8b65a801c..0509c0b92f 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1829,7 +1829,7 @@ static void parse_original_identifier(void)\n \n static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)\n {\n-\tconst char *data;\n+\tconst char *data = NULL;\n \tstrbuf_reset(sb);\n \n \tif (!skip_prefix(command_buf.buf, \"data \", &data))\n@@ -2719,7 +2719,7 @@ static void parse_new_commit(const char *arg)\n static void parse_new_tag(const char *arg)\n {\n \tstatic struct strbuf msg = STRBUF_INIT;\n-\tconst char *from;\n+\tconst char *from = NULL;\n \tchar *tagger;\n \tstruct branch *s;\n \tstruct tag *t;\ndiff --git a/http-backend.c b/http-backend.c\nindex ec3144b444..1091cdf2cd 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -252,7 +252,7 @@ static void http_config(void)\n \n static struct rpc_service *select_service(struct strbuf *hdr, const char *name)\n {\n-\tconst char *svc_name;\n+\tconst char *svc_name = NULL;\n \tstruct rpc_service *svc = NULL;\n \tint i;\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex b1812cb69a..6abd48f81a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1938,7 +1938,7 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,\n {\n \tstruct string_list prefixes = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *prefix;\n-\tint ret;\n+\tint ret = 0;\n \n \tif (!filter->match_as_path) {\n \t\t/*\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 4458a0f69c..f37c6d19a6 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -627,7 +627,7 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n \n \t/* If the file has a header line, process it: */\n \tif (snapshot->buf < snapshot->eof && *snapshot->buf == '#') {\n-\t\tchar *tmp, *p, *eol;\n+\t\tchar *tmp, *p = NULL, *eol;\n \t\tstruct string_list traits = STRING_LIST_INIT_NODUP;\n \n \t\teol = memchr(snapshot->buf, '\\n',\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 799fc00aa1..d82dcb83a3 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -19,7 +19,7 @@ static unsigned int arg_flags(const char *arg, const char *name)\n \n static const char **get_store(const char **argv, struct ref_store **refs)\n {\n-\tconst char *gitdir;\n+\tconst char *gitdir = NULL;\n \n \tif (!argv[0]) {\n \t\tdie(\"ref store required\");\ndiff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\nindex ae052a07fe..910301a53d 100644\n--- a/trace2/tr2_dst.c\n+++ b/trace2/tr2_dst.c\n@@ -227,7 +227,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n {\n \tunsigned int uds_try = 0;\n \tint fd;\n-\tint e;\n+\tint e = 0;\n \tconst char *path = NULL;\n \n \t/*\ndiff --git a/trailer.c b/trailer.c\nindex 0c414f2fed..cac9d0a4d9 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -507,7 +507,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n \tstruct arg_item *item;\n \tstruct conf_info *conf;\n \tchar *name = NULL;\n-\tenum trailer_info_type type;\n+\tenum trailer_info_type type = -1;\n \tint i;\n \n \tif (!skip_prefix(conf_key, \"trailer.\", &trailer_item))\n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"394470","messageId":"20200401090508.GA828142@generichostname","threadId":"53150","inReplyTo":"CAPUEspgBkmxszgBee8C9hZnEwqztf-XKEj7LB_jWVFJaJCge0w@mail.gmail.com","subject":"Re: [PATCH] Fix -Wmaybe-uninitialized warnings under -O0","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-01T09:05:08Z","receivedAt":"2020-04-01T09:05:15Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Carlos,\n\nOn Wed, Apr 01, 2020 at 01:01:41AM -0700, Carlo Arenas wrote:\n> On Wed, Apr 1, 2020 at 12:31 AM Denton Liu wrote:\n> \n> > Nonetheless, these warnings block the compilation process when\n> > DEVELOPER=1 is enabled (which enables -Werror).\n> >\n> \n> you can also alternatively use DEVOPTS=no-error\n\nThanks for the tip. By the way, it seems like your message was sent with\nHTML so it didn't hit the list.\n\n-Denton\n\n> \n> Carlo\n"},{"id":"394471","messageId":"20200401095255.GA60227@coredump.intra.peff.net","threadId":"53150","inReplyTo":"33984eeaabbfbcfd4b9d3903549d8b7d6c4ced7e.1585726172.git.liu.denton@gmail.com","subject":"Re: [PATCH] Fix -Wmaybe-uninitialized warnings under -O0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-01T09:52:55Z","receivedAt":"2020-04-01T09:52:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 01, 2020 at 03:30:16AM -0400, Denton Liu wrote:\n\n> When compiling Git under -O0, gcc (Arch Linux 9.3.0-1) 9.3.0 produces\n> many -Wmaybe-uninitialized warnings. These are false positives since\n> when Git is compiled under -O2, gcc is smart enough to see that the\n> code paths that use these variables all initialise them beforehand.\n> Nonetheless, these warnings block the compilation process when\n> DEVELOPER=1 is enabled (which enables -Werror).\n> \n> Fix these warnings by initializing these variables with dummy values (0,\n> -1 or NULL as appropriate).\n\nHmph. I almost always compile with -O0 and have been using gcc 9.3.0\nsince it was packaged for Debian a few weeks ago, but I don't see any of\nthese warnings.\n\nThe current version in Debian unstable is 9.3.0-8, which picks up some\nextra patches from the upstream gcc-9 branch. But even if I download a\nsnapshot of the original 9.3.0 release, it builds fine.\n\nSo why does your version behave differently? And if this is a temporary\nstate for a buggy version of gcc (that may be fixed in the next point\nrelease), is it worth changing our source code to appease it?\n\n-Peff\n"},{"id":"394495","messageId":"20200401140643.GA880372@generichostname","threadId":"53150","inReplyTo":"20200401095255.GA60227@coredump.intra.peff.net","subject":"Re: [PATCH] Fix -Wmaybe-uninitialized warnings under -O0","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-01T14:06:43Z","receivedAt":"2020-04-01T14:06:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Peff,\n\nOn Wed, Apr 01, 2020 at 05:52:55AM -0400, Jeff King wrote:\n> On Wed, Apr 01, 2020 at 03:30:16AM -0400, Denton Liu wrote:\n> \n> > When compiling Git under -O0, gcc (Arch Linux 9.3.0-1) 9.3.0 produces\n> > many -Wmaybe-uninitialized warnings. These are false positives since\n> > when Git is compiled under -O2, gcc is smart enough to see that the\n> > code paths that use these variables all initialise them beforehand.\n> > Nonetheless, these warnings block the compilation process when\n> > DEVELOPER=1 is enabled (which enables -Werror).\n> > \n> > Fix these warnings by initializing these variables with dummy values (0,\n> > -1 or NULL as appropriate).\n> \n> Hmph. I almost always compile with -O0 and have been using gcc 9.3.0\n> since it was packaged for Debian a few weeks ago, but I don't see any of\n> these warnings.\n> \n> The current version in Debian unstable is 9.3.0-8, which picks up some\n> extra patches from the upstream gcc-9 branch. But even if I download a\n> snapshot of the original 9.3.0 release, it builds fine.\n> \n> So why does your version behave differently? And if this is a temporary\n> state for a buggy version of gcc (that may be fixed in the next point\n> release), is it worth changing our source code to appease it?\n\nA correction to the earlier message... It seems like I wasn't reporting\nthe correct settings. I was actually compiling with -Og, not -O0\n(whoops!).\n\nI tested it with gcc-8 and it seems like it also reports the same\nproblem. Also, -O1 reports warnings as well.\n\nI guess the question is do we only support -O0 and -O2 or should we\nsupport the levels in between as well?\n\nSorry for the confusion,\n\nDenton\n\n> \n> -Peff\n"},{"id":"394665","messageId":"20200403140447.GA639586@coredump.intra.peff.net","threadId":"53150","inReplyTo":"20200401140643.GA880372@generichostname","subject":"Re: [PATCH] Fix -Wmaybe-uninitialized warnings under -O0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-03T14:04:47Z","receivedAt":"2020-04-03T14:04:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 01, 2020 at 10:06:43AM -0400, Denton Liu wrote:\n\n> > So why does your version behave differently? And if this is a temporary\n> > state for a buggy version of gcc (that may be fixed in the next point\n> > release), is it worth changing our source code to appease it?\n> \n> A correction to the earlier message... It seems like I wasn't reporting\n> the correct settings. I was actually compiling with -Og, not -O0\n> (whoops!).\n> \n> I tested it with gcc-8 and it seems like it also reports the same\n> problem. Also, -O1 reports warnings as well.\n\nAh, OK, I can reproduce easily with -Og (up through gcc-10). Most of\nthem don't trigger with -O1; just the one in ref-filter.c.\n\nThat one's interesting. We have:\n\n int ret = 0;\n ...\n if (...)\n ...\n else\n ret = for_each_fullref_in_pattern(...);\n ...\n return ret;\n\nSo we'd either have 0 or an assigned return. But the bug is actually in\nfor_each_fullref_in_pattern(), which does this:\n\n int ret; /* uninitialized! */\n\n /* a bunch of early return conditionals */\n if (...)\n return ...;\n\n for_each_string_list_item(...) {\n ret = for_each_fullref_in(...);\n }\n\n return ret;\n\nbut that will return an uninitialized value when there are no patterns.\nI doubt we have such a case, but that may explain why -O0 does not\ncomplain (it assumes \"in_pattern\" will return a useful value) and -O2\ndoes not (it is able to figure out that it always does), but -O1 only\ninlines part of it.\n\nCuriously, -Og _does_ find the correct function.\n\nYour patch silences it, but is it doing the right thing? It sets \"ret =\n0\", but we haven't actually iterated anything. Should it be an error\ninstead?\n\n-Peff\n"},{"id":"394667","messageId":"20200403143844.GB639586@coredump.intra.peff.net","threadId":"53150","inReplyTo":"20200403140447.GA639586@coredump.intra.peff.net","subject":"Re: [PATCH] Fix -Wmaybe-uninitialized warnings under -O0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-03T14:38:44Z","receivedAt":"2020-04-03T14:38:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 03, 2020 at 10:04:47AM -0400, Jeff King wrote:\n\n> Ah, OK, I can reproduce easily with -Og (up through gcc-10). Most of\n> them don't trigger with -O1; just the one in ref-filter.c.\n\nI guess I should have been more clear since the -O1 and -Og locations\nare different. -O1 complains in filter_refs().\n\nBy the way, that function's handling of filter->kind seems very sketchy\nto me. It does:\n\n int ret = 0;\n if (!filter->kind)\n die(\"filter_refs: invalid type\");\n else {\n /*\n * For common cases where we need only branches or remotes or tags,\n * we only iterate through those refs. If a mix of refs is needed,\n * we iterate over all refs and filter out required refs with the help\n * of filter_ref_kind().\n */\n if (filter->kind == FILTER_REFS_BRANCHES)\n ret = for_each_fullref_in(\"refs/heads/\", ...);\n else if (filter->kind == FILTER_REFS_REMOTES)\n ret = for_each_fullref_in(\"refs/remotes/\", ...);\n else if (filter->kind == FILTER_REFS_TAGS)\n ret = for_each_fullref_in(\"refs/tags/\", ...);\n else if (filter->kind & FILTER_REFS_ALL)\n ret = for_each_fullref_in_pattern(filter, ...);\n if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))\n head_ref(...);\n }\n\nSo filter->kind is sometimes treated like a bitfield and sometimes not.\nI can set it to (ALL & DETACHED_HEAD) to get something useful, but not\n(BRANCHES & HEAD). The up-front check tries to complain if you didn't\nask for anything, but there are other flags like INCLUDE_BROKEN that\nwould cause \"!filter->kind\" to be false, but still not produce any\noutput.\n\nAnd shouldn't we be checking the return value of head_ref() like the\nothers?\n\nAll of this is outside the scope of our current discussion, and\nuntangling it might be messy (because it could touch the callers). I\njust wanted to document my findings for now. :)\n\n-Peff\n"},{"id":"394762","messageId":"20200404120743.GA636417@generichostname","threadId":"53150","inReplyTo":"20200403140447.GA639586@coredump.intra.peff.net","subject":"Re: [PATCH] Fix -Wmaybe-uninitialized warnings under -O0","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-04T12:07:43Z","receivedAt":"2020-04-04T12:07:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Peff,\n\nOn Fri, Apr 03, 2020 at 10:04:47AM -0400, Jeff King wrote:\n> On Wed, Apr 01, 2020 at 10:06:43AM -0400, Denton Liu wrote:\n> \n> > > So why does your version behave differently? And if this is a temporary\n> > > state for a buggy version of gcc (that may be fixed in the next point\n> > > release), is it worth changing our source code to appease it?\n> > \n> > A correction to the earlier message... It seems like I wasn't reporting\n> > the correct settings. I was actually compiling with -Og, not -O0\n> > (whoops!).\n> > \n> > I tested it with gcc-8 and it seems like it also reports the same\n> > problem. Also, -O1 reports warnings as well.\n> \n> Ah, OK, I can reproduce easily with -Og (up through gcc-10). Most of\n> them don't trigger with -O1; just the one in ref-filter.c.\n> \n> That one's interesting. We have:\n> \n> int ret = 0;\n> ...\n> if (...)\n> ...\n> else\n> ret = for_each_fullref_in_pattern(...);\n> ...\n> return ret;\n> \n> So we'd either have 0 or an assigned return. But the bug is actually in\n> for_each_fullref_in_pattern(), which does this:\n> \n> int ret; /* uninitialized! */\n> \n> /* a bunch of early return conditionals */\n> if (...)\n> return ...;\n> \n> for_each_string_list_item(...) {\n> ret = for_each_fullref_in(...);\n\nThis loop is missing a bit of important context:\n\n\tif (ret)\n\t\tbreak;\n\n> }\n> \n> return ret;\n> \n> but that will return an uninitialized value when there are no patterns.\n> I doubt we have such a case, but that may explain why -O0 does not\n> complain (it assumes \"in_pattern\" will return a useful value) and -O2\n> does not (it is able to figure out that it always does), but -O1 only\n> inlines part of it.\n> \n> Curiously, -Og _does_ find the correct function.\n> \n> Your patch silences it, but is it doing the right thing? It sets \"ret =\n> 0\", but we haven't actually iterated anything. Should it be an error\n> instead?\n\nI understood the semantics of for_each_fullref_in_pattern() (in the\nnon-early return case) to be \"for each item, iterate and compute a\nvalue; if that value is non-zero return it. If not found, return zero\".\nThe missing context above is important since, without it, we lose the\nsemantics.\n\nIf I'm understanding the above correctly then, studying this function in\na vacuum, it would make sense to assign a default value of 0 since if\nthe for operates on an empty list, the function should consider the item\nvacuously not found and we should return 0 as a result.\n\nThis was the type of analysis I applied to the other changes. I'll admit\nthat I studied the other functions in a vacuum as well since these\nseemed to be superficial warnings (since they aren't triggered with\n-O{0,2}) which indicated to me that the code was correct except for some\n\"cosmetic\" errors. I dunno, perhaps this is my inexperience showing\nthrough.\n\nThanks,\n\nDenton\n\nP.S. Do we want to quash the -O3 warnings as well?\n\n> -Peff\n"},{"id":"394776","messageId":"20200404142131.GA679473@coredump.intra.peff.net","threadId":"53150","inReplyTo":"20200404120743.GA636417@generichostname","subject":"Re: [PATCH] Fix -Wmaybe-uninitialized warnings under -O0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-04T14:21:31Z","receivedAt":"2020-04-04T14:21:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 04, 2020 at 08:07:43AM -0400, Denton Liu wrote:\n\n> > for_each_string_list_item(...) {\n> > ret = for_each_fullref_in(...);\n> \n> This loop is missing a bit of important context:\n> \n> \tif (ret)\n> \t\tbreak;\n> \n\nYes, I omitted it because it's not relevant to whether ret is\ninitialized or not (i.e., if we enter the loop then it always is.\nBut I think it is to the argument you make below.\n\n> > Your patch silences it, but is it doing the right thing? It sets \"ret =\n> > 0\", but we haven't actually iterated anything. Should it be an error\n> > instead?\n> \n> I understood the semantics of for_each_fullref_in_pattern() (in the\n> non-early return case) to be \"for each item, iterate and compute a\n> value; if that value is non-zero return it. If not found, return zero\".\n> The missing context above is important since, without it, we lose the\n> semantics.\n> \n> If I'm understanding the above correctly then, studying this function in\n> a vacuum, it would make sense to assign a default value of 0 since if\n> the for operates on an empty list, the function should consider the item\n> vacuously not found and we should return 0 as a result.\n\nI think the break on \"ret\" is better understood as \"if we saw an error,\nreturn it; otherwise keep going\".\n\nIf we were given zero patterns, is that a noop success, or is it an\nerror? This should never happen because we cover that case earlier, so\nit would be a bug in find_longest_prefixes. Perhaps:\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex b1812cb69a..b358567663 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1964,6 +1964,8 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,\n \t}\n \n \tfind_longest_prefixes(&prefixes, filter->name_patterns);\n+\tif (!prefixes.nr)\n+\t\tBUG(\"no common ref prefix?\");\n \n \tfor_each_string_list_item(prefix, &prefixes) {\n \t\tret = for_each_fullref_in(prefix->string, cb, cb_data, broken);\n\nis worth doing to document that assumption. Ideally that would then let\nthe compiler know that we'll always enter the loop, but it doesn't seem\nto be quite the clever.\n\nSo I dunno. I think it probably doesn't matter between \"0\" and \"-1\",\nbecause this case really shouldn't be reachable.\n\n> This was the type of analysis I applied to the other changes. I'll admit\n> that I studied the other functions in a vacuum as well since these\n> seemed to be superficial warnings (since they aren't triggered with\n> -O{0,2}) which indicated to me that the code was correct except for some\n> \"cosmetic\" errors. I dunno, perhaps this is my inexperience showing\n> through.\n\nYeah, the code is correct in this case, and I don't think the\nuninitialized case can be reached. But the subtle linkage here is that\npatterns[0] being non-NULL means that find_longest_prefixes() will never\nreturn a zero-length list, which means we'll always enter that loop at\nleast once.\n\n> P.S. Do we want to quash the -O3 warnings as well?\n\nThey're probably worth looking at. I've periodically swept through and\nfixed them, as recently as last September (try \"git log --grep=-O3\").\nBut new ones seem to have cropped up. I'm not sure if they were\nintroduced in the code or if the compiler got smarter (or dumber).\n\nJust skimming the output, I see:\n\n In function ‘ll_binary_merge’,\n inlined from ‘ll_xdl_merge’ at ll-merge.c:115:10,\n inlined from ‘ll_union_merge’ at ll-merge.c:151:9:\n ll-merge.c:74:4: error: ‘%s’ directive argument is null [-Werror=format-overflow=]\n 74 | warning(\"Cannot merge binary files: %s (%s vs. %s)\",\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n 75 | path, name1, name2);\n | ~~~~~~~~~~~~~~~~~~~\n\nThis one seems likely to be accurate (and just uncovered by more\naggressive inlining). The union_merge function passes in NULL filenames,\nand probably could trigger this warning on binary input.\n\n trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.0’:\n trace2/tr2_dst.c:296:10: error: ‘fd’ may be used uninitialized in this function [-Werror=maybe-uninitialized]\n 296 | dst->fd = fd;\n | ~~~~~~~~^~~~\n trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here\n 229 | int fd;\n | ^~\n\nThis one looks like a false positive. The tr2 code does something\nunusual for our code base: on error it returns errno. And I think the\ncompiler is not aware that errno would not be zero after a syscall\nfailure. It might be worth changing the return value to match our usual\npattern (return -1, and the caller can look in errno), which would\nappease the compiler as a side effect.\n\n revision.c: In function ‘do_add_index_objects_to_pending’:\n revision.c:322:22: error: array subscript [1, 2147483647] is outside array bounds of ‘char[1]’ [-Werror=array-bounds]\n 322 | if (0 < len && name[len] && buf.len)\n | ~~~~^~~~~\n\nThis one is confusing. I imagine the char[1] is the empty string we\nfrequently pass to add_pending_object() and friends. And the \"len\" here\ncomes from interpret_branch_name(name, ...). So somehow the compiler\ndoesn't quite know that the length we'll return is going to be less than\nor equal to the string length we pass in.\n\nI don't know how to fix that aside from this, which isn't great (btw, it\nlooks like a lot of this code could stand to switch to using ssize_t\ninstead of int; there are probably weird errors if you managed to\nsomehow feed a 2GB branch name).\n\ndiff --git a/revision.c b/revision.c\nindex 8136929e23..9bc398bec1 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -317,9 +317,10 @@ static void add_pending_object_with_path(struct rev_info *revs,\n \t\trevs->no_walk = 0;\n \tif (revs->reflog_info && obj->type == OBJ_COMMIT) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint len = interpret_branch_name(name, 0, &buf, 0);\n+\t\tsize_t namelen = strlen(name);\n+\t\tint len = interpret_branch_name(name, namelen, &buf, 0);\n \n-\t\tif (0 < len && name[len] && buf.len)\n+\t\tif (len == namelen && buf.len)\n \t\t\tstrbuf_addstr(&buf, name + len);\n \t\tadd_reflog_for_walk(revs->reflog_info,\n \t\t\t\t (struct commit *)obj,\n\n-Peff\n"},{"id":"423647","messageId":"patch-1.1-87d9bcf1095-20210505T083951Z-avarab@gmail.com","threadId":"53150","inReplyTo":"20200404142131.GA679473@coredump.intra.peff.net","subject":"[PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-05T08:40:32Z","receivedAt":"2021-05-05T08:40:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that\nappears under -O3 (but not -O2). This makes the build pass under\nDEVELOPER=1 without needing a DEVOPTS=no-error.\n\nThis can be reproduced with GCC Debian 8.3.0-6, but not e.g. with\nclang 7.0.1-8+deb10u2. We've had this warning since\nee4512ed481 (trace2: create new combined trace facility, 2019-02-22).\n\nAs noted in [2] this warning happens because the compiler doesn't\nassume that errno must be non-zero after a failed syscall. Let's work\naround it as suggested in that analysis. We now return -1 ourselves on\nerror, and save away the value of errno in a variable the caller\npasses in.\n\n1.\n\n trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’:\n trace2/tr2_dst.c:296:10: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n dst->fd = fd;\n ~~~~~~~~^~~~\n trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here\n int fd;\n ^~\n2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/\n---\n trace2/tr2_dst.c | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\nindex ae052a07fe2..c2aba71041b 100644\n--- a/trace2/tr2_dst.c\n+++ b/trace2/tr2_dst.c\n@@ -197,22 +197,25 @@ static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value)\n #define PREFIX_AF_UNIX_STREAM \"af_unix:stream:\"\n #define PREFIX_AF_UNIX_DGRAM \"af_unix:dgram:\"\n \n-static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)\n+static int tr2_dst_try_uds_connect(const char *path, int sock_type,\n+\t\t\t\t int *out_fd, int *saved_errno)\n {\n \tint fd;\n \tstruct sockaddr_un sa;\n \n \tfd = socket(AF_UNIX, sock_type, 0);\n-\tif (fd == -1)\n-\t\treturn errno;\n+\tif (fd == -1) {\n+\t\t*saved_errno = errno;\n+\t\treturn -1;\n+\t}\n \n \tsa.sun_family = AF_UNIX;\n \tstrlcpy(sa.sun_path, path, sizeof(sa.sun_path));\n \n \tif (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {\n-\t\tint e = errno;\n+\t\t*saved_errno = errno;\n \t\tclose(fd);\n-\t\treturn e;\n+\t\treturn -1;\n \t}\n \n \t*out_fd = fd;\n@@ -227,7 +230,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n {\n \tunsigned int uds_try = 0;\n \tint fd;\n-\tint e;\n+\tint saved_errno;\n \tconst char *path = NULL;\n \n \t/*\n@@ -271,15 +274,15 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n \t}\n \n \tif (uds_try & TR2_DST_UDS_TRY_STREAM) {\n-\t\te = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd);\n-\t\tif (!e)\n+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd,\n+\t\t\t\t\t &saved_errno))\n \t\t\tgoto connected;\n-\t\tif (e != EPROTOTYPE)\n+\t\tif (saved_errno != EPROTOTYPE)\n \t\t\tgoto error;\n \t}\n \tif (uds_try & TR2_DST_UDS_TRY_DGRAM) {\n-\t\te = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd);\n-\t\tif (!e)\n+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd,\n+\t\t\t\t\t &saved_errno))\n \t\t\tgoto connected;\n \t}\n \n@@ -287,7 +290,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n \tif (tr2_dst_want_warning())\n \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n-\t\t\tstrerror(e));\n+\t\t\tstrerror(saved_errno));\n \n \ttr2_dst_trace_disable(dst);\n \treturn 0;\n-- \n2.31.1.745.g2af7c6593ce\n\n"},{"id":"423650","messageId":"xmqqwnsd3426.fsf@gitster.g","threadId":"53150","inReplyTo":"patch-1.1-87d9bcf1095-20210505T083951Z-avarab@gmail.com","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-05T09:47:29Z","receivedAt":"2021-05-05T09:47:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that\n> appears under -O3 (but not -O2). This makes the build pass under\n> DEVELOPER=1 without needing a DEVOPTS=no-error.\n>\n> This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with\n> clang 7.0.1-8+deb10u2. We've had this warning since\n> ee4512ed481 (trace2: create new combined trace facility, 2019-02-22).\n>\n> As noted in [2] this warning happens because the compiler doesn't\n> assume that errno must be non-zero after a failed syscall. Let's work\n> around it as suggested in that analysis. We now return -1 ourselves on\n> error, and save away the value of errno in a variable the caller\n> passes in.\n>\n> 1.\n>\n> trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’:\n> trace2/tr2_dst.c:296:10: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n> dst->fd = fd;\n> ~~~~~~~~^~~~\n> trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here\n> int fd;\n> ^~\n> 2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/\n> ---\n> trace2/tr2_dst.c | 27 +++++++++++++++------------\n> 1 file changed, 15 insertions(+), 12 deletions(-)\n\nThe patch makes sense to me, modulo that the way the variable\n\"saved_errno\" introduced by this patch is used and the way a\nvariable with that name is typically used in our codebase are at\nodds. I.e. we typically call a variable \"saved_errno\" when it is\nused in this pattern:\n\n if (a_syscall_whose_error_condition_we_care_about()) {\n\tint saved_errno = errno;\n\tperform_some_cleanup_operation_that_might_clobber_errno();\n\treturn error_errno(..., saved_errno);\n\t/*\n\t * or\n\t * errno = saved_errno;\n\t * return -1;\n\t * and let the caller handle 'errno'\n\t */\n }\n\nBut since I do not think of a better name for this new variable that\nis not exactly used like so, let's queue it as-is.\n\nThanks.\n\n> diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\n> index ae052a07fe2..c2aba71041b 100644\n> --- a/trace2/tr2_dst.c\n> +++ b/trace2/tr2_dst.c\n> @@ -197,22 +197,25 @@ static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value)\n> #define PREFIX_AF_UNIX_STREAM \"af_unix:stream:\"\n> #define PREFIX_AF_UNIX_DGRAM \"af_unix:dgram:\"\n> \n> -static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)\n> +static int tr2_dst_try_uds_connect(const char *path, int sock_type,\n> +\t\t\t\t int *out_fd, int *saved_errno)\n> {\n> \tint fd;\n> \tstruct sockaddr_un sa;\n> \n> \tfd = socket(AF_UNIX, sock_type, 0);\n> -\tif (fd == -1)\n> -\t\treturn errno;\n> +\tif (fd == -1) {\n> +\t\t*saved_errno = errno;\n> +\t\treturn -1;\n> +\t}\n> \n> \tsa.sun_family = AF_UNIX;\n> \tstrlcpy(sa.sun_path, path, sizeof(sa.sun_path));\n> \n> \tif (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {\n> -\t\tint e = errno;\n> +\t\t*saved_errno = errno;\n> \t\tclose(fd);\n> -\t\treturn e;\n> +\t\treturn -1;\n> \t}\n> \n> \t*out_fd = fd;\n> @@ -227,7 +230,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n> {\n> \tunsigned int uds_try = 0;\n> \tint fd;\n> -\tint e;\n> +\tint saved_errno;\n> \tconst char *path = NULL;\n> \n> \t/*\n> @@ -271,15 +274,15 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n> \t}\n> \n> \tif (uds_try & TR2_DST_UDS_TRY_STREAM) {\n> -\t\te = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd);\n> -\t\tif (!e)\n> +\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd,\n> +\t\t\t\t\t &saved_errno))\n> \t\t\tgoto connected;\n> -\t\tif (e != EPROTOTYPE)\n> +\t\tif (saved_errno != EPROTOTYPE)\n> \t\t\tgoto error;\n> \t}\n> \tif (uds_try & TR2_DST_UDS_TRY_DGRAM) {\n> -\t\te = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd);\n> -\t\tif (!e)\n> +\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd,\n> +\t\t\t\t\t &saved_errno))\n> \t\t\tgoto connected;\n> \t}\n> \n> @@ -287,7 +290,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n> \tif (tr2_dst_want_warning())\n> \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n> \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n> -\t\t\tstrerror(e));\n> +\t\t\tstrerror(saved_errno));\n> \n> \ttr2_dst_trace_disable(dst);\n> \treturn 0;\n"},{"id":"423673","messageId":"YJKe+ncDqrqq1i8P@coredump.intra.peff.net","threadId":"53150","inReplyTo":"xmqqwnsd3426.fsf@gitster.g","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-05T13:34:50Z","receivedAt":"2021-05-05T13:34:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 05, 2021 at 06:47:29PM +0900, Junio C Hamano wrote:\n\n> The patch makes sense to me, modulo that the way the variable\n> \"saved_errno\" introduced by this patch is used and the way a\n> variable with that name is typically used in our codebase are at\n> odds. I.e. we typically call a variable \"saved_errno\" when it is\n> used in this pattern:\n> \n> if (a_syscall_whose_error_condition_we_care_about()) {\n> \tint saved_errno = errno;\n> \tperform_some_cleanup_operation_that_might_clobber_errno();\n> \treturn error_errno(..., saved_errno);\n> \t/*\n> \t * or\n> \t * errno = saved_errno;\n> \t * return -1;\n> \t * and let the caller handle 'errno'\n> \t */\n> }\n> \n> But since I do not think of a better name for this new variable that\n> is not exactly used like so, let's queue it as-is.\n\nI'd probably have just called it \"err\", but I think it is fine either\nway. :)\n\nThe patch also looks good to me. I used to compile with -O3 occasionally\nto fix warnings, but given the date on this commit, it seems I have not\ndone so in quite a while. (It reproduces on gcc 10 for me, which is not\nsurprising).\n\n-Peff\n"},{"id":"423689","messageId":"nycvar.QRO.7.76.6.2105051623230.50@tvgsbejvaqbjf.bet","threadId":"53150","inReplyTo":"patch-1.1-87d9bcf1095-20210505T083951Z-avarab@gmail.com","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-05-05T14:38:42Z","receivedAt":"2021-05-05T14:39:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Wed, 5 May 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that\n> appears under -O3 (but not -O2). This makes the build pass under\n> DEVELOPER=1 without needing a DEVOPTS=no-error.\n>\n> This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with\n> clang 7.0.1-8+deb10u2. We've had this warning since\n> ee4512ed481 (trace2: create new combined trace facility, 2019-02-22).\n>\n> As noted in [2] this warning happens because the compiler doesn't\n> assume that errno must be non-zero after a failed syscall. Let's work\n> around it as suggested in that analysis. We now return -1 ourselves on\n> error, and save away the value of errno in a variable the caller\n> passes in.\n\nIt would probably be a lot nicer if you lead with this insight. I could\nimagine, for example, that a oneline like this would be much more helpful\nto any reader:\n\n\ttrace2: do not assume errno != 0 after a failed syscall\n\nThe first two paragraphs are less interesting than the third paragraph,\ntoo, therefore I would recommend\n\nAbout the patch...\n\n>\n> 1.\n>\n> trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’:\n> trace2/tr2_dst.c:296:10: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n> dst->fd = fd;\n> ~~~~~~~~^~~~\n> trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here\n> int fd;\n> ^~\n> 2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/\n> ---\n> trace2/tr2_dst.c | 27 +++++++++++++++------------\n> 1 file changed, 15 insertions(+), 12 deletions(-)\n>\n> diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\n> index ae052a07fe2..c2aba71041b 100644\n> --- a/trace2/tr2_dst.c\n> +++ b/trace2/tr2_dst.c\n> @@ -197,22 +197,25 @@ static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value)\n> #define PREFIX_AF_UNIX_STREAM \"af_unix:stream:\"\n> #define PREFIX_AF_UNIX_DGRAM \"af_unix:dgram:\"\n>\n> -static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)\n> +static int tr2_dst_try_uds_connect(const char *path, int sock_type,\n> +\t\t\t\t int *out_fd, int *saved_errno)\n> {\n> \tint fd;\n> \tstruct sockaddr_un sa;\n>\n> \tfd = socket(AF_UNIX, sock_type, 0);\n> -\tif (fd == -1)\n> -\t\treturn errno;\n> +\tif (fd == -1) {\n> +\t\t*saved_errno = errno;\n> +\t\treturn -1;\n> +\t}\n\nI don't think this is necessary. My manual page for socket(2) says this:\n\n\tRETURN VALUE\n\t\tIf the connection or binding succeeds, zero is returned.\n\t\tOn error, -1 is returned, and errno is set appropriately.\n\n> \tsa.sun_family = AF_UNIX;\n> \tstrlcpy(sa.sun_path, path, sizeof(sa.sun_path));\n>\n> \tif (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {\n> -\t\tint e = errno;\n> +\t\t*saved_errno = errno;\n> \t\tclose(fd);\n> -\t\treturn e;\n> +\t\treturn -1;\n\nLikewise, my manual page for connect(2) says the same as for socket(2):\nupon return value -1, errno is set appropriately (i.e. non-zero).\n\nTherefore, I would say this patch is actually only papering over an\noverzealous (and incorrect) compiler warning.\n\nIf you _must_ shut up GCC, a better idea would be a much less intrusive,\neasier to read\n\n\t\t/* GCC thinks socket()/connect() might fail to set errno */\n\t\treturn errno ? errno : EIO;\n\nCiao,\nDscho\n\n> \t}\n>\n> \t*out_fd = fd;\n> @@ -227,7 +230,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n> {\n> \tunsigned int uds_try = 0;\n> \tint fd;\n> -\tint e;\n> +\tint saved_errno;\n> \tconst char *path = NULL;\n>\n> \t/*\n> @@ -271,15 +274,15 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n> \t}\n>\n> \tif (uds_try & TR2_DST_UDS_TRY_STREAM) {\n> -\t\te = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd);\n> -\t\tif (!e)\n> +\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd,\n> +\t\t\t\t\t &saved_errno))\n> \t\t\tgoto connected;\n> -\t\tif (e != EPROTOTYPE)\n> +\t\tif (saved_errno != EPROTOTYPE)\n> \t\t\tgoto error;\n> \t}\n> \tif (uds_try & TR2_DST_UDS_TRY_DGRAM) {\n> -\t\te = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd);\n> -\t\tif (!e)\n> +\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd,\n> +\t\t\t\t\t &saved_errno))\n> \t\t\tgoto connected;\n> \t}\n>\n> @@ -287,7 +290,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n> \tif (tr2_dst_want_warning())\n> \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n> \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n> -\t\t\tstrerror(e));\n> +\t\t\tstrerror(saved_errno));\n>\n> \ttr2_dst_trace_disable(dst);\n> \treturn 0;\n> --\n> 2.31.1.745.g2af7c6593ce\n>\n>\n"},{"id":"423728","messageId":"xmqqv97w1wl1.fsf@gitster.g","threadId":"53150","inReplyTo":"nycvar.QRO.7.76.6.2105051623230.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-06T01:26:34Z","receivedAt":"2021-05-06T01:26:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Ævar,\n>\n> On Wed, 5 May 2021, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that\n>> appears under -O3 (but not -O2). This makes the build pass under\n>> DEVELOPER=1 without needing a DEVOPTS=no-error.\n>>\n>> This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with\n>> clang 7.0.1-8+deb10u2. We've had this warning since\n>> ee4512ed481 (trace2: create new combined trace facility, 2019-02-22).\n>>\n>> As noted in [2] this warning happens because the compiler doesn't\n>> assume that errno must be non-zero after a failed syscall. Let's work\n>> around it as suggested in that analysis. We now return -1 ourselves on\n>> error, and save away the value of errno in a variable the caller\n>> passes in.\n>\n> It would probably be a lot nicer if you lead with this insight. I could\n> imagine, for example, that a oneline like this would be much more helpful\n> to any reader:\n>\n> \ttrace2: do not assume errno != 0 after a failed syscall\n\nBut that is misleading. \n\nMy understanding is that this patch is about working around\ncompilers that do not know that a failed syscall means errno would\nbe set to non-zero. Am I mistaken?\n\nOtherwise I'd strongly prefer to see a word that hints that this is\nan otherwise unneeded workaround for comiplers. Your suggested\ntitle instead hints that it is wrong to assume that errno will be\nset to non-zero after a syscall. I do not think that is the message\nwe want to send to our readers.\n\n\n"},{"id":"423816","messageId":"nycvar.QRO.7.76.6.2105062229270.50@tvgsbejvaqbjf.bet","threadId":"53150","inReplyTo":"xmqqv97w1wl1.fsf@gitster.g","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-05-06T20:29:57Z","receivedAt":"2021-05-06T20:30:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 6 May 2021, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > Hi Ævar,\n> >\n> > On Wed, 5 May 2021, Ævar Arnfjörð Bjarmason wrote:\n> >\n> >> Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that\n> >> appears under -O3 (but not -O2). This makes the build pass under\n> >> DEVELOPER=1 without needing a DEVOPTS=no-error.\n> >>\n> >> This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with\n> >> clang 7.0.1-8+deb10u2. We've had this warning since\n> >> ee4512ed481 (trace2: create new combined trace facility, 2019-02-22).\n> >>\n> >> As noted in [2] this warning happens because the compiler doesn't\n> >> assume that errno must be non-zero after a failed syscall. Let's work\n> >> around it as suggested in that analysis. We now return -1 ourselves on\n> >> error, and save away the value of errno in a variable the caller\n> >> passes in.\n> >\n> > It would probably be a lot nicer if you lead with this insight. I could\n> > imagine, for example, that a oneline like this would be much more helpful\n> > to any reader:\n> >\n> > \ttrace2: do not assume errno != 0 after a failed syscall\n>\n> But that is misleading.\n>\n> My understanding is that this patch is about working around\n> compilers that do not know that a failed syscall means errno would\n> be set to non-zero. Am I mistaken?\n>\n> Otherwise I'd strongly prefer to see a word that hints that this is\n> an otherwise unneeded workaround for comiplers. Your suggested\n> title instead hints that it is wrong to assume that errno will be\n> set to non-zero after a syscall. I do not think that is the message\n> we want to send to our readers.\n\nRight, the oneline I suggested was only for the original patch, with which\nI disagreed.\n\nCiao,\nDscho\n"},{"id":"423819","messageId":"xmqqh7jfy3du.fsf@gitster.g","threadId":"53150","inReplyTo":"nycvar.QRO.7.76.6.2105062229270.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-06T21:10:53Z","receivedAt":"2021-05-06T21:11:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Otherwise I'd strongly prefer to see a word that hints that this is\n>> an otherwise unneeded workaround for comiplers. Your suggested\n>> title instead hints that it is wrong to assume that errno will be\n>> set to non-zero after a syscall. I do not think that is the message\n>> we want to send to our readers.\n>\n> Right, the oneline I suggested was only for the original patch, with which\n> I disagreed.\n\nI actually do not know how your rewrite could be better, though.\n\n\t\t/* GCC thinks socket()/connect() might fail to set errno */\n\t\treturn errno ? errno : EIO;\n\nIf a compiler thinks errno may *not* be set, can 'errno' be reliably\nused to decide if it can be returned as-is or a fallback value EIO\nshould be used, without triggering the same (incorrect) working in\nthe first place?\n"},{"id":"424114","messageId":"xmqqv97pn454.fsf@gitster.g","threadId":"53150","inReplyTo":"patch-1.1-87d9bcf1095-20210505T083951Z-avarab@gmail.com","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-11T07:03:35Z","receivedAt":"2021-05-11T07:03:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that\n> appears under -O3 (but not -O2). This makes the build pass under\n> DEVELOPER=1 without needing a DEVOPTS=no-error.\n>\n> This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with\n> clang 7.0.1-8+deb10u2. We've had this warning since\n> ee4512ed481 (trace2: create new combined trace facility, 2019-02-22).\n>\n> As noted in [2] this warning happens because the compiler doesn't\n> assume that errno must be non-zero after a failed syscall. Let's work\n> around it as suggested in that analysis. We now return -1 ourselves on\n> error, and save away the value of errno in a variable the caller\n> passes in.\n>\n> 1.\n>\n> trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’:\n> trace2/tr2_dst.c:296:10: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n> dst->fd = fd;\n> ~~~~~~~~^~~~\n> trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here\n> int fd;\n> ^~\n> 2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/\n> ---\n> trace2/tr2_dst.c | 27 +++++++++++++++------------\n> 1 file changed, 15 insertions(+), 12 deletions(-)\n\nWhat's the concensus if any on this topic?\n\nIn any case, this needs to be signed off before it gets carved into\nour history.\n\nThanks.\n"},{"id":"424127","messageId":"patch-1.1-782555daade-20210511T130231Z-avarab@gmail.com","threadId":"53150","inReplyTo":"patch-1.1-87d9bcf1095-20210505T083951Z-avarab@gmail.com","subject":"[PATCH v2] trace2: refactor to avoid gcc warning under -O3","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-11T13:04:28Z","receivedAt":"2021-05-11T13:04:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that\nappears under -O3 (but not -O2). This makes the build pass under\nDEVELOPER=1 without needing a DEVOPTS=no-error.\n\nThis can be reproduced with GCC Debian 8.3.0-6, but not e.g. with\nclang 7.0.1-8+deb10u2. We've had this warning since\nee4512ed481 (trace2: create new combined trace facility, 2019-02-22).\n\nAs noted in [2] this warning happens because the compiler doesn't\nassume that errno must be non-zero after a failed syscall. Let's work\naround it as suggested in that analysis. We now return -1 ourselves on\nerror, and save away the value of errno in a variable the caller\npasses in.\n\nSee https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61846 for a related\nbug report against GCC.\n\n1.\n\n trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’:\n trace2/tr2_dst.c:296:10: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n dst->fd = fd;\n ~~~~~~~~^~~~\n trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here\n int fd;\n ^~\n2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nOn Tue, May 11 2021, Junio C Hamano wrote:\n\n> What's the concensus if any on this topic?\n\nHaving read Johannes's comments I think it's still most readable to\njust return -1 unconditionally. The resulting code isn't weird, I'd\nargue that it's a better pattern to save away errno like this, but the\ncommit messages notes that we're working around a GCC bug.\n\n> In any case, this needs to be signed off before it gets carved into\n> our history.\n\nDone, and also changed the variable name to minimize the size of the\ndiff. A shorter name allowed for less re-flowing of lines.\n\nRange-diff against v1:\n1: 87d9bcf1095 ! 1: 782555daade trace2: refactor to avoid gcc warning under -O3\n @@ Commit message\n error, and save away the value of errno in a variable the caller\n passes in.\n \n + See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61846 for a related\n + bug report against GCC.\n +\n 1.\n \n trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’:\n @@ Commit message\n ^~\n 2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/\n \n + Signed-off-by: Ævar Arnfjörð Bjarmason \n +\n ## trace2/tr2_dst.c ##\n @@ trace2/tr2_dst.c: static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value)\n #define PREFIX_AF_UNIX_STREAM \"af_unix:stream:\"\n @@ trace2/tr2_dst.c: static int tr2_dst_try_path(struct tr2_dst *dst, const char *t\n \n -static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)\n +static int tr2_dst_try_uds_connect(const char *path, int sock_type,\n -+\t\t\t\t int *out_fd, int *saved_errno)\n ++\t\t\t\t int *out_fd, int *e)\n {\n \tint fd;\n \tstruct sockaddr_un sa;\n @@ trace2/tr2_dst.c: static int tr2_dst_try_path(struct tr2_dst *dst, const char *t\n -\tif (fd == -1)\n -\t\treturn errno;\n +\tif (fd == -1) {\n -+\t\t*saved_errno = errno;\n ++\t\t*e = errno;\n +\t\treturn -1;\n +\t}\n \n @@ trace2/tr2_dst.c: static int tr2_dst_try_path(struct tr2_dst *dst, const char *t\n \n \tif (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {\n -\t\tint e = errno;\n -+\t\t*saved_errno = errno;\n ++\t\t*e = errno;\n \t\tclose(fd);\n -\t\treturn e;\n +\t\treturn -1;\n \t}\n \n \t*out_fd = fd;\n -@@ trace2/tr2_dst.c: static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n - {\n - \tunsigned int uds_try = 0;\n - \tint fd;\n --\tint e;\n -+\tint saved_errno;\n - \tconst char *path = NULL;\n - \n - \t/*\n @@ trace2/tr2_dst.c: static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n \t}\n \n \tif (uds_try & TR2_DST_UDS_TRY_STREAM) {\n -\t\te = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd);\n -\t\tif (!e)\n -+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd,\n -+\t\t\t\t\t &saved_errno))\n ++\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd, &e))\n \t\t\tgoto connected;\n --\t\tif (e != EPROTOTYPE)\n -+\t\tif (saved_errno != EPROTOTYPE)\n + \t\tif (e != EPROTOTYPE)\n \t\t\tgoto error;\n \t}\n \tif (uds_try & TR2_DST_UDS_TRY_DGRAM) {\n -\t\te = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd);\n -\t\tif (!e)\n -+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd,\n -+\t\t\t\t\t &saved_errno))\n ++\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd, &e))\n \t\t\tgoto connected;\n \t}\n \n -@@ trace2/tr2_dst.c: static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n - \tif (tr2_dst_want_warning())\n - \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n - \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n --\t\t\tstrerror(e));\n -+\t\t\tstrerror(saved_errno));\n - \n - \ttr2_dst_trace_disable(dst);\n - \treturn 0;\n\n trace2/tr2_dst.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\nindex ae052a07fe2..a44fe6b73e0 100644\n--- a/trace2/tr2_dst.c\n+++ b/trace2/tr2_dst.c\n@@ -197,22 +197,25 @@ static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value)\n #define PREFIX_AF_UNIX_STREAM \"af_unix:stream:\"\n #define PREFIX_AF_UNIX_DGRAM \"af_unix:dgram:\"\n \n-static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)\n+static int tr2_dst_try_uds_connect(const char *path, int sock_type,\n+\t\t\t\t int *out_fd, int *e)\n {\n \tint fd;\n \tstruct sockaddr_un sa;\n \n \tfd = socket(AF_UNIX, sock_type, 0);\n-\tif (fd == -1)\n-\t\treturn errno;\n+\tif (fd == -1) {\n+\t\t*e = errno;\n+\t\treturn -1;\n+\t}\n \n \tsa.sun_family = AF_UNIX;\n \tstrlcpy(sa.sun_path, path, sizeof(sa.sun_path));\n \n \tif (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {\n-\t\tint e = errno;\n+\t\t*e = errno;\n \t\tclose(fd);\n-\t\treturn e;\n+\t\treturn -1;\n \t}\n \n \t*out_fd = fd;\n@@ -271,15 +274,13 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n \t}\n \n \tif (uds_try & TR2_DST_UDS_TRY_STREAM) {\n-\t\te = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd);\n-\t\tif (!e)\n+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd, &e))\n \t\t\tgoto connected;\n \t\tif (e != EPROTOTYPE)\n \t\t\tgoto error;\n \t}\n \tif (uds_try & TR2_DST_UDS_TRY_DGRAM) {\n-\t\te = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd);\n-\t\tif (!e)\n+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd, &e))\n \t\t\tgoto connected;\n \t}\n \n-- \n2.31.1.909.g789bb6d90e\n\n"},{"id":"424135","messageId":"nycvar.QRO.7.76.6.2105111632380.57@tvgsbejvaqbjf.bet","threadId":"53150","inReplyTo":"xmqqh7jfy3du.fsf@gitster.g","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-05-11T14:34:49Z","receivedAt":"2021-05-11T14:35:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 7 May 2021, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> >> Otherwise I'd strongly prefer to see a word that hints that this is\n> >> an otherwise unneeded workaround for comiplers. Your suggested\n> >> title instead hints that it is wrong to assume that errno will be\n> >> set to non-zero after a syscall. I do not think that is the message\n> >> we want to send to our readers.\n> >\n> > Right, the oneline I suggested was only for the original patch, with which\n> > I disagreed.\n>\n> I actually do not know how your rewrite could be better, though.\n>\n> \t\t/* GCC thinks socket()/connect() might fail to set errno */\n> \t\treturn errno ? errno : EIO;\n>\n> If a compiler thinks errno may *not* be set, can 'errno' be reliably\n> used to decide if it can be returned as-is or a fallback value EIO\n> should be used, without triggering the same (incorrect) working in\n> the first place?\n\nOh, I guess I mistook the problem for something else, then.\n\nIf the problem is that `errno` is not set in case of failure, the\nresolution is easy (and even recommended in the manual page of `errno`):\nsimply set it to 0 before the syscall (or in the function that relies on\n`errno == 0` means success).\n\nThere is really no need to introduce a `saved_errno` variable (which to me\nwould suggest that we need to save the current value of `errno` and\nreinstate it later, as we do sometimes e.g. when we call `close()` after\nnoticing an error whose `errno` we need to preserve for the caller).\n\nCiao,\nDscho\n"},{"id":"424143","messageId":"c595d021-5fc2-ded7-d462-51ce7495739b@jeffhostetler.com","threadId":"53150","inReplyTo":"patch-1.1-782555daade-20210511T130231Z-avarab@gmail.com","subject":"Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-05-11T16:40:01Z","receivedAt":"2021-05-11T16:40:06Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/11/21 9:04 AM, Ævar Arnfjörð Bjarmason wrote:\n> Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that\n> appears under -O3 (but not -O2). This makes the build pass under\n> DEVELOPER=1 without needing a DEVOPTS=no-error.\n> \n...\n\n\nI suppose if we really need to paper around a compiler bug,\nthen this fix LGTM.\n\nThanks,\nJeff\n"},{"id":"424151","messageId":"YJrEy8IPi+PFWuzT@coredump.intra.peff.net","threadId":"53150","inReplyTo":"patch-1.1-782555daade-20210511T130231Z-avarab@gmail.com","subject":"Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-11T17:54:19Z","receivedAt":"2021-05-11T17:54:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 11, 2021 at 03:04:28PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that\n> appears under -O3 (but not -O2). This makes the build pass under\n> DEVELOPER=1 without needing a DEVOPTS=no-error.\n> \n> This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with\n> clang 7.0.1-8+deb10u2. We've had this warning since\n> ee4512ed481 (trace2: create new combined trace facility, 2019-02-22).\n> \n> As noted in [2] this warning happens because the compiler doesn't\n> assume that errno must be non-zero after a failed syscall. Let's work\n> around it as suggested in that analysis. We now return -1 ourselves on\n> error, and save away the value of errno in a variable the caller\n> passes in.\n\nThanks, I think this describes the problem nicely.\n\n> On Tue, May 11 2021, Junio C Hamano wrote:\n> \n> > What's the concensus if any on this topic?\n> \n> Having read Johannes's comments I think it's still most readable to\n> just return -1 unconditionally. The resulting code isn't weird, I'd\n> argue that it's a better pattern to save away errno like this, but the\n> commit messages notes that we're working around a GCC bug.\n\nAgreed. Returning \"-1\" is the usual style in our code base. And while I\nthink the original code is correct, I did have to go double-check the C\nstandard to confirm that it's so.\n\nI slightly disagree with the notion that gcc's behavior is a bug. It\nseems more like a lack of feature (it does not have any way to annotate\nthis special property of errno). But that is neither here nor there for\nyour patch, and really a matter of opinion. :)\n\n> > In any case, this needs to be signed off before it gets carved into\n> > our history.\n> \n> Done, and also changed the variable name to minimize the size of the\n> diff. A shorter name allowed for less re-flowing of lines.\n\nIt's quite short. I'm OK with it for a static-local function with few\ncallers like this, though.\n\n-Peff\n"},{"id":"424153","messageId":"YJrGOqjS10Z/WkE8@coredump.intra.peff.net","threadId":"53150","inReplyTo":"nycvar.QRO.7.76.6.2105111632380.57@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-11T18:00:26Z","receivedAt":"2021-05-11T18:00:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 11, 2021 at 04:34:49PM +0200, Johannes Schindelin wrote:\n\n> On Fri, 7 May 2021, Junio C Hamano wrote:\n> \n> > Johannes Schindelin writes:\n> >\n> > >> Otherwise I'd strongly prefer to see a word that hints that this is\n> > >> an otherwise unneeded workaround for comiplers. Your suggested\n> > >> title instead hints that it is wrong to assume that errno will be\n> > >> set to non-zero after a syscall. I do not think that is the message\n> > >> we want to send to our readers.\n> > >\n> > > Right, the oneline I suggested was only for the original patch, with which\n> > > I disagreed.\n> >\n> > I actually do not know how your rewrite could be better, though.\n> >\n> > \t\t/* GCC thinks socket()/connect() might fail to set errno */\n> > \t\treturn errno ? errno : EIO;\n> >\n> > If a compiler thinks errno may *not* be set, can 'errno' be reliably\n> > used to decide if it can be returned as-is or a fallback value EIO\n> > should be used, without triggering the same (incorrect) working in\n> > the first place?\n> \n> Oh, I guess I mistook the problem for something else, then.\n> \n> If the problem is that `errno` is not set in case of failure, the\n> resolution is easy (and even recommended in the manual page of `errno`):\n> simply set it to 0 before the syscall (or in the function that relies on\n> `errno == 0` means success).\n\nI don't think that is the problem. According to the standard, errno is\nalways set to a non-zero value after a syscall failure.\n\nThe problem is only that the compiler does not incorporate that special\nknowledge of the variable, so it generates a warning even though we can\nreason that the situation it describes is impossible.\n\n-Peff\n"},{"id":"424155","messageId":"YJrIMbr6VkYGQMfs@coredump.intra.peff.net","threadId":"53150","inReplyTo":"YJrEy8IPi+PFWuzT@coredump.intra.peff.net","subject":"Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-11T18:08:49Z","receivedAt":"2021-05-11T18:09:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 11, 2021 at 01:54:19PM -0400, Jeff King wrote:\n\n> > > In any case, this needs to be signed off before it gets carved into\n> > > our history.\n> > \n> > Done, and also changed the variable name to minimize the size of the\n> > diff. A shorter name allowed for less re-flowing of lines.\n> \n> It's quite short. I'm OK with it for a static-local function with few\n> callers like this, though.\n\nActually, I just noticed that you did not introduce \"e\" in the caller.\nSo it is not even a new name, and you are just following convention.\n\nI also wondered briefly why we needed the out-parameter at all, and not\njust letting the caller look at errno. The answer is that we need to\npreserve it across the close() call. The more usual thing in our code\nbase _would_ be to use saved_errno, but not have it as an out-parameter.\n\nI.e.:\n\ndiff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\nindex ae052a07fe..bda283e7f4 100644\n--- a/trace2/tr2_dst.c\n+++ b/trace2/tr2_dst.c\n@@ -204,15 +204,16 @@ static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)\n \n \tfd = socket(AF_UNIX, sock_type, 0);\n \tif (fd == -1)\n-\t\treturn errno;\n+\t\treturn -1;\n \n \tsa.sun_family = AF_UNIX;\n \tstrlcpy(sa.sun_path, path, sizeof(sa.sun_path));\n \n \tif (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {\n-\t\tint e = errno;\n+\t\tint saved_errno = errno;\n \t\tclose(fd);\n-\t\treturn e;\n+\t\terrno = saved_errno;\n+\t\treturn -1;\n \t}\n \n \t*out_fd = fd;\n@@ -227,7 +228,6 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n {\n \tunsigned int uds_try = 0;\n \tint fd;\n-\tint e;\n \tconst char *path = NULL;\n \n \t/*\n@@ -271,23 +271,21 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n \t}\n \n \tif (uds_try & TR2_DST_UDS_TRY_STREAM) {\n-\t\te = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd);\n-\t\tif (!e)\n+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd))\n \t\t\tgoto connected;\n-\t\tif (e != EPROTOTYPE)\n+\t\tif (errno != EPROTOTYPE)\n \t\t\tgoto error;\n \t}\n \tif (uds_try & TR2_DST_UDS_TRY_DGRAM) {\n-\t\te = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd);\n-\t\tif (!e)\n+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd))\n \t\t\tgoto connected;\n \t}\n \n error:\n \tif (tr2_dst_want_warning())\n \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n-\t\t\tstrerror(e));\n+\t\t\tstrerror(errno));\n \n \ttr2_dst_trace_disable(dst);\n \treturn 0;\n\n\nI do prefer that approach, since I think it's more idiomatic for our\ncode base, but for the sake of wrapping up this simple fix which has\nbeen discussed much more than I think it deserves, I am OK with either.\n:)\n\n(I also found it interesting that the \"error\" goto in the caller only\nhas one source. I think the code would be easier to reason about if it\nwere inlined, but I'm happy to stop here for now).\n\n-Peff\n"},{"id":"424202","messageId":"xmqqlf8lkmwo.fsf@gitster.g","threadId":"53150","inReplyTo":"YJrGOqjS10Z/WkE8@coredump.intra.peff.net","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-11T20:58:47Z","receivedAt":"2021-05-11T20:58:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, May 11, 2021 at 04:34:49PM +0200, Johannes Schindelin wrote:\n>\n>> On Fri, 7 May 2021, Junio C Hamano wrote:\n>> \n>> > \t\t/* GCC thinks socket()/connect() might fail to set errno */\n>> > \t\treturn errno ? errno : EIO;\n>> >\n>> > If a compiler thinks errno may *not* be set, can 'errno' be reliably\n>> > used to decide if it can be returned as-is or a fallback value EIO\n>> > should be used, without triggering the same (incorrect) working in\n>> > the first place?\n>> \n>> Oh, I guess I mistook the problem for something else, then.\n>> \n>> If the problem is that `errno` is not set in case of failure, the\n>> resolution is easy (and even recommended in the manual page of `errno`):\n>> simply set it to 0 before the syscall (or in the function that relies on\n>> `errno == 0` means success).\n>\n> I don't think that is the problem. According to the standard, errno is\n> always set to a non-zero value after a syscall failure.\n>\n> The problem is only that the compiler does not incorporate that special\n> knowledge of the variable, so it generates a warning even though we can\n> reason that the situation it describes is impossible.\n\nYes, that is what I tried to say (i.e. if the compiler does not know\nerrno has a defined value at certain places in our code and\ncomplain, then \"return errno ? errno : EIO\" would get the same\nwarning because bases its outcome on the value of errno the compiler\nthinks is possibly undefined at that point), but apparently I failed\nto convey that clearly enough.\n\nThanks.\n\n\n"},{"id":"424204","messageId":"YJryDGSliWvvABDB@coredump.intra.peff.net","threadId":"53150","inReplyTo":"xmqqlf8lkmwo.fsf@gitster.g","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-11T21:07:24Z","receivedAt":"2021-05-11T21:07:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 12, 2021 at 05:58:47AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Tue, May 11, 2021 at 04:34:49PM +0200, Johannes Schindelin wrote:\n> >\n> >> On Fri, 7 May 2021, Junio C Hamano wrote:\n> >> \n> >> > \t\t/* GCC thinks socket()/connect() might fail to set errno */\n> >> > \t\treturn errno ? errno : EIO;\n> >> >\n> >> > If a compiler thinks errno may *not* be set, can 'errno' be reliably\n> >> > used to decide if it can be returned as-is or a fallback value EIO\n> >> > should be used, without triggering the same (incorrect) working in\n> >> > the first place?\n> >> \n> >> Oh, I guess I mistook the problem for something else, then.\n> >> \n> >> If the problem is that `errno` is not set in case of failure, the\n> >> resolution is easy (and even recommended in the manual page of `errno`):\n> >> simply set it to 0 before the syscall (or in the function that relies on\n> >> `errno == 0` means success).\n> >\n> > I don't think that is the problem. According to the standard, errno is\n> > always set to a non-zero value after a syscall failure.\n> >\n> > The problem is only that the compiler does not incorporate that special\n> > knowledge of the variable, so it generates a warning even though we can\n> > reason that the situation it describes is impossible.\n> \n> Yes, that is what I tried to say (i.e. if the compiler does not know\n> errno has a defined value at certain places in our code and\n> complain, then \"return errno ? errno : EIO\" would get the same\n> warning because bases its outcome on the value of errno the compiler\n> thinks is possibly undefined at that point), but apparently I failed\n> to convey that clearly enough.\n\nOne thing in what you said puzzles me, though. The problem is not that\nthe compiler thinks errno is not set at all (i.e., undefined). It simply\ndoes not know whether it is non-zero or not after the call.\n\nSo switching to \"return errno ? errno : EIO\" does indeed work here,\nbecause the compiler now knows that the result of that return will\nalways be non-zero.\n\nIt must assume that \"errno\" is always defined to _something_, because\nit's a global (so there's no undefined behavior here). It was only it's\nzero/non-zero implication that let to code-paths were \"fd\" could be\nused uninitialized.\n\n-Peff\n"},{"id":"424205","messageId":"xmqqcztxkmeu.fsf@gitster.g","threadId":"53150","inReplyTo":"YJrIMbr6VkYGQMfs@coredump.intra.peff.net","subject":"Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-11T21:09:29Z","receivedAt":"2021-05-11T21:09:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I also wondered briefly why we needed the out-parameter at all, and not\n> just letting the caller look at errno. The answer is that we need to\n> preserve it across the close() call. The more usual thing in our code\n> base _would_ be to use saved_errno, but not have it as an out-parameter.\n>\n> I.e.:\n>\n> diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\n> index ae052a07fe..bda283e7f4 100644\n> --- a/trace2/tr2_dst.c\n> +++ b/trace2/tr2_dst.c\n> @@ -204,15 +204,16 @@ static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)\n> \n> \tfd = socket(AF_UNIX, sock_type, 0);\n> \tif (fd == -1)\n> -\t\treturn errno;\n> +\t\treturn -1;\n> \n> \tsa.sun_family = AF_UNIX;\n> \tstrlcpy(sa.sun_path, path, sizeof(sa.sun_path));\n> \n> \tif (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {\n> -\t\tint e = errno;\n> +\t\tint saved_errno = errno;\n> \t\tclose(fd);\n> -\t\treturn e;\n> +\t\terrno = saved_errno;\n> +\t\treturn -1;\n> \t}\n> \n> \t*out_fd = fd;\n> @@ -227,7 +228,6 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n> {\n> \tunsigned int uds_try = 0;\n> \tint fd;\n> -\tint e;\n> \tconst char *path = NULL;\n> \n> \t/*\n> @@ -271,23 +271,21 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n> \t}\n> \n> \tif (uds_try & TR2_DST_UDS_TRY_STREAM) {\n> -\t\te = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd);\n> -\t\tif (!e)\n> +\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd))\n> \t\t\tgoto connected;\n> -\t\tif (e != EPROTOTYPE)\n> +\t\tif (errno != EPROTOTYPE)\n> \t\t\tgoto error;\n> \t}\n> \tif (uds_try & TR2_DST_UDS_TRY_DGRAM) {\n> -\t\te = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd);\n> -\t\tif (!e)\n> +\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd))\n> \t\t\tgoto connected;\n> \t}\n> \n> error:\n> \tif (tr2_dst_want_warning())\n> \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n> \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n> -\t\t\tstrerror(e));\n> +\t\t\tstrerror(errno));\n> \n> \ttr2_dst_trace_disable(dst);\n> \treturn 0;\n>\n>\n> I do prefer that approach, since I think it's more idiomatic for our\n> code base, but for the sake of wrapping up this simple fix which has\n> been discussed much more than I think it deserves, I am OK with either.\n> :)\n\nYeah, the above looks nicer to me too.\n\n>\n> (I also found it interesting that the \"error\" goto in the caller only\n> has one source. I think the code would be easier to reason about if it\n> were inlined, but I'm happy to stop here for now).\n>\n> -Peff\n"},{"id":"424208","messageId":"xmqq4kf9klbk.fsf@gitster.g","threadId":"53150","inReplyTo":"YJryDGSliWvvABDB@coredump.intra.peff.net","subject":"Re: [PATCH] trace2: refactor to avoid gcc warning under -O3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-11T21:33:03Z","receivedAt":"2021-05-11T21:33:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So switching to \"return errno ? errno : EIO\" does indeed work here,\n> because the compiler now knows that the result of that return will\n> always be non-zero.\n\nOK.\n"},{"id":"425007","messageId":"xmqqim3ew905.fsf@gitster.g","threadId":"53150","inReplyTo":"YJrIMbr6VkYGQMfs@coredump.intra.peff.net","subject":"Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-20T00:20:58Z","receivedAt":"2021-05-20T00:21:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I also wondered briefly why we needed the out-parameter at all, and not\n> just letting the caller look at errno. The answer is that we need to\n> preserve it across the close() call. The more usual thing in our code\n> base _would_ be to use saved_errno, but not have it as an out-parameter.\n>\n> I.e.:\n>\n> diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\n> index ae052a07fe..bda283e7f4 100644\n> --- a/trace2/tr2_dst.c\n> +++ b/trace2/tr2_dst.c\n> @@ -204,15 +204,16 @@ static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)\n> \n> \tfd = socket(AF_UNIX, sock_type, 0);\n> \tif (fd == -1)\n> -\t\treturn errno;\n> +\t\treturn -1;\n> \n> \tsa.sun_family = AF_UNIX;\n> \tstrlcpy(sa.sun_path, path, sizeof(sa.sun_path));\n> \n> \tif (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {\n> -\t\tint e = errno;\n> +\t\tint saved_errno = errno;\n> \t\tclose(fd);\n> -\t\treturn e;\n> +\t\terrno = saved_errno;\n> +\t\treturn -1;\n> \t}\n> \n> ...\n>\n> I do prefer that approach, since I think it's more idiomatic for our\n> code base, but for the sake of wrapping up this simple fix which has\n> been discussed much more than I think it deserves, I am OK with either.\n> :)\n\nI think this alternative is more readable as well.\n\nI'll mark the topic to be \"Expecting a reroll\" in the what's cooking\nreport.\n\nThanks.\n"},{"id":"425078","messageId":"patch-1.1-2e41e3e4cb-20210520T110357Z-avarab@gmail.com","threadId":"53150","inReplyTo":"xmqqim3ew905.fsf@gitster.g","subject":"[PATCH v3] trace2: refactor to avoid gcc warning under -O3","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-20T11:05:45Z","receivedAt":"2021-05-20T12:17:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that\nappears under -O3 (but not -O2). This makes the build pass under\nDEVELOPER=1 without needing a DEVOPTS=no-error.\n\nThis can be reproduced with GCC Debian 8.3.0-6, but not e.g. with\nclang 7.0.1-8+deb10u2. We've had this warning since\nee4512ed481 (trace2: create new combined trace facility, 2019-02-22).\n\nAs noted in [2] this warning happens because the compiler doesn't\nassume that errno must be non-zero after a failed syscall.\n\nLet's work around by using the well-established \"saved_errno\" pattern,\nalong with returning -1 ourselves instead of \"errno\". The caller can\nthus rely on our \"errno\" on failure.\n\nSee https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61846 for a related\nbug report against GCC.\n\n1.\n\n trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’:\n trace2/tr2_dst.c:296:10: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized]\n dst->fd = fd;\n ~~~~~~~~^~~~\n trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here\n int fd;\n ^~\n2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Jeff King \n---\n\nOn Thu, May 20 2021, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n>> I also wondered briefly why we needed the out-parameter at all, and not\n>> just letting the caller look at errno. The answer is that we need to\n>> preserve it across the close() call. The more usual thing in our code\n>> base would be to use saved_errno, but not have it as an out-parameter.\n>> [...]\n> I think this alternative is more readable as well.\n>\n> I'll mark the topic to be \"Expecting a reroll\" in the what's cooking\n> report.\n>\n> Thanks.\n\nHere's that re-roll, thanks both.\n\nThe patch is entirely Jeff's at this point (from\n), with my amended commit\nmessage. So I added his SOB per his recent-ish ML \"every patch of mine\ncan be assumed to have my SOB\" message.\n\nRange-diff against v2:\n1: 782555daad ! 1: 2e41e3e4cb trace2: refactor to avoid gcc warning under -O3\n @@ Commit message\n ee4512ed481 (trace2: create new combined trace facility, 2019-02-22).\n \n As noted in [2] this warning happens because the compiler doesn't\n - assume that errno must be non-zero after a failed syscall. Let's work\n - around it as suggested in that analysis. We now return -1 ourselves on\n - error, and save away the value of errno in a variable the caller\n - passes in.\n + assume that errno must be non-zero after a failed syscall.\n +\n + Let's work around by using the well-established \"saved_errno\" pattern,\n + along with returning -1 ourselves instead of \"errno\". The caller can\n + thus rely on our \"errno\" on failure.\n \n See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61846 for a related\n bug report against GCC.\n @@ Commit message\n 2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n + Signed-off-by: Jeff King \n \n ## trace2/tr2_dst.c ##\n -@@ trace2/tr2_dst.c: static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value)\n - #define PREFIX_AF_UNIX_STREAM \"af_unix:stream:\"\n - #define PREFIX_AF_UNIX_DGRAM \"af_unix:dgram:\"\n - \n --static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)\n -+static int tr2_dst_try_uds_connect(const char *path, int sock_type,\n -+\t\t\t\t int *out_fd, int *e)\n - {\n - \tint fd;\n - \tstruct sockaddr_un sa;\n +@@ trace2/tr2_dst.c: static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)\n \n \tfd = socket(AF_UNIX, sock_type, 0);\n --\tif (fd == -1)\n + \tif (fd == -1)\n -\t\treturn errno;\n -+\tif (fd == -1) {\n -+\t\t*e = errno;\n +\t\treturn -1;\n -+\t}\n \n \tsa.sun_family = AF_UNIX;\n \tstrlcpy(sa.sun_path, path, sizeof(sa.sun_path));\n \n \tif (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {\n -\t\tint e = errno;\n -+\t\t*e = errno;\n ++\t\tint saved_errno = errno;\n \t\tclose(fd);\n -\t\treturn e;\n ++\t\terrno = saved_errno;\n +\t\treturn -1;\n \t}\n \n \t*out_fd = fd;\n +@@ trace2/tr2_dst.c: static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n + {\n + \tunsigned int uds_try = 0;\n + \tint fd;\n +-\tint e;\n + \tconst char *path = NULL;\n + \n + \t/*\n @@ trace2/tr2_dst.c: static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n \t}\n \n \tif (uds_try & TR2_DST_UDS_TRY_STREAM) {\n -\t\te = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd);\n -\t\tif (!e)\n -+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd, &e))\n ++\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd))\n \t\t\tgoto connected;\n - \t\tif (e != EPROTOTYPE)\n +-\t\tif (e != EPROTOTYPE)\n ++\t\tif (errno != EPROTOTYPE)\n \t\t\tgoto error;\n \t}\n \tif (uds_try & TR2_DST_UDS_TRY_DGRAM) {\n -\t\te = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd);\n -\t\tif (!e)\n -+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd, &e))\n ++\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd))\n \t\t\tgoto connected;\n \t}\n \n +@@ trace2/tr2_dst.c: static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n + \tif (tr2_dst_want_warning())\n + \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n + \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n +-\t\t\tstrerror(e));\n ++\t\t\tstrerror(errno));\n + \n + \ttr2_dst_trace_disable(dst);\n + \treturn 0;\n\n trace2/tr2_dst.c | 18 ++++++++----------\n 1 file changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\nindex ae052a07fe..bda283e7f4 100644\n--- a/trace2/tr2_dst.c\n+++ b/trace2/tr2_dst.c\n@@ -204,15 +204,16 @@ static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)\n \n \tfd = socket(AF_UNIX, sock_type, 0);\n \tif (fd == -1)\n-\t\treturn errno;\n+\t\treturn -1;\n \n \tsa.sun_family = AF_UNIX;\n \tstrlcpy(sa.sun_path, path, sizeof(sa.sun_path));\n \n \tif (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {\n-\t\tint e = errno;\n+\t\tint saved_errno = errno;\n \t\tclose(fd);\n-\t\treturn e;\n+\t\terrno = saved_errno;\n+\t\treturn -1;\n \t}\n \n \t*out_fd = fd;\n@@ -227,7 +228,6 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n {\n \tunsigned int uds_try = 0;\n \tint fd;\n-\tint e;\n \tconst char *path = NULL;\n \n \t/*\n@@ -271,15 +271,13 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n \t}\n \n \tif (uds_try & TR2_DST_UDS_TRY_STREAM) {\n-\t\te = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd);\n-\t\tif (!e)\n+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd))\n \t\t\tgoto connected;\n-\t\tif (e != EPROTOTYPE)\n+\t\tif (errno != EPROTOTYPE)\n \t\t\tgoto error;\n \t}\n \tif (uds_try & TR2_DST_UDS_TRY_DGRAM) {\n-\t\te = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd);\n-\t\tif (!e)\n+\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd))\n \t\t\tgoto connected;\n \t}\n \n@@ -287,7 +285,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n \tif (tr2_dst_want_warning())\n \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n-\t\t\tstrerror(e));\n+\t\t\tstrerror(errno));\n \n \ttr2_dst_trace_disable(dst);\n \treturn 0;\n-- \n2.32.0.rc0.406.g73369325f8d\n\n"},{"id":"425100","messageId":"YKZgZZrZdKyUk9d9@coredump.intra.peff.net","threadId":"53150","inReplyTo":"patch-1.1-2e41e3e4cb-20210520T110357Z-avarab@gmail.com","subject":"Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-20T13:13:09Z","receivedAt":"2021-05-20T13:13:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 20, 2021 at 01:05:45PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> >> I also wondered briefly why we needed the out-parameter at all, and not\n> >> just letting the caller look at errno. The answer is that we need to\n> >> preserve it across the close() call. The more usual thing in our code\n> >> base would be to use saved_errno, but not have it as an out-parameter.\n> >> [...]\n> > I think this alternative is more readable as well.\n> >\n> > I'll mark the topic to be \"Expecting a reroll\" in the what's cooking\n> > report.\n> >\n> > Thanks.\n> \n> Here's that re-roll, thanks both.\n\nThanks, this looks OK to me. There is one minor nit (introduced by me!)\nbelow, but I'm not sure if we should care or not.\n\n> The patch is entirely Jeff's at this point (from\n> ), with my amended commit\n> message. So I added his SOB per his recent-ish ML \"every patch of mine\n> can be assumed to have my SOB\" message.\n\nSo I have definitely said that, and I stand by it. But as a matter of\nproject policy, I think we probably shouldn't consider that \"enough\".\nThe point of the DSO is to make an affirmative statement about a\nparticular patch. So probably my blanket statement should be taken more\nas \"I will almost definitely add my signoff if you ask me\". :)\n\nAnd of course in the case of this particular patch, it is very much:\n\n Signed-off-by: Jeff King \n\n> @@ -271,15 +271,13 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n> \t}\n> \n> \tif (uds_try & TR2_DST_UDS_TRY_STREAM) {\n> -\t\te = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd);\n> -\t\tif (!e)\n> +\t\tif (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd))\n> \t\t\tgoto connected;\n> -\t\tif (e != EPROTOTYPE)\n> +\t\tif (errno != EPROTOTYPE)\n> \t\t\tgoto error;\n> \t}\n> \tif (uds_try & TR2_DST_UDS_TRY_DGRAM) {\n> -\t\te = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd);\n> -\t\tif (!e)\n> +\t\tif (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd))\n> \t\t\tgoto connected;\n> \t}\n> \n> @@ -287,7 +285,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n> \tif (tr2_dst_want_warning())\n> \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n> \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n> -\t\t\tstrerror(e));\n> +\t\t\tstrerror(errno));\n\nWe expect the value of errno to persist across tr2_dst_want_warning()\nand tr2_sysenv_display_name() here. The former may call getenv() and\natoi(). I think that's probably fine, but if we wanted to be really\nparanoid, we'd have to preserve errno manually here, too.\n\n-Peff\n"},{"id":"425160","messageId":"xmqqk0ntt5x0.fsf@gitster.g","threadId":"53150","inReplyTo":"YKZgZZrZdKyUk9d9@coredump.intra.peff.net","subject":"Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-20T22:08:11Z","receivedAt":"2021-05-20T22:08:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> @@ -287,7 +285,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n>> \tif (tr2_dst_want_warning())\n>> \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n>> \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n>> -\t\t\tstrerror(e));\n>> +\t\t\tstrerror(errno));\n>\n> We expect the value of errno to persist across tr2_dst_want_warning()\n> and tr2_sysenv_display_name() here. The former may call getenv() and\n> atoi(). I think that's probably fine, but if we wanted to be really\n> paranoid, we'd have to preserve errno manually here, too.\n\nBeing \"really paranoid\" consistently within the file would mean a\nchange like the attached, I would think, on top of what was posted.\n\nOr tr2_dst_want_warning() and tr2_sysenv_display_name() can be\ntaught to preserve errno like tr2_dst_dry_uds_connect() was taught\nto do so by the patch under discussion, which may reduce the amount\nof apparent change, but constantly moving the contents of errno\naround just in case we later might want to use its value feels\ndirty.\n\nI dunno.\n\n trace2/tr2_dst.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git c/trace2/tr2_dst.c w/trace2/tr2_dst.c\nindex 0031476350..f740a0a076 100644\n--- c/trace2/tr2_dst.c\n+++ w/trace2/tr2_dst.c\n@@ -62,11 +62,13 @@ static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)\n \t}\n \n \tif (fd == -1) {\n+\t\tint saved_errno = errno;\n+\n \t\tif (tr2_dst_want_warning())\n \t\t\twarning(\"trace2: could not open '%.*s' for '%s' tracing: %s\",\n \t\t\t\t(int) base_path_len, path.buf,\n \t\t\t\ttr2_sysenv_display_name(dst->sysenv_var),\n-\t\t\t\tstrerror(errno));\n+\t\t\t\tstrerror(saved_errno));\n \n \t\ttr2_dst_trace_disable(dst);\n \t\tstrbuf_release(&path);\n@@ -86,6 +88,8 @@ static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value)\n {\n \tint fd = open(tgt_value, O_WRONLY | O_APPEND | O_CREAT, 0666);\n \tif (fd == -1) {\n+\t\tint saved_errno = errno;\n+\n \t\tif (tr2_dst_want_warning())\n \t\t\twarning(\"trace2: could not open '%s' for '%s' tracing: %s\",\n \t\t\t\ttgt_value,\n@@ -140,6 +144,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n \tunsigned int uds_try = 0;\n \tint fd;\n \tconst char *path = NULL;\n+\tint saved_errno;\n \n \t/*\n \t * Allow \"af_unix:[:]\"\n@@ -193,10 +198,11 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n \t}\n \n error:\n+\tsaved_errno = errno;\n \tif (tr2_dst_want_warning())\n \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n-\t\t\tstrerror(errno));\n+\t\t\tstrerror(saved_errno));\n \n \ttr2_dst_trace_disable(dst);\n \treturn 0;\n@@ -276,6 +282,7 @@ int tr2_dst_trace_want(struct tr2_dst *dst)\n void tr2_dst_write_line(struct tr2_dst *dst, struct strbuf *buf_line)\n {\n \tint fd = tr2_dst_get_trace_fd(dst);\n+\tint saved_errno;\n \n \tstrbuf_complete_line(buf_line); /* ensure final NL on buffer */\n \n@@ -297,9 +304,10 @@ void tr2_dst_write_line(struct tr2_dst *dst, struct strbuf *buf_line)\n \tif (write(fd, buf_line->buf, buf_line->len) >= 0)\n \t\treturn;\n \n+\tsaved_errno = errno;\n \tif (tr2_dst_want_warning())\n \t\twarning(\"unable to write trace to '%s': %s\",\n \t\t\ttr2_sysenv_display_name(dst->sysenv_var),\n-\t\t\tstrerror(errno));\n+\t\t\tstrerror(saved_errno));\n \ttr2_dst_trace_disable(dst);\n }\n"},{"id":"425190","messageId":"YKd+mtQuW9Yz0qwh@coredump.intra.peff.net","threadId":"53150","inReplyTo":"xmqqk0ntt5x0.fsf@gitster.g","subject":"Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-21T09:34:18Z","receivedAt":"2021-05-21T09:34:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 21, 2021 at 07:08:11AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> @@ -287,7 +285,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,\n> >> \tif (tr2_dst_want_warning())\n> >> \t\twarning(\"trace2: could not connect to socket '%s' for '%s' tracing: %s\",\n> >> \t\t\tpath, tr2_sysenv_display_name(dst->sysenv_var),\n> >> -\t\t\tstrerror(e));\n> >> +\t\t\tstrerror(errno));\n> >\n> > We expect the value of errno to persist across tr2_dst_want_warning()\n> > and tr2_sysenv_display_name() here. The former may call getenv() and\n> > atoi(). I think that's probably fine, but if we wanted to be really\n> > paranoid, we'd have to preserve errno manually here, too.\n> \n> Being \"really paranoid\" consistently within the file would mean a\n> change like the attached, I would think, on top of what was posted.\n> \n> Or tr2_dst_want_warning() and tr2_sysenv_display_name() can be\n> taught to preserve errno like tr2_dst_dry_uds_connect() was taught\n> to do so by the patch under discussion, which may reduce the amount\n> of apparent change, but constantly moving the contents of errno\n> around just in case we later might want to use its value feels\n> dirty.\n> \n> I dunno.\n> \n> trace2/tr2_dst.c | 14 +++++++++++---\n> 1 file changed, 11 insertions(+), 3 deletions(-)\n\nAh, yeah. I didn't look to see if there were existing cases of the same\nthing.\n\nI could go either way on this kind of saved_errno thing in general (the\ntr2 functions called in between are really quite unlikely to set errno\n(I am not even sure if getenv() and atoi() can, so this really might\njust be future-proofing in case those tr2 functions get more\ncomplicated).\n\nBut seeing that there are other cases of the same, I definitely think it\nis not something that should be in Ævar's patch. It is a cleanup we\ncould do on top if we cared to.\n\n-Peff\n"}]}