{"thread":{"id":"46018","subject":"[PATCH] usage: add NORETURN to BUG() function definitions","startedAt":"2017-05-21T22:26:00Z","lastAt":"2017-05-23T20:48:17Z","messageCount":9,"participants":["Ramsay Jones","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"320381","messageId":"4a5619af-d695-ab6c-e603-368e38827455@ramsayjones.plus.com","threadId":"46018","inReplyTo":null,"subject":"[PATCH] usage: add NORETURN to BUG() function definitions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-21T22:25:39Z","receivedAt":"2017-05-21T22:26:00Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nCommit d8193743e0 (\"usage.c: add BUG() function\", 12-05-2017) added the\nBUG() functions and macros as a replacement for calls to die(\"BUG: ..\").\nThe use of NORETURN on the declarations (in git-compat-util.h) and the\nlack of NORETURN on the function definitions, however, leads sparse to\ncomplain thus:\n\n SP usage.c\n usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type\n (originally declared at git-compat-util.h:1074) - different modifiers\n\nIn order to suppress the sparse error, add the NORETURN to the function\ndefinitions.\n\nSigned-off-by: Ramsay Jones \n---\n\nHi Junio,\n\nThis is built on 'next', which has now merged the 'jk/bug-to-abort'\nbranch.\n\nThanks!\n\nATB,\nRamsay Jones\n\n usage.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/usage.c b/usage.c\nindex 7e6cb2028..1f63e033e 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -217,7 +217,7 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis\n }\n \n #ifdef HAVE_VARIADIC_MACROS\n-void BUG_fl(const char *file, int line, const char *fmt, ...)\n+NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)\n {\n \tva_list ap;\n \tva_start(ap, fmt);\n@@ -225,7 +225,7 @@ void BUG_fl(const char *file, int line, const char *fmt, ...)\n \tva_end(ap);\n }\n #else\n-void BUG(const char *fmt, ...)\n+NORETURN void BUG(const char *fmt, ...)\n {\n \tva_list ap;\n \tva_start(ap, fmt);\n-- \n2.13.0\n"},{"id":"320386","messageId":"xmqqpof1psy7.fsf@gitster.mtv.corp.google.com","threadId":"46018","inReplyTo":"4a5619af-d695-ab6c-e603-368e38827455@ramsayjones.plus.com","subject":"Re: [PATCH] usage: add NORETURN to BUG() function definitions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-22T01:43:12Z","receivedAt":"2017-05-22T01:43:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> Commit d8193743e0 (\"usage.c: add BUG() function\", 12-05-2017) added the\n> BUG() functions and macros as a replacement for calls to die(\"BUG: ..\").\n> The use of NORETURN on the declarations (in git-compat-util.h) and the\n> lack of NORETURN on the function definitions, however, leads sparse to\n> complain thus:\n>\n> SP usage.c\n> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type\n> (originally declared at git-compat-util.h:1074) - different modifiers\n>\n> In order to suppress the sparse error, add the NORETURN to the function\n> definitions.\n>\n> Signed-off-by: Ramsay Jones \n> ---\n>\n> Hi Junio,\n>\n> This is built on 'next', which has now merged the 'jk/bug-to-abort'\n> branch.\n\nThanks. \n\nI cannot seem to cause sparse (v0.5.0-207-g14964df) to complain the\nsame way, though.\n\n>\n> Thanks!\n>\n> ATB,\n> Ramsay Jones\n>\n> usage.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/usage.c b/usage.c\n> index 7e6cb2028..1f63e033e 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -217,7 +217,7 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis\n> }\n> \n> #ifdef HAVE_VARIADIC_MACROS\n> -void BUG_fl(const char *file, int line, const char *fmt, ...)\n> +NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)\n> {\n> \tva_list ap;\n> \tva_start(ap, fmt);\n> @@ -225,7 +225,7 @@ void BUG_fl(const char *file, int line, const char *fmt, ...)\n> \tva_end(ap);\n> }\n> #else\n> -void BUG(const char *fmt, ...)\n> +NORETURN void BUG(const char *fmt, ...)\n> {\n> \tva_list ap;\n> \tva_start(ap, fmt);\n"},{"id":"320387","messageId":"11cae8d7-46a6-9ab5-5bee-a7e6897c0a88@ramsayjones.plus.com","threadId":"46018","inReplyTo":"xmqqpof1psy7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] usage: add NORETURN to BUG() function definitions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-22T02:13:34Z","receivedAt":"2017-05-22T02:13:42Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 22/05/17 02:43, Junio C Hamano wrote:\n> Ramsay Jones writes:\n> \n>> Commit d8193743e0 (\"usage.c: add BUG() function\", 12-05-2017) added the\n>> BUG() functions and macros as a replacement for calls to die(\"BUG: ..\").\n>> The use of NORETURN on the declarations (in git-compat-util.h) and the\n>> lack of NORETURN on the function definitions, however, leads sparse to\n>> complain thus:\n>>\n>> SP usage.c\n>> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type\n>> (originally declared at git-compat-util.h:1074) - different modifiers\n>>\n>> In order to suppress the sparse error, add the NORETURN to the function\n>> definitions.\n>>\n>> Signed-off-by: Ramsay Jones \n>> ---\n>>\n>> Hi Junio,\n>>\n>> This is built on 'next', which has now merged the 'jk/bug-to-abort'\n>> branch.\n> \n> Thanks. \n> \n> I cannot seem to cause sparse (v0.5.0-207-g14964df) to complain the\n> same way, though.\n\nHmm, interesting... I have an older version installed:\n\n $ sparse --version\n v0.5.0-60-g6c283a0\n $ sparse usage.c\n usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type\n (originally declared at git-compat-util.h:1074) - different modifiers\n $ \n\n... but a more up-to-date version agrees:\n\n $ ~/sparse/sparse --version\n v0.5.0-237-g51de1cc\n $ ~/sparse/sparse usage.c\n usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type\n (originally declared at git-compat-util.h:1074) - different modifiers\n $ \n\nSo, I don't know. Wait let me try your specific version:\n\n$ ~/sparse/sparse --version\nv0.5.0-207-g14964df\n$ ~/sparse/sparse usage.c\nusage.c:220:6: error: symbol 'BUG_fl' redeclared with different type (originally declared at git-compat-util.h:1074) - different modifiers\n$ \n\nEr, dunno. (This is on Linux Mint 18.1).\n\nATB,\nRamsay Jones\n\n"},{"id":"320389","messageId":"xmqqh90dpqja.fsf@gitster.mtv.corp.google.com","threadId":"46018","inReplyTo":"11cae8d7-46a6-9ab5-5bee-a7e6897c0a88@ramsayjones.plus.com","subject":"Re: [PATCH] usage: add NORETURN to BUG() function definitions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-22T02:35:21Z","receivedAt":"2017-05-22T02:35:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> So, I don't know. Wait let me try your specific version:\n>\n> $ ~/sparse/sparse --version\n> v0.5.0-207-g14964df\n> $ ~/sparse/sparse usage.c\n> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type (originally declared at git-compat-util.h:1074) - different modifiers\n> $ \n>\n> Er, dunno. (This is on Linux Mint 18.1).\n\nOh, I don't question your expertise or competence. There must be\nsomething I am doing wrong, and the version of sparse I happened to\nhave run was the easiest thing to point a finger at, but that does\nnot seem to be it.\n\nThanks for helping. I'll find time to dig deeper to find what's\nbreaking it for me.\n\n\n\n"},{"id":"320390","messageId":"xmqqd1b1pq1j.fsf@gitster.mtv.corp.google.com","threadId":"46018","inReplyTo":"xmqqh90dpqja.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] usage: add NORETURN to BUG() function definitions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-22T02:46:00Z","receivedAt":"2017-05-22T02:46:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Ramsay Jones writes:\n>\n>> So, I don't know. Wait let me try your specific version:\n>>\n>> $ ~/sparse/sparse --version\n>> v0.5.0-207-g14964df\n>> $ ~/sparse/sparse usage.c\n>> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type (originally declared at git-compat-util.h:1074) - different modifiers\n>> $ \n>>\n>> Er, dunno. (This is on Linux Mint 18.1).\n>\n> Oh, I don't question your expertise or competence. There must be\n> something I am doing wrong, and the version of sparse I happened to\n> have run was the easiest thing to point a finger at, but that does\n> not seem to be it.\n>\n> Thanks for helping. I'll find time to dig deeper to find what's\n> breaking it for me.\n\nHmph. I do not know what went wrong. The one I had in /usr/bin\nthat came from the distro was too old that it didn't give any useful\nresult and failed, and that was why I got v0.5.0-207-g14964df\ninstalled in ~/gitstuff/bin/ which is early on my $PATH; I do not\nthink I did any other updates but now I am seeing happy results.\n\n $ git checkout jk/bug-to-abort^1\n $ make SP_OBJ=usage.sp sparse\n GIT_VERSION = 2.13.0.3.g25cd291963\n SP usage.c\n usage.c:220:6: error: symbol 'BUG_fl' redeclared with diff...\n\nAnd then with your fix, of course,\n\n $ git checkout jk/bug-to-abort\n $ make SP_OBJ=usage.sp sparse\n GIT_VERSION = 2.13.0.4.g3d7dd2d3b6\n SP usage.c\n\nI am still puzzled but anyway now the problem is clearly on my end\nand no longer reproduces, there is no reason to waste your time.\n\nSorry for the noise, and thanks for a fix again.\n"},{"id":"320403","messageId":"20170522111920.244yup7wol55mtdm@sigill.intra.peff.net","threadId":"46018","inReplyTo":"4a5619af-d695-ab6c-e603-368e38827455@ramsayjones.plus.com","subject":"Re: [PATCH] usage: add NORETURN to BUG() function definitions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-22T11:19:21Z","receivedAt":"2017-05-22T11:19:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 21, 2017 at 11:25:39PM +0100, Ramsay Jones wrote:\n\n> Commit d8193743e0 (\"usage.c: add BUG() function\", 12-05-2017) added the\n> BUG() functions and macros as a replacement for calls to die(\"BUG: ..\").\n> The use of NORETURN on the declarations (in git-compat-util.h) and the\n> lack of NORETURN on the function definitions, however, leads sparse to\n> complain thus:\n> \n> SP usage.c\n> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type\n> (originally declared at git-compat-util.h:1074) - different modifiers\n> \n> In order to suppress the sparse error, add the NORETURN to the function\n> definitions.\n> \n> Signed-off-by: Ramsay Jones \n\nThanks, I never ended up re-rolling the original, but from our previous\ndiscussion this is obviously fine by me.\n\n-Peff\n"},{"id":"320419","messageId":"15d73455-6d0e-e67a-7cf3-eb0ae9aa3b0d@ramsayjones.plus.com","threadId":"46018","inReplyTo":"xmqqd1b1pq1j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] usage: add NORETURN to BUG() function definitions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-22T14:02:16Z","receivedAt":"2017-05-22T14:08:07Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 22/05/17 03:46, Junio C Hamano wrote:\n> Hmph. I do not know what went wrong. The one I had in /usr/bin\n> that came from the distro was too old that it didn't give any useful\n> result and failed, and that was why I got v0.5.0-207-g14964df\n> installed in ~/gitstuff/bin/ which is early on my $PATH; I do not\n> think I did any other updates but now I am seeing happy results.\n> \n> $ git checkout jk/bug-to-abort^1\n> $ make SP_OBJ=usage.sp sparse\n> GIT_VERSION = 2.13.0.3.g25cd291963\n> SP usage.c\n> usage.c:220:6: error: symbol 'BUG_fl' redeclared with diff...\n\nTo save yourself a little typing, if you want to run sparse over a\nsingle file:\n\n $ make usage.sp # ie simply replace '.c' with '.sp'\n\n... is the recommended way to do it. This makes sure that the same\nflags are passed to cgcc as are passed to gcc as part of the build.\n\nNote that I would not normally run sparse directly on a source file\n(except when messing around with different versions!), the idea is\nto use cgcc as a frontend (as the Makefile does).\n\nHaving said that, I rarely run sparse over just one file (except\nwhen fixing a sparse error/warning). On each branch (master->next->pu)\nI do\n\n $ make sparse >sp-out 2>&1 # nsp-out on 'next', psp-out on 'pu'\n\n... so that I can diff the files from branch to branch. (I check the\nmaster branch file by hand. There is a single warning on Linux that\nis actually a sparse problem).\n\nJust FYI, for today's fetch:\n\n $ diff sp-out nsp-out\n $ diff nsp-out psp-out\n 12a13\n > SP blame.c\n 42a44,46\n > diff.c:813:6: warning: symbol 'emit_line' was not declared. Should it be static?\n > diff.c:828:6: warning: symbol 'emit_line_fmt' was not declared. Should it be static?\n > diff.c:1865:6: warning: symbol 'print_stat_summary_0' was not declared. Should it be static?\n 54a59\n > SP fsmonitor.c\n 137a143\n > SP sub-process.c\n 170a177\n > SP compat/fopen.c\n 276a284\n > builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer\n 296a305\n > SP t/helper/test-dir-iterator.c\n $ \n \n\nATB,\nRamsay Jones\n\n\n"},{"id":"320515","messageId":"xmqqpof0memv.fsf@gitster.mtv.corp.google.com","threadId":"46018","inReplyTo":"15d73455-6d0e-e67a-7cf3-eb0ae9aa3b0d@ramsayjones.plus.com","subject":"Re: [PATCH] usage: add NORETURN to BUG() function definitions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-23T03:32:56Z","receivedAt":"2017-05-23T03:33:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> Having said that, I rarely run sparse over just one file (except\n> when fixing a sparse error/warning). On each branch (master->next->pu)\n> I do\n>\n> $ make sparse >sp-out 2>&1 # nsp-out on 'next', psp-out on 'pu'\n>\n> ... so that I can diff the files from branch to branch. (I check the\n> master branch file by hand. There is a single warning on Linux that\n> is actually a sparse problem).\n>\n> Just FYI, for today's fetch:\n>\n> $ diff sp-out nsp-out\n> $ diff nsp-out psp-out\n> 12a13\n> > SP blame.c\n> 42a44,46\n> > diff.c:813:6: warning: symbol 'emit_line' was not declared. Should it be static?\n> > diff.c:828:6: warning: symbol 'emit_line_fmt' was not declared. Should it be static?\n> > diff.c:1865:6: warning: symbol 'print_stat_summary_0' was not declared. Should it be static?\n> 54a59\n> > SP fsmonitor.c\n> 137a143\n> > SP sub-process.c\n> 170a177\n> > SP compat/fopen.c\n> 276a284\n> > builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer\n> 296a305\n> > SP t/helper/test-dir-iterator.c\n> $ \n\nInteresting. One thing that I found somewhat suboptimal is that we\ndo not get signalled by non-zero exit. Otherwise it would make a\ngood addition to the \"Static Analysis\" task in .travis.yml file.\n\n\n"},{"id":"320595","messageId":"84e547c2-8a5b-225c-1363-361e091821f4@ramsayjones.plus.com","threadId":"46018","inReplyTo":"xmqqpof0memv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] usage: add NORETURN to BUG() function definitions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-23T20:47:52Z","receivedAt":"2017-05-23T20:48:17Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 23/05/17 04:32, Junio C Hamano wrote:\n> Interesting. One thing that I found somewhat suboptimal is that we\n> do not get signalled by non-zero exit.\n\nWarnings don't lead to non-zero exit, but similarly to -Werror, you can\nprovide a -Wsparse-error to turn warnings into errors:\n\n $ make builtin/worktree.sp\n SP builtin/worktree.c\n builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer\n $ \n $ make SPARSE_FLAGS=-Wsparse-error builtin/worktree.sp\n SP builtin/worktree.c\n builtin/worktree.c:539:38: error: Using plain integer as NULL pointer\n Makefile:2370: recipe for target 'builtin/worktree.sp' failed\n make: *** [builtin/worktree.sp] Error 1\n $ \n\nUnfortunately, that does not help too much because, as I mentioned before,\none warning is actually a sparse problem (and you can't turn it off):\n\n $ make pack-revindex.sp\n SP pack-revindex.c\n pack-revindex.c:64:23: warning: memset with byte count of 262144\n $ \n\nThis is caused by sparse _unconditionally_ complaining about the byte count\nused in calls to memset(), memcpy(), copy_to_user() and copy_from_user().\nIn addition, the byte count limits are hard-coded (v <= 0 || v > 100000).\nAbout a decade ago, I wrote a patch to enable/set the limit value from the\ncommand line, but didn't get around to sending the patch upstream. :-D\n \n[There is actually another problem warning, if you build with NO_REGEX=1].\n\nSince cgcc was intended to be used as proxy for gcc, you might think you\ncould use CC=cgcc on a regular build, but that has problems of it's own:\n\n $ make clean >/dev/null 2>&1 # on 'pu' branch, build output in 'pout'\n $ make CC=cgcc >pout1 2>&1\n $ diff pout pout1\n 99a100\n > pack-revindex.c:64:23: warning: memset with byte count of 262144\n 199a201\n > imap-send.c:1439:9: warning: expression using sizeof on a function\n 200a203,207\n > http.c:675:9: warning: expression using sizeof on a function\n > http.c:1676:25: warning: expression using sizeof on a function\n > http.c:1681:25: warning: expression using sizeof on a function\n > http.c:2082:9: warning: expression using sizeof on a function\n > http.c:2249:9: warning: expression using sizeof on a function\n 219a227\n > http-walker.c:377:9: warning: expression using sizeof on a function\n 222a231,233\n > http-push.c:189:9: warning: expression using sizeof on a function\n > http-push.c:200:9: warning: expression using sizeof on a function\n > http-push.c:202:9: warning: expression using sizeof on a function\n 228a240,243\n > remote-curl.c:524:9: warning: expression using sizeof on a function\n > remote-curl.c:605:17: warning: expression using sizeof on a function\n > remote-curl.c:608:17: warning: expression using sizeof on a function\n > remote-curl.c:676:9: warning: expression using sizeof on a function\n 374a390\n > builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer\n \n ...\n\n $ \n\nSee commit 9371322a60 (sparse: suppress some \"using sizeof on a function\"\nwarnings, 06-10-2013) for an explanation of the additional warnings.\nI chose the SPARSE_FLAGS method to suppress those warnings, precisely\nbecause I don't build git that way. (git grep -n SPARSE_FLAGS).\n\nSo, using CC='cgcc -Wsparse-error' as it stands isn't much help:\n \n $ make clean >/dev/null 2>&1\n $ make CC='cgcc -Wsparse-error'\n GIT_VERSION = 2.13.0.530.g896b4ae59\n * new build flags\n CC credential-store.o\n * new link flags\n CC common-main.o\n \n ...\n \n CC pack-objects.o\n CC pack-revindex.o\n pack-revindex.c:64:23: error: memset with byte count of 262144\n Makefile:2036: recipe for target 'pack-revindex.o' failed\n make: *** [pack-revindex.o] Error 1\n $ \n\n> Otherwise it would make a\n> good addition to the \"Static Analysis\" task in .travis.yml file.\n\nUnfortunately, some additional work required. :-P\n\nATB,\nRamsay Jones\n\n\n"}]}