Re: gcc -Wclobbered in PostgresMain
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Sergey Shinderuk <s.shinderuk@postgrespro.ru>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2023-07-08T15:11:22Z
Lists: pgsql-hackers
Attachments
- v2-fix-PostgresMain-local-vars.patch (text/x-diff) patch v2
Sergey Shinderuk <s.shinderuk@postgrespro.ru> writes: > While analyzing -Wclobbered warnings from gcc we found a true one in > PostgresMain(): > ... > These variables must be declared volatile, because they are read after > longjmp(). send_ready_for_query declared there is volatile. Yeah, you're on to something there. > Without volatile, these variables are kept in registers and restored by > longjump(). I think, this is harmless because the error handling code > calls disable_all_timeouts() anyway. Hmm. So what could happen (if these *aren't* in registers) is that we might later uselessly call disable_timeout to get rid of timeouts that are long gone anyway. While that's not terribly expensive, it's not great either. What we ought to be doing is resetting these two flags after the disable_all_timeouts call. Having done that, it wouldn't really be necessary to mark these as volatile. I kept that marking anyway for consistency with send_ready_for_query, but perhaps we shouldn't? > I also moved firstchar's declaration inside the loop where it's used, to > make it clear that this variable needn't be volatile and is not > preserved after longjmp(). Good idea, but then why not the same for input_message? It's fully reinitialized each time through the loop, too. In short, something like the attached, except I'm not totally sold on changing the volatility of the timeout flags. regards, tom lane
Commits
-
Be more rigorous about local variables in PostgresMain().
- a8b7424684b3 17.0 landed
- 93dcdfa88f5f 16.0 landed
- 6b51fe8340bb 13.12 landed
- 671bf1cf27cf 11.21 landed
- 48582cf9ed96 14.9 landed
- 22447db17cc0 15.4 landed
- 0c32818d7559 12.16 landed