Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-19 Thread Tom Lane
Peter Eisentraut writes: > With the attached patch, that warning goes way, and the logic is > arguably slightly clearer, too. No objection. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: htt

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-19 Thread Peter Eisentraut
On Wed, 2013-11-13 at 12:43 -0500, Tom Lane wrote: > Kevin Grittner writes: > > If nobody objects, I'll fix that small memory leak in the > > regression test driver. Hopefully someone more familiar with > > pg_basebackup will fix the double-free (and related problems > > mentioned by Tom) in strea

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-13 Thread Tom Lane
Kevin Grittner writes: > If nobody objects, I'll fix that small memory leak in the > regression test driver.  Hopefully someone more familiar with > pg_basebackup will fix the double-free (and related problems > mentioned by Tom) in streamutil.c. Here's a less convoluted (IMHO) approach to the pa

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-13 Thread Kevin Grittner
Tom Lane wrote: > No, this isn't about test code vs production, it's about not bothering > to free memory explicitly when a program is about to terminate.  Alvaro > is suggesting that the proposed addition to pg_regress.c is just a waste > of cycles.  IMO it's not that big a deal either way in th

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Tom Lane
Jeffrey Walton writes: > I did not check any with the long path lengths, but the > `pqsecure_write` in fe-secure.c looks valid to me. `spinfo` is > declared, Clang builds/finds the path, then the unitializaed `spinfo` > is used in `RESTORE_SIGPIPE(conn, spinfo);`. It's junk AFAICS, though I will

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Peter Eisentraut
On Tue, 2013-11-12 at 19:11 -0500, Jeffrey Walton wrote: > The reports being generated with Clang 3.3 on Postgres 9.3.1 are > different that posted. For example, french.c is not listed in the > Clang 3.3 reports. What version of Clang is used in the online report? LLVM 3.0 -- Sent via pgsql-ha

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Tom Lane
Jeffrey Walton writes: > On Tue, Nov 12, 2013 at 7:11 PM, Alvaro Herrera > wrote: >> We have marked a large number of memory leak reports by Coverity in >> initdb and other short-lived programs as false positive, on the grounds >> that there's no point in freeing memory in a program that's about

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Jeffrey Walton
On Tue, Nov 12, 2013 at 7:11 PM, Alvaro Herrera wrote: > Kevin Grittner escribió: > >> These both seemed legitimate to me. Patch attached. Any >> objections to applying it? I realize the memory leak is a tiny one >> in the regression testing code, so it could never amount to enough >> to matter

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Jeffrey Walton
On Tue, Nov 12, 2013 at 5:19 PM, Peter Eisentraut wrote: > On 11/12/13, 8:18 AM, Kevin Grittner wrote: >> Here is the summary of what was reported: >> >> All Bugs: 313 > >> Does anything stand out as something that is particularly worth >> looking into? Does anything here seem worth assuming is

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Alvaro Herrera
Kevin Grittner escribió: > These both seemed legitimate to me.  Patch attached.  Any > objections to applying it?  I realize the memory leak is a tiny one > in the regression testing code, so it could never amount to enough > to matter; but it seems worth fixing just to avoid noise in code > analy

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Jeffrey Walton
On Tue, Nov 12, 2013 at 6:04 PM, Kevin Grittner wrote: > Peter Eisentraut wrote: > >> I have tracked scan-build for some time, and I'm sure that almost >> all of these bugs are false positives at this point. > > From poking around, I agree. One particular error I noticed that > it makes a lot is

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Kevin Grittner
Peter Eisentraut wrote: > I have tracked scan-build for some time, and I'm sure that almost > all of these bugs are false positives at this point. From poking around, I agree.  One particular error I noticed that it makes a lot is that in a loop it says that an assigned value is not referenced i

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Peter Eisentraut
On 11/12/13, 8:18 AM, Kevin Grittner wrote: > Here is the summary of what was reported: > > All Bugs: 313 > Does anything stand out as something that is particularly worth > looking into? Does anything here seem worth assuming is completely > bogus because of the Coverity and Valgrind passes?

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Jeffrey Walton
On Tue, Nov 12, 2013 at 3:35 PM, Andres Freund wrote: > On 2013-11-12 15:33:13 -0500, Jeffrey Walton wrote: >> On Tue, Nov 12, 2013 at 3:25 PM, Andres Freund >> wrote: >> > On 2013-11-12 15:17:18 -0500, Jeffrey Walton wrote: >> > ... >> > It might not recognize our Assert() because it expands as

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Jeffrey Walton
On Tue, Nov 12, 2013 at 9:38 AM, Tom Lane wrote: > Kevin Grittner writes: >> Does anything stand out as something that is particularly worth >> looking into? Does anything here seem worth assuming is completely >> bogus because of the Coverity and Valgrind passes? > > I thought most of it was ob

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Andres Freund
On 2013-11-12 15:33:13 -0500, Jeffrey Walton wrote: > On Tue, Nov 12, 2013 at 3:25 PM, Andres Freund wrote: > > On 2013-11-12 15:17:18 -0500, Jeffrey Walton wrote: > >> On Tue, Nov 12, 2013 at 9:38 AM, Tom Lane wrote: > >> > ... > >> > One thought for the Clang people is that most of the reports

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Jeffrey Walton
On Tue, Nov 12, 2013 at 3:25 PM, Andres Freund wrote: > On 2013-11-12 15:17:18 -0500, Jeffrey Walton wrote: >> On Tue, Nov 12, 2013 at 9:38 AM, Tom Lane wrote: >> > ... >> > One thought for the Clang people is that most of the reports such as "null >> > pointer dereference" presumably mean "I thi

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Andres Freund
On 2013-11-12 15:17:18 -0500, Jeffrey Walton wrote: > On Tue, Nov 12, 2013 at 9:38 AM, Tom Lane wrote: > > ... > > > > One thought for the Clang people is that most of the reports such as "null > > pointer dereference" presumably mean "I think I see an execution path > > whereby we could get here

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Jeffrey Walton
On Tue, Nov 12, 2013 at 9:38 AM, Tom Lane wrote: > ... > > One thought for the Clang people is that most of the reports such as "null > pointer dereference" presumably mean "I think I see an execution path > whereby we could get here with a null pointer". If so, it'd be awfully > helpful if the c

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Tom Lane
Kevin Grittner writes: > Kevin Grittner wrote: >> Memory Error >>    Double free:  1 >>    Memory leak:  1 > These both seemed legitimate to me.  Patch attached.  Any > objections to applying it?  I realize the memory leak is a tiny one > in the regression testing code, so it could never amount

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Kevin Grittner
Kevin Grittner wrote: > Memory Error >   Double free:  1 >   Memory leak:  1 These both seemed legitimate to me.  Patch attached.  Any objections to applying it?  I realize the memory leak is a tiny one in the regression testing code, so it could never amount to enough to matter; but it seems wo

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote: > I think Coverity does that, or at least I've seen output from some tool > that does it. Coverity does provide the path (including going through multiple interations of a loop, if applicable). Doesn't make it perfect, sadly, but I've been trying to feed bac

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Tom Lane
Kevin Grittner writes: > Does anything stand out as something that is particularly worth > looking into?  Does anything here seem worth assuming is completely > bogus because of the Coverity and Valgrind passes? I thought most of it was obvious junk: if there were actually uninitialized-variable

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Stephen Frost
* Peter Geoghegan (p...@heroku.com) wrote: > I seem to also recall > Coverity correctly handling that, although perhaps I'm unfairly > crediting them with taking advantage of the abort() trick because of > the state of Postgres when I tried each of those two tools - it might > be that scan-build *w

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Kevin Grittner
Kevin Grittner wrote: > Logic error >   Stack address stored into global variable:  1 I took a look at this one, and it is a totally legitimate use, the reason for which is explained with this comment: /*  * check_stack_depth: check for excessively deep recursion  *  * This should be called so

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-12 Thread Kevin Grittner
Tom Lane wrote: > quite a lot of people have looked at Postgres with Coverity > already.  If Clang is throwing up lots and lots of warnings, the > odds are *very* high that most of them are false positives. > Running through such a list to see if there's anything real isn't > all that exciting a

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-11 Thread Tom Lane
Kevin Grittner writes: > It does seem hard to believe that clang tools would find as enough > problems that were missed by Coverity and Valgrind to account for > all the warnings that are scrolling by; but it looks like it has > pointed out at least *one* problem that's worth fixing. Yeah, that's

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-11 Thread Jeffrey Walton
On Mon, Nov 11, 2013 at 6:01 PM, Kevin Grittner wrote: > Peter Geoghegan wrote: >> Kevin Grittner wrote: >> >>> I'm currently capturing a text version of all the warnings from >>> this. Will gzip and post when it finishes. It's generating a lot >>> of warnings; I have no idea how many are Post

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-11 Thread Kevin Grittner
Peter Geoghegan wrote: > Kevin Grittner wrote: > >> I'm currently capturing a text version of all the warnings from >> this.  Will gzip and post when it finishes.  It's generating a lot >> of warnings; I have no idea how many are PostgreSQL problems and >> how many are false positives; will just

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-11 Thread Jeffrey Walton
On Mon, Nov 11, 2013 at 5:51 PM, Peter Geoghegan wrote: > On Mon, Nov 11, 2013 at 2:45 PM, Jeffrey Walton wrote: >> I think you are right. Coverity is a very nice tool, and Clang has >> some growing to do. > > To be fair to the LLVM/Clang guys, it's not as if static analysis is a > very high prio

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-11 Thread Jeffrey Walton
On Mon, Nov 11, 2013 at 5:18 PM, Kevin Grittner wrote: > [moving the discussion to pgsql-hackers] > > Jeffrey Walton wrote: >> ... >> ## >> # Sanitizers >> >> make distclean >> >> export DYLD_FALLBACK_LIBRARY_PATH=/usr/local/lib/clang/3.3/lib/darwin/ >> export CC=/usr/local/bin/clang

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-11 Thread Peter Geoghegan
On Mon, Nov 11, 2013 at 2:45 PM, Jeffrey Walton wrote: > I think you are right. Coverity is a very nice tool, and Clang has > some growing to do. To be fair to the LLVM/Clang guys, it's not as if static analysis is a very high priority for them. -- Peter Geoghegan -- Sent via pgsql-hackers

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-11 Thread Jeffrey Walton
On Mon, Nov 11, 2013 at 5:29 PM, Peter Geoghegan wrote: > On Mon, Nov 11, 2013 at 2:18 PM, Kevin Grittner wrote: >> I'm currently capturing a text version of all the warnings from >> this. Will gzip and post when it finishes. It's generating a lot >> of warnings; I have no idea how many are Pos

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-11 Thread Peter Geoghegan
On Mon, Nov 11, 2013 at 2:18 PM, Kevin Grittner wrote: > I'm currently capturing a text version of all the warnings from > this. Will gzip and post when it finishes. It's generating a lot > of warnings; I have no idea how many are PostgreSQL problems and > how many are false positives; will just

Re: [HACKERS] Clang 3.3 Analyzer Results

2013-11-11 Thread Kevin Grittner
[moving the discussion to pgsql-hackers] Jeffrey Walton wrote: > The Analyzer is invoked with scan-build. Its used when compiling > the package because it performs static analysis. > > The Santizers are invoked with the runtime flags. They are used > with the `check` program because they perform