{"thread":{"id":"45894","subject":"git and the Clang Static Analyzer","startedAt":"2017-05-07T21:43:42Z","lastAt":"2017-05-08T20:19:04Z","messageCount":5,"participants":["Дилян Палаузов","Johannes Schindelin","Lars Schneider","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"319011","messageId":"5696a8a3-0a17-18ac-8da4-4556586acee7@aegee.org","threadId":"45894","inReplyTo":null,"subject":"git and the Clang Static Analyzer","fromName":"Дилян Палаузов","fromEmail":"dilyan.palauzov@aegee.org","sentAt":"2017-05-07T11:50:54Z","receivedAt":"2017-05-07T21:43:42Z","isPatch":false,"sender":{"key":"dilyan.palauzov@aegee.org","avatar":null},"body":"Hello,\n\nI compiled git/master using the clang 4.0 static analyzer with\n\nscan-build ./configure --with-libpcre --with-openssl\nscan-build make\n\nand here are the results:\n   https://mail.aegee.org/dpa/scan-build-git-4fa66c85f11/\n\nPlease note, that the information is only about what gets actually \ncompiled, code disabled by #if .. #endif is not considered (e.g. when \ndetermining whether a variable assignment is useless).  There are \nprobably false-positives.  However in case of e.g. builtin/notes.c:1018, \nbuiltin/reset.c:294 or fast-import.c:2057 I consider the hints as justified.\n\nThis is for your information,  I wouldn't have a problem if you ignore \nthe analysis.  When you are worried about javascript, use lynx.\n\nRegards\n   Дилян\n"},{"id":"319077","messageId":"alpine.DEB.2.21.1.1705081250550.146734@virtualbox","threadId":"45894","inReplyTo":"5696a8a3-0a17-18ac-8da4-4556586acee7@aegee.org","subject":"Re: git and the Clang Static Analyzer","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-08T11:12:50Z","receivedAt":"2017-05-08T11:13:26Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Dilyan,\n\n\nOn Sun, 7 May 2017, Дилян Палаузов wrote:\n\n> I compiled git/master\n\n... which advances from time to time, so you definitely want to include a\nmore informative data point here, e.g. 4fa66c85f11 (Git 2.13-rc2,\n2017-05-04) ...\n\n> using the clang 4.0 static analyzer with\n> \n> scan-build ./configure --with-libpcre --with-openssl\n> scan-build make\n> \n> and here are the results:\n>   https://mail.aegee.org/dpa/scan-build-git-4fa66c85f11/\n> \n> Please note, that the information is only about what gets actually compiled,\n> code disabled by #if .. #endif is not considered (e.g. when determining\n> whether a variable assignment is useless).\n\nSo you already know that the report is specific to your setup. It may make\na lot of sense to actually state what your setup is, i.e. Operating\nSystem, installed libraries (and their respective versions), CPU, etc.\n\n> There are probably false-positives.\n\nProbably. So why don't you give it a try and look through the report? Then\nsummarize your findings here. That would definitely find a warm welcome, I\nwould expect.\n\n> However in case of e.g. builtin/notes.c:1018, builtin/reset.c:294 or\n> fast-import.c:2057 I consider the hints as justified.\n\nOkay. And those hint are...?\n\nRemember: the easier you make it for your readers to follow your thoughts,\nthe more readers will try to do that.\n\n> This is for your information,  I wouldn't have a problem if you ignore\n> the analysis.\n\nThank you for running the scan.\n\nHowever, I would like to encourage you to do better than just call Clang\nand slap the report verbatim, without much in the way of a manual\nanalysis, onto some website.\n\nIn fact, I fear that doing it this way is rather counter-productive.\n\nInstead, you could try to organize the report a lot better (see the many\nmails about Coverity in the past):\n\n- we already *know* that static analyzers have tons of problems with the\n  way we specify \"FLEX_ARRAYS\": we define structs to have a 0-item or\n  1-item array as their last field, then use a variable-length malloc() to\n  tailor the allocated data to the needs of a variable-length array (most\n  often: string). To have these issues in one category would be very\n  helpful, as we could ignore those parts of the report easier.\n\n- we also already know quite well that Git's source code often abuses\n  exit() for a garbage collector on short-running processes: In many\n  cmd_*() functions, there are \"memory leaks\" that are intentional (read:\n  lazy). Again, it would be helpful to be able to fade out those reports.\n\n- Likewise, our test helpers abide by a lot less strict rules, as we only\n  use them in the test suite (and therefore it is not *really* necessary\n  to, say, validate the input passed as command-line parameters). Here\n  again, it would be very helpful to put those into a separate category\n  that can be selectively hidden.\n\n- there have been a number of patches floating about to fix one or more of\n  the issues reported by Coverity. These patches may not yet have made it\n  into `master`, but at least some of them are in `pu`. A careful analysis\n  which issues reported by Clang would be addressed by `pu` would also be\n  quite welcome.\n\nAlso: to make it *really* useful for other developers, it would be a good\nidea for you to try your hand at patching the .travis.yml file in such a\nway that the static analysis is performed as part of the Continuous\nTesting, and to contribute said patch. That way, other contributors could\nnot only see how it is done (and copy-paste the commands, including `apt\ninstall` calls insofar necessary), but also to see the reports on a\ntrusted website (Travis').\n\nI really would like to believe that you meant to be helpful in sending\nthis mail. But the style does come over as \"throwing a couple of scraps to\nthe dogs\". I do not believe that is what you wanted, and I think you can\nturn that impression around by at least some of the things I mentioned\nabove.\n\nCiao,\nJohannes"},{"id":"319092","messageId":"C925C220-DD28-41A0-BF72-F8A906BB460D@gmail.com","threadId":"45894","inReplyTo":"alpine.DEB.2.21.1.1705081250550.146734@virtualbox","subject":"Re: git and the Clang Static Analyzer","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-05-08T16:45:38Z","receivedAt":"2017-05-08T16:45:45Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 08 May 2017, at 12:12, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:\n> \n> Hi Dilyan,\n> \n> \n> On Sun, 7 May 2017, Дилян Палаузов wrote:\n> \n>> ...\n> \n> Also: to make it *really* useful for other developers, it would be a good\n> idea for you to try your hand at patching the .travis.yml file in such a\n> way that the static analysis is performed as part of the Continuous\n> Testing, and to contribute said patch. That way, other contributors could\n> not only see how it is done (and copy-paste the commands, including `apt\n> install` calls insofar necessary), but also to see the reports on a\n> trusted website (Travis').\n\nI did that here:\nhttps://github.com/larsxschneider/git/commit/faf4ecfdca1a732459c1f93c334928ee2826d490\n\nUnfortunately, there are still too many issues to activate this job:\nhttps://travis-ci.org/larsxschneider/git/jobs/205507241\n\nSee this thread for more info:\nhttp://public-inbox.org/git/BAB1EE0E-B258-4108-AE24-110172086DE4@gmail.com/\n\n- Lars"},{"id":"319110","messageId":"373a6f4b-1b4d-6c75-30b1-c2498555c3d9@aegee.org","threadId":"45894","inReplyTo":"alpine.DEB.2.21.1.1705081250550.146734@virtualbox","subject":"Re: git and the Clang Static Analyzer","fromName":"Дилян Палаузов","fromEmail":"dilyan.palauzov@aegee.org","sentAt":"2017-05-08T19:26:11Z","receivedAt":"2017-05-08T19:26:24Z","isPatch":false,"sender":{"key":"dilyan.palauzov@aegee.org","avatar":null},"body":"Hello Johannes,\n\n>> I compiled git/master\n>\n> ... which advances from time to time, so you definitely want to include a\n> more informative data point here, e.g. 4fa66c85f11 (Git 2.13-rc2,\n> 2017-05-04) ...\n>\n\nThe 4fa66c85f11 you mentioned is part of the URL I sent.\n\n>> Please note, that the information is only about what gets actually compiled,\n>> code disabled by #if .. #endif is not considered (e.g. when determining\n>> whether a variable assignment is useless).\n>\n> So you already know that the report is specific to your setup. It may make\n> a lot of sense to actually state what your setup is, i.e. Operating\n> System, installed libraries (and their respective versions), CPU, etc.\n\nI don't think this is of much relevance.  The hints provided encourage one to look at the code and to evaluate mentally the lines.  By tweaking the preprocessor directives, you could get less warnings (a previously unused variable now appears within an asser()), or more warnings (as more code gets compiled).  Getting more warnings makes sense, after the current ones are processed.  Getting less warnings means (again) compiling more code.  I use already pcre and openssl, what else can I enable?\n  \n>> There are probably false-positives.\n>\n> Probably. So why don't you give it a try and look through the report? Then\n> summarize your findings here. That would definitely find a warm welcome, I\n> would expect.\n>\n>> However in case of e.g. builtin/notes.c:1018, builtin/reset.c:294 or\n>> fast-import.c:2057 I consider the hints as justified.\n>\n> Okay. And those hint are...?\n\nClick on  https://mail.aegee.org/dpa/scan-build-git-4fa66c85f11/ and then on \"fast-import.c: line 2057 -> View Report\" and you will see pointless assignment.\n\nI cannot organize the report much better, as filtering out the false positives requires usually too deep understanding of the code organization of git, which I do not have.\n\nThis is the analysis done on the pu-branch:\n   https://mail.aegee.org/dpa/scan-build-git-7dd243c75\n\nBoth reports do not list files in the same order, as I did parallel builds, but I do not see on the spot any difference.\n\nLearning Travis is not on my priority list, I sent the commands I called to get the report.  I also compiled clang by myself.  For those who mistrust sites, there are no-javascipt, no-css browsers like lynx.\n\nGreetings\n   Dilyan\n"},{"id":"319114","messageId":"20170508201855.ki4rqry47pdt5u26@sigill.intra.peff.net","threadId":"45894","inReplyTo":"373a6f4b-1b4d-6c75-30b1-c2498555c3d9@aegee.org","subject":"Re: git and the Clang Static Analyzer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-08T20:18:56Z","receivedAt":"2017-05-08T20:19:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 08, 2017 at 09:26:11PM +0200, Дилян Палаузов wrote:\n\n> Click on  https://mail.aegee.org/dpa/scan-build-git-4fa66c85f11/ and\n> then on \"fast-import.c: line 2057 -> View Report\" and you will see\n> pointless assignment.\n> \n> I cannot organize the report much better, as filtering out the false\n> positives requires usually too deep understanding of the code\n> organization of git, which I do not have.\n\nRight, but I think Johannes's point is that we already know about\nscan-build. It's been discussed on the list multiple times. The next\nstep is somebody actually looking at each reported issue, fixing any\nproblematic ones, and suppressing (or at least cataloging) the false\npositives.\n\nHere's a link to the last discussion on this topic, from a few months\nago:\n\n  http://public-inbox.org/git/CAJZjrdXP3n5fOLx4rEEkbJT7JBMPUqk4Qdutm6KpvMVUMwCSPQ@mail.gmail.com/T/#u\n\nIt even involves a Travis build. :)\n\n-Peff\n"}]}