Content-Type: multipart/mixed; boundary="------------isDcY7jixIZ5oKvErutpSyAN"
Message-ID: <2eda015b-7dff-47fd-d5e2-f1a9899b90a6@postgrespro.ru>
Date: Fri, 7 Jul 2023 15:13:14 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Content-Language: en-US
From: Sergey Shinderuk <s.shinderuk@postgrespro.ru>
Subject: gcc -Wclobbered in PostgresMain
To: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/2eda015b-7dff-47fd-d5e2-f1a9899b90a6%40postgrespro.ru>
Precedence: bulk

This is a multi-part message in MIME format.
--------------isDcY7jixIZ5oKvErutpSyAN
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi, hackers!

While analyzing -Wclobbered warnings from gcc we found a true one in 
PostgresMain():

postgres.c: In function ‘PostgresMain’:
postgres.c:4118:25: warning: variable 
‘idle_in_transaction_timeout_enabled’ might be clobbered by ‘longjmp’ or 
‘vfork’ [-Wclobbered]
  4118 |         bool            idle_in_transaction_timeout_enabled = 
false;
       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
postgres.c:4119:25: warning: variable ‘idle_session_timeout_enabled’ 
might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
  4119 |         bool            idle_session_timeout_enabled = false;
       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

These variables must be declared volatile, because they are read after 
longjmp(). send_ready_for_query declared there is volatile.

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. But strictly speaking the code is 
invoking undefined behavior by reading those variables after longjmp(), 
so it's worth fixing. And for consistency with send_ready_for_query too. 
I believe, making them volatile doesn't affect performance in any way.

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().

Best regards,

-- 
Sergey Shinderuk		https://postgrespro.com/
--------------isDcY7jixIZ5oKvErutpSyAN
Content-Type: text/x-patch; charset=UTF-8;
 name="missing-volatile-longjmp.diff"
Content-Disposition: attachment; filename="missing-volatile-longjmp.diff"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3Rjb3AvcG9zdGdyZXMuYyBiL3NyYy9iYWNrZW5k
L3Rjb3AvcG9zdGdyZXMuYwppbmRleCAwMWI2Y2MxZjdkMy4uNTZkOGIwODE0YjUgMTAwNjQ0
Ci0tLSBhL3NyYy9iYWNrZW5kL3Rjb3AvcG9zdGdyZXMuYworKysgYi9zcmMvYmFja2VuZC90
Y29wL3Bvc3RncmVzLmMKQEAgLTQxMTEsMTIgKzQxMTEsMTEgQEAgUG9zdGdyZXNTaW5nbGVV
c2VyTWFpbihpbnQgYXJnYywgY2hhciAqYXJndltdLAogdm9pZAogUG9zdGdyZXNNYWluKGNv
bnN0IGNoYXIgKmRibmFtZSwgY29uc3QgY2hhciAqdXNlcm5hbWUpCiB7Ci0JaW50CQkJZmly
c3RjaGFyOwogCVN0cmluZ0luZm9EYXRhIGlucHV0X21lc3NhZ2U7CiAJc2lnam1wX2J1Zgls
b2NhbF9zaWdqbXBfYnVmOwogCXZvbGF0aWxlIGJvb2wgc2VuZF9yZWFkeV9mb3JfcXVlcnkg
PSB0cnVlOwotCWJvb2wJCWlkbGVfaW5fdHJhbnNhY3Rpb25fdGltZW91dF9lbmFibGVkID0g
ZmFsc2U7Ci0JYm9vbAkJaWRsZV9zZXNzaW9uX3RpbWVvdXRfZW5hYmxlZCA9IGZhbHNlOwor
CXZvbGF0aWxlIGJvb2wgaWRsZV9pbl90cmFuc2FjdGlvbl90aW1lb3V0X2VuYWJsZWQgPSBm
YWxzZTsKKwl2b2xhdGlsZSBib29sIGlkbGVfc2Vzc2lvbl90aW1lb3V0X2VuYWJsZWQgPSBm
YWxzZTsKIAogCUFzc2VydChkYm5hbWUgIT0gTlVMTCk7CiAJQXNzZXJ0KHVzZXJuYW1lICE9
IE5VTEwpOwpAQCAtNDQxOCw2ICs0NDE3LDggQEAgUG9zdGdyZXNNYWluKGNvbnN0IGNoYXIg
KmRibmFtZSwgY29uc3QgY2hhciAqdXNlcm5hbWUpCiAKIAlmb3IgKDs7KQogCXsKKwkJaW50
CQkJZmlyc3RjaGFyOworCiAJCS8qCiAJCSAqIEF0IHRvcCBvZiBsb29wLCByZXNldCBleHRl
bmRlZC1xdWVyeS1tZXNzYWdlIGZsYWcsIHNvIHRoYXQgYW55CiAJCSAqIGVycm9ycyBlbmNv
dW50ZXJlZCBpbiAiaWRsZSIgc3RhdGUgZG9uJ3QgcHJvdm9rZSBza2lwLgo=

--------------isDcY7jixIZ5oKvErutpSyAN--