On Mon, Mar 06, 2006 at 12:50:15PM -0400, Marc G. Fournier wrote:
> >I thought we ran the Converity analysis a year ago and cleaned up the
> >warnings, so I am surprised at our high number, but I assume they are
> >mostly noise.
>
> Got an account and will take a look at the details this evening ...

Are the people who look at the results allowed to tell the rest of us
what kind of bugs are involved (ie trivial or otherwise)? I mean,
presumably they can't cut and paste the results to the mailing list but
are we going to see anything other than a list of bug-fixes for
(apparently) unreported bugs?

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFEDVE5IB7bNG8LQkwRAgfPAJ9WlSAal6lFp03IneYIH1 dcNGli+ACeJg/5
svmrBVEkMmvAfzcjjEOk4JU=
=sSnN
-----END PGP SIGNATURE-----

Neil Conway wrote:

> I'm a bit surprised to see that there are ~300 unfixed defects: AFAIR I
> fixed all the issues the EDB guys passed on to me, with the exception of
> some false positives and a handful of minor issues in ECPG that I
> couldn't be bothered fixing (frankly I would rather not touch the ECPG
> code). I've requested access to the Coverity results -- I'll be curious
> to see if we can get any more useful fixes from the tool.

It's not unlikely that the checker got improved.

FWIW I don't see any report related to the MemoryContext stuff, which I
was expecting we would be flooded with. I haven't seen the entire list
yet, but they do make the mistake of not noticing that ereport(ERROR)
does not continue execution. For example:

-------->-------->-------->-------->-------->-------->-------->-------->
pgsql/src/backend/optimizer/plan/createplan.c

638 foreach(l, uniq_exprs)
639 {
640 Node *uniqexpr = lfirst(l);
641 TargetEntry *tle;
642
643 tle = tlist_member(uniqexpr, newtlist);

Event var_compare_op: Added "tle" due to comparison "tle == 0"
Also see events: [var_deref_op]
At conditional (1): "tle == 0" taking true path

644 if (!tle) /* shouldn't happen */
645 elog(ERROR, "failed to find unique expression in subplan tlist");

Event var_deref_op: Variable "tle" tracked as NULL was dereferenced.
Also see events: [var_compare_op]

646 groupColIdx[groupColPos++] = tle->resno;
647 }
-------->-------->-------->-------->-------->-------->-------->-------->

We all know that this is not a bug. There are lots of these, as you can
imagine.

There are lots of other "not a bugs". For example in initdb, there is a
lot of noise about how we don't free the resources.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

Alvaro Herrera <alvherre@commandprompt.com> writes:

> but they do make the mistake of not noticing that ereport(ERROR)
> does not continue execution.

There is a way in gcc to indicate that a function never returns. But in
Postgres it's a bit weird since elog()/ereport() sometimes return and
sometimes don't. Perhaps it would be better to make the forms that don't
return separate functions. Then hopefully they can be marked to not trigger
these kinds of warnings.

> We all know that this is not a bug. There are lots of these, as you can
> imagine.
>
> There are lots of other "not a bugs". For example in initdb, there is a
> lot of noise about how we don't free the resources.

Presumably the reason they don't immediately release the reports is for fear
of security holes revealed. Once somebody has checked the errors and checked
for any exploitable holes would it be possible to just open access to anyone
so mere mortals can clean up the mundane bugs?

--
greg


---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

On Tue, Mar 07, 2006 at 05:10:44PM -0500, Greg Stark wrote:
>
> Alvaro Herrera <alvherre@commandprompt.com> writes:
>
> > but they do make the mistake of not noticing that ereport(ERROR)
> > does not continue execution.
>
> There is a way in gcc to indicate that a function never returns. But in
> Postgres it's a bit weird since elog()/ereport() sometimes return and
> sometimes don't. Perhaps it would be better to make the forms that don't
> return separate functions. Then hopefully they can be marked to not trigger
> these kinds of warnings.

I think the problem is that both of those are macros that expand into
calls to errstart and errfinish. The error level is passed to errstart
but the actual exception is thrown in errfinish using the value stored
on the exception stack. For a static analysis tool to pick that up
would be quite a trick. For gcc I wouldn't bet on it.

One possibility would be to add code to the elog/ereport macros that is
only used when using one of these tools. For example:

#ifdef STATIC_ANALYSIS
#define ereport(elevel, rest) \
(errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
(errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
#else
/* Normal def */
#endif

The actual code never gets executed but it would give gcc and any other
tools the info they need to handle this situation.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFEDgeoIB7bNG8LQkwRAtJPAJ9WVptXTEbMI+/HOhPQ4LTmOnkXHwCeJ/Te
IRKfidVZIJ31qG7SBTD4HWo=
=bx84
-----END PGP SIGNATURE-----

Martijn van Oosterhout <kleptog@svana.org> writes:
> #ifdef STATIC_ANALYSIS
> #define ereport(elevel, rest) \
> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
> #else
> /* Normal def */
> #endif

Hmm, neat idea ... though I wonder whether either gcc or Coverity's tool
is smart enough to draw the right conclusions from a conditional exit()
call ...

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

On Tue, Mar 07, 2006 at 05:39:18PM -0500, Tom Lane wrote:
> Martijn van Oosterhout <kleptog@svana.org> writes:
> > #ifdef STATIC_ANALYSIS
> > #define ereport(elevel, rest) \
> > (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
> > (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
> > #else
> > /* Normal def */
> > #endif
>
> Hmm, neat idea ... though I wonder whether either gcc or Coverity's tool
> is smart enough to draw the right conclusions from a conditional exit()
> call ...

Well, remember this is a macro so the conditional is known at compile
time and the optimiser should see that the exit is unconditional. A
quick test with the attached program shows that gcc does correctly
determine that the last few lines are unreachable and are optimised out
entirely (with -Wunreachable-code which is not the default).

I tried to create an empty static inline function with
attribute((noreturn)) to optimise out the call to exit(), but gcc
merely points out the function does actually return and proceeds to
assume that the rest of main() is also reachable.

Another possibility would be to create two versions of errfinish, one
marked (noreturn), and use a conditional on elevel to decide which to
use. However, then you get issues with multiple evaluation of macro
arguments...

gcc 3.3.5
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFEDo/EIB7bNG8LQkwRAnX+AKCFloxU/JqlGf8xwutz669io14iUwCff/oA
c6qD3t5tumvnarz9qDpMcSo=
=RDx5
-----END PGP SIGNATURE-----

Martijn van Oosterhout wrote:
> On Tue, Mar 07, 2006 at 05:39:18PM -0500, Tom Lane wrote:
>
>>Martijn van Oosterhout <kleptog@svana.org> writes:
>>
>>>#ifdef STATIC_ANALYSIS
>>>#define ereport(elevel, rest) \
>>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
>>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
>>>#else
>>>/* Normal def */
>>>#endif
>>
>>Hmm, neat idea ... though I wonder whether either gcc or Coverity's tool
>>is smart enough to draw the right conclusions from a conditional exit()
>>call ...
>

As for Coverity, if the elevel that's passed to the ereport is really a
constant, the above #ifdef should absolutely do the trick for us so we
know to stop analyzing on that path...Let me know if it doesn't actually
do that

-ben


---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

Folks,
> As for Coverity, if the elevel that's passed to the ereport is really a
> constant, the above #ifdef should absolutely do the trick for us so we
> know to stop analyzing on that path...Let me know if it doesn't actually
> do that

Um, I think the answer is to train Coverity, not change our code to avoid
the false-positives. I know that Coverity is sophisticated enough to, for
example, be programed to understand that elog(ERROR) does not continue.

Actually, I thougth that Neil/eDB did this with their copy. Is there any
way to get a copy of that "training configuration"?

--
--Josh

Josh Berkus
Aglio Database Solutions
San Francisco

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

On 3/8/06, Josh Berkus <josh@agliodbs.com> wrote:
>
> Actually, I thougth that Neil/eDB did this with their copy. Is there any
> way to get a copy of that "training configuration"?


I think we have a backup of it somewhere. I'll look into it.


--
Jonah H. Harris, Database Internals Architect
EnterpriseDB Corporation
732.331.1324

Ben Chelf <ben@coverity.com> writes:

> >>>#ifdef STATIC_ANALYSIS
> >>>#define ereport(elevel, rest) \
> >>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
> >>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
> >>>#else
> >>>/* Normal def */
> >>>#endif
>
> As for Coverity, if the elevel that's passed to the ereport is really a
> constant, the above #ifdef should absolutely do the trick for us so we know to
> stop analyzing on that path...Let me know if it doesn't actually do that

If you're willing to require elevel to always be a constant then why not just
tack on the (elevel >= ERROR ? exit(0) : 0) onto the end of the regular
definition of ereport instead of having an ifdef?

Incidentally, if it's not guaranteed to be a constant then the definition
above is wrong because it's missing parentheses around elevel at both
occurrences.

--
greg


---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match