Horton

Thread

Commits

  1. Silence compiler warnings

  2. ilence compiler warnings

  3. Silence Bison deprecation warnings

  4. Suppress clang's unhelpful gripes about -pthread switch being unused.

  1. Compiler warnings

    Stephen Frost <sfrost@snowman.net> — 2016-11-29T15:21:02Z

    Greetings,

Not sure if anyone else has been seeing these, but I'm getting a bit
tired of them.  Neither is a live bug, but they also seem pretty simple
to fix.  The attached patch makes both of these warnings go away.  At
least for my common build, these are the only warnings that are thrown.

I'm building with:

gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609

.../src/backend/storage/lmgr/lwlock.c: In function ‘LWLockRelease’:
.../src/backend/storage/lmgr/lwlock.c:1802:5: warning: ‘mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  if (mode == LW_EXCLUSIVE)
     ^
.../src/backend/utils/cache/plancache.c: In function ‘GetCachedPlan’:
.../src/backend/utils/cache/plancache.c:1232:9: warning: ‘plan’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  return plan;
         ^

Thoughts?

Thanks!

Stephen

  2. Re: Compiler warnings

    Stephen Frost <sfrost@snowman.net> — 2016-11-29T15:25:38Z

    * Stephen Frost (sfrost@snowman.net) wrote:
> diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
> new file mode 100644
> index 884cdab..b5d97c8
> *** a/src/backend/utils/cache/plancache.c
> --- b/src/backend/utils/cache/plancache.c
> *************** GetCachedPlan(CachedPlanSource *plansour
> *** 1196,1204 ****
>   			 */
>   			qlist = NIL;
>   		}
> ! 	}
> ! 
> ! 	if (customplan)
>   	{
>   		/* Build a custom plan */
>   		plan = BuildCachedPlan(plansource, qlist, boundParams);
> --- 1196,1203 ----
>   			 */
>   			qlist = NIL;
>   		}
> ! 	} 
> ! 	else
>   	{
>   		/* Build a custom plan */
>   		plan = BuildCachedPlan(plansource, qlist, boundParams);

Meh, of course this isn't correct since we could change 'customplan'
inside the first if() block to be true, the right answer is really to
just do:

CachedPlan *plan = NULL;

at the top and keep it simple.

ENEEDMORECOFFEE.

Thanks!

Stephen

  3. Re: Compiler warnings

    Stephen Frost <sfrost@snowman.net> — 2016-12-06T20:23:05Z

    All,

* Stephen Frost (sfrost@snowman.net) wrote:
> Not sure if anyone else has been seeing these, but I'm getting a bit
> tired of them.  Neither is a live bug, but they also seem pretty simple
> to fix.  The attached patch makes both of these warnings go away.  At
> least for my common build, these are the only warnings that are thrown.
> 
> I'm building with:
> 
> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
> 
> .../src/backend/storage/lmgr/lwlock.c: In function ‘LWLockRelease’:
> .../src/backend/storage/lmgr/lwlock.c:1802:5: warning: ‘mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   if (mode == LW_EXCLUSIVE)
>      ^
> .../src/backend/utils/cache/plancache.c: In function ‘GetCachedPlan’:
> .../src/backend/utils/cache/plancache.c:1232:9: warning: ‘plan’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   return plan;
>          ^

Given the lack of screaming, I'll push the attached in a bit, which just
initializes the two variables being complained about.  As mentioned,
there doesn't appear to be any live bugs here, this is just to silence
the compiler warnings.

Thanks!

Stephen

  4. Re: Compiler warnings

    Robert Haas <robertmhaas@gmail.com> — 2016-12-06T20:31:16Z

    On Tue, Dec 6, 2016 at 3:23 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Stephen Frost (sfrost@snowman.net) wrote:
>> Not sure if anyone else has been seeing these, but I'm getting a bit
>> tired of them.  Neither is a live bug, but they also seem pretty simple
>> to fix.  The attached patch makes both of these warnings go away.  At
>> least for my common build, these are the only warnings that are thrown.
>>
>> I'm building with:
>>
>> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
>>
>> .../src/backend/storage/lmgr/lwlock.c: In function ‘LWLockRelease’:
>> .../src/backend/storage/lmgr/lwlock.c:1802:5: warning: ‘mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>   if (mode == LW_EXCLUSIVE)
>>      ^
>> .../src/backend/utils/cache/plancache.c: In function ‘GetCachedPlan’:
>> .../src/backend/utils/cache/plancache.c:1232:9: warning: ‘plan’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>   return plan;
>>          ^
>
> Given the lack of screaming, I'll push the attached in a bit, which just
> initializes the two variables being complained about.  As mentioned,
> there doesn't appear to be any live bugs here, this is just to silence
> the compiler warnings.

In LWLockRelease, why not just move mode = held_lwlocks[i].mode; after
the if (i < 0) elog(...), instead of initializing with a bogus value?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  5. Re: Compiler warnings

    Stephen Frost <sfrost@snowman.net> — 2016-12-06T20:46:44Z

    Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, Dec 6, 2016 at 3:23 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > Given the lack of screaming, I'll push the attached in a bit, which just
> > initializes the two variables being complained about.  As mentioned,
> > there doesn't appear to be any live bugs here, this is just to silence
> > the compiler warnings.
> 
> In LWLockRelease, why not just move mode = held_lwlocks[i].mode; after
> the if (i < 0) elog(...), instead of initializing with a bogus value?

Good thought, thanks!

Updated patch attached with that change and I also added an Assert() to
GetCachedPlan(), in case that code gets whacked around later and somehow
we end up falling through without actually setting *plan.

Thoughts?

Thanks!

Stephen

  6. Re: Compiler warnings

    Robert Haas <robertmhaas@gmail.com> — 2016-12-06T21:59:47Z

    On Tue, Dec 6, 2016 at 3:46 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Good thought, thanks!
>
> Updated patch attached with that change and I also added an Assert() to
> GetCachedPlan(), in case that code gets whacked around later and somehow
> we end up falling through without actually setting *plan.
>
> Thoughts?

wfm

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  7. Re: Compiler warnings

    Joe Conway <mail@joeconway.com> — 2016-12-29T15:47:28Z

    On 12/06/2016 01:59 PM, Robert Haas wrote:
> On Tue, Dec 6, 2016 at 3:46 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> Good thought, thanks!
>>
>> Updated patch attached with that change and I also added an Assert() to
>> GetCachedPlan(), in case that code gets whacked around later and somehow
>> we end up falling through without actually setting *plan.
>>
>> Thoughts?
> 
> wfm
> 

Shouldn't this be back-patched? The plancache warning goes back through
9.2 (at least) and the lwlocks warning through 9.5 (or maybe it was 9.4).

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  8. Re: Compiler warnings

    Robert Haas <robertmhaas@gmail.com> — 2017-01-02T18:18:29Z

    On Thu, Dec 29, 2016 at 10:47 AM, Joe Conway <mail@joeconway.com> wrote:
> Shouldn't this be back-patched? The plancache warning goes back through
> 9.2 (at least) and the lwlocks warning through 9.5 (or maybe it was 9.4).

Warnings are going to be different for each individual developer, but
I am cautiously in favor making more of an effort to fix back-branch
warnings provided that it doesn't generate too much code churn.  For
example, if your toolchain generates only these two warnings on 9.2
then, sure, let's back-port these two fixes; making things
warning-clean is great.  But if there are dozens or hundreds of
warnings currently, fixing only a handful of those warnings probably
isn't valuable, and fixing all of them is probably a little more risk
than we necessarily want to take.  Someone could goof and make a bug.
On my MacBook Pro with my toolchain, we're warning-clean back to 9.3
or 9.4, and before that there are some problems -- most annoyingly the
fact that 73b416b2e41237b657d29d8f42a4bb34bf700928 couldn't be easily
backported to older branches.  I don't think it would be crazy to try
to get all of the warnings I see fixed up and it would be convenient
for me, but I haven't been willing to do the work, either.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: Compiler warnings

    Joe Conway <mail@joeconway.com> — 2017-01-02T18:55:48Z

    On 01/02/2017 10:18 AM, Robert Haas wrote:
> On Thu, Dec 29, 2016 at 10:47 AM, Joe Conway <mail@joeconway.com> wrote:
>> Shouldn't this be back-patched? The plancache warning goes back through
>> 9.2 (at least) and the lwlocks warning through 9.5 (or maybe it was 9.4).
> 
> Warnings are going to be different for each individual developer, but
> I am cautiously in favor making more of an effort to fix back-branch
> warnings provided that it doesn't generate too much code churn.  For
> example, if your toolchain generates only these two warnings on 9.2
> then, sure, let's back-port these two fixes; making things
> warning-clean is great.  But if there are dozens or hundreds of
> warnings currently, fixing only a handful of those warnings probably
> isn't valuable, and fixing all of them is probably a little more risk
> than we necessarily want to take.  Someone could goof and make a bug.
> On my MacBook Pro with my toolchain, we're warning-clean back to 9.3
> or 9.4, and before that there are some problems -- most annoyingly the
> fact that 73b416b2e41237b657d29d8f42a4bb34bf700928 couldn't be easily
> backported to older branches.  I don't think it would be crazy to try
> to get all of the warnings I see fixed up and it would be convenient
> for me, but I haven't been willing to do the work, either.
> 

FWIW, I'm running Mint 18 which is based on Unbuntu 16.04 I believe. On
the 9.2 and 9.3 branches I see two warnings:

This one once:
---------------
plancache.c:1197:9: warning: ‘plan’ may be used uninitialized in this
function [-Wmaybe-uninitialized]

And this one once per bison file:
---------------
gram.y:169.1-13: warning: deprecated directive, use ‘%name-prefix’
[-Wdeprecated]
 %name-prefix="base_yy"
 ^^^^^^^^^^^^^

The plancache.c fix in Stephen's patch was really simple: initialize
plan = NULL and add an assert. To me that seems like something we should
definitely back-patch.

On the other hand, seems like we have had bison related warnings of one
kind or another since I first got involved with Postgres. So while it
would be nice to make that one go away, it somehow bothers me less. It
also goes away starting in 9.4 anyway.


Starting in 9.5 I only get the plancache.c warning and this one:
---------------
lwlock.c:1555:5: warning: ‘mode’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
  if (mode == LW_EXCLUSIVE)
     ^

That is the other warning fixed in Stephens commit to master.

For the sake of completeness, in 9.4. I get the plancache.c warning and
this one:
---------------
basebackup.c:1284:6: warning: variable ‘wait_result’ set but not used
[-Wunused-but-set-variable]
  int   wait_result;
      ^

Seems like that should be easily fixed.

If there is agreement on fixing these warnings, other than the bison
generated warning, I would be happy to do it. I'd also be happy to look
for a fix the bison warning as well if desired, but that should be
handled separately I would think.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  10. Re: Compiler warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-02T19:09:05Z

    Joe Conway <mail@joeconway.com> writes:
> If there is agreement on fixing these warnings, other than the bison
> generated warning, I would be happy to do it. I'd also be happy to look
> for a fix the bison warning as well if desired, but that should be
> handled separately I would think.

The bison issue is discussed in
https://www.postgresql.org/message-id/flat/E1WpjkB-0003zA-N4%40gemulon.postgresql.org

			regards, tom lane

  11. Re: Compiler warnings

    Joe Conway <mail@joeconway.com> — 2017-01-02T19:51:05Z

    On 01/02/2017 11:09 AM, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> If there is agreement on fixing these warnings, other than the bison
>> generated warning, I would be happy to do it. I'd also be happy to look
>> for a fix the bison warning as well if desired, but that should be
>> handled separately I would think.
> 
> The bison issue is discussed in
> https://www.postgresql.org/message-id/flat/E1WpjkB-0003zA-N4%40gemulon.postgresql.org

Ah, thanks. I vaguely remember that thread now.

Looks like there was some consensus for applying Peter's patch with the
addition of a comment, but apparently that never happened. Would we
still consider that for 9.2 and 9.3 branches?

Any thoughts on fixing the other warnings?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  12. Re: Compiler warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-02T19:58:34Z

    Joe Conway <mail@joeconway.com> writes:
> On 01/02/2017 11:09 AM, Tom Lane wrote:
>> The bison issue is discussed in
>> https://www.postgresql.org/message-id/flat/E1WpjkB-0003zA-N4%40gemulon.postgresql.org

> Ah, thanks. I vaguely remember that thread now.

> Looks like there was some consensus for applying Peter's patch with the
> addition of a comment, but apparently that never happened. Would we
> still consider that for 9.2 and 9.3 branches?

Sure it did, see 55fb759ab3e7543a6be72a35e6b6961455c5b393.
That's why you don't see the complaints in 9.4 and up.
I'm not sure why Peter didn't back-patch it, but doing so now seems
safe enough.

> Any thoughts on fixing the other warnings?

I'm okay with small, low-risk patches to silence warnings in back
branches.  Like Robert, I'd be concerned about anything invasive.

			regards, tom lane

  13. Re: Compiler warnings

    Joe Conway <mail@joeconway.com> — 2017-01-02T22:53:31Z

    On 01/02/2017 10:55 AM, Joe Conway wrote:
> On the 9.2 and 9.3 branches I see two warnings:

> This one once:
> ---------------
> plancache.c:1197:9: warning: ‘plan’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
> 
> And this one once per bison file:
> ---------------
> gram.y:169.1-13: warning: deprecated directive, use ‘%name-prefix’
> [-Wdeprecated]
>  %name-prefix="base_yy"
>  ^^^^^^^^^^^^^

> Starting in 9.5 I only get the plancache.c warning and this one:
> ---------------
> lwlock.c:1555:5: warning: ‘mode’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
>   if (mode == LW_EXCLUSIVE)

Peter Eisentraut's Bison deprecation warnings patch (per Tom's reply
nearby in this thread) back-patched to 9.2 and 9.3 branches.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=55fb759ab3e7543a6be72a35e6b6961455c5b393

Stephen Frosts's plancache.c back-patched to 9.6 through 9.2
and lwlock.c back-patched 9.6 through 9.5:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=d97b14ddab2059e1d73c0cd17f26bac4ef13e682

> For the sake of completeness, in 9.4. I get the plancache.c warning and
> this one:
> ---------------
> basebackup.c:1284:6: warning: variable ‘wait_result’ set but not used
> [-Wunused-but-set-variable]
>   int   wait_result;

This one is no longer seen -- I must have neglected to pull before
making that comment.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development