Обсуждение: Compiler warnings
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
Вложения
* 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
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
Вложения
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
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
Вложения
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
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
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
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
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
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
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
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