From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Subject: Silencing upcoming warning about stack_base_ptr
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3773722.1645141403.0@sss.pgh.pa.us>
Date: Thu, 17 Feb 2022 18:44:27 -0500
Message-ID: <3773792.1645141467@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3773792.1645141467%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3773722.1645141403.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

GCC 12, coming soon to a distro near you, complains like this:

postgres.c: In function 'set_stack_base':
postgres.c:3430:24: warning: storing the address of local variable 'stack_=
base' in 'stack_base_ptr' [-Wdangling-pointer=3D]
 3430 |         stack_base_ptr =3D &stack_base;
      |         ~~~~~~~~~~~~~~~^~~~~~~~~~~~~
postgres.c:3419:25: note: 'stack_base' declared here
 3419 |         char            stack_base;
      |                         ^~~~~~~~~~
postgres.c:136:13: note: 'stack_base_ptr' declared here
  136 | char       *stack_base_ptr =3D NULL;
      |             ^~~~~~~~~~~~~~

(that's visible now on buildfarm member caiman).  We probably
should take some thought for silencing this before it starts
to be in people's faces during routine development.

Fixing this is a bit harder than one could wish because we export
set_stack_base() for use by PL/Java, so it would be better to not
change that API.  I ended up with the attached patch, which works
to silence the warning so long as the new subroutine
set_stack_base_from() is marked pg_noinline.  I'm a little worried
that in a year or two GCC will be smart enough to complain anyway.
If that happens, we could probably silence the warning again by
moving set_stack_base() to a different source file --- but at some
point we might have to give up and change its API, I suppose.

I also took this opportunity to re-static-ify the stack_base_ptr
variable itself, as PL/Java seems to have adopted set_stack_base
since 1.5.0.  That part perhaps should not be back-patched.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-stack-base-warning.patch";
	charset="us-ascii"
Content-ID: <3773722.1645141403.2@sss.pgh.pa.us>
Content-Description: fix-stack-base-warning.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/=
postmaster.c
index 735fed490b..a05d84c0f4 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -582,6 +582,7 @@ HANDLE		PostmasterHandle;
 void
 PostmasterMain(int argc, char *argv[])
 {
+	char		stack_reference =3D '\0';
 	int			opt;
 	int			status;
 	char	   *userDoption =3D NULL;
@@ -1083,7 +1084,7 @@ PostmasterMain(int argc, char *argv[])
 	/*
 	 * Set reference point for stack-depth checking.
 	 */
-	set_stack_base();
+	set_stack_base_from(&stack_reference);
 =

 	/*
 	 * Initialize pipe (or process handle on Windows) that allows children t=
o
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 38d8b97894..b228948045 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -129,17 +129,15 @@ static long max_stack_depth_bytes =3D 100 * 1024L;
 =

 /*
  * Stack base pointer -- initialized by PostmasterMain and inherited by
- * subprocesses. This is not static because old versions of PL/Java modif=
y
- * it directly. Newer versions use set_stack_base(), but we want to stay
- * binary-compatible for the time being.
+ * subprocesses (but see also InitPostmasterChild).
  */
-char	   *stack_base_ptr =3D NULL;
+static char *stack_base_ptr =3D NULL;
 =

 /*
  * On IA64 we also have to remember the register stack base.
  */
 #if defined(__ia64__) || defined(__ia64)
-char	   *register_stack_base_ptr =3D NULL;
+static char *register_stack_base_ptr =3D NULL;
 #endif
 =

 /*
@@ -3408,15 +3406,36 @@ ia64_get_bsp(void)
 #endif							/* IA64 */
 =

 =

+/*
+ * set_stack_base_from: set up reference point for stack depth checking
+ *
+ * The passed-in value must be the address of a local variable in the cal=
ler.
+ * Ideally the caller is the process's outermost function, but it's also
+ * acceptable for it to be not too many stack levels down from there.
+ */
+pg_noinline void
+set_stack_base_from(char *stack_reference)
+{
+	/* Set up reference point for stack depth checking */
+	stack_base_ptr =3D stack_reference;
+#if defined(__ia64__) || defined(__ia64)
+	register_stack_base_ptr =3D ia64_get_bsp();
+#endif
+}
+
 /*
  * set_stack_base: set up reference point for stack depth checking
  *
+ * This is exported for use by PL/Java.  At some point we might have to
+ * require it to take a stack_reference pointer to silence compiler warni=
ngs.
+ * For now, we don't, so don't break the API unnecessarily.
+ *
  * Returns the old reference point, if any.
  */
 pg_stack_base_t
 set_stack_base(void)
 {
-	char		stack_base;
+	char		stack_reference =3D '\0';
 	pg_stack_base_t old;
 =

 #if defined(__ia64__) || defined(__ia64)
@@ -3426,11 +3445,7 @@ set_stack_base(void)
 	old =3D stack_base_ptr;
 #endif
 =

-	/* Set up reference point for stack depth checking */
-	stack_base_ptr =3D &stack_base;
-#if defined(__ia64__) || defined(__ia64)
-	register_stack_base_ptr =3D ia64_get_bsp();
-#endif
+	set_stack_base_from(&stack_reference);
 =

 	return old;
 }
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/mi=
scinit.c
index 0868e5a24f..1cb15d54d4 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -94,6 +94,8 @@ bool		IgnoreSystemIndexes =3D false;
 void
 InitPostmasterChild(void)
 {
+	char		stack_reference =3D '\0';
+
 	IsUnderPostmaster =3D true;	/* we are a postmaster subprocess now */
 =

 	/*
@@ -106,13 +108,14 @@ InitPostmasterChild(void)
 #endif
 =

 	/*
-	 * Set reference point for stack-depth checking. We re-do that even in t=
he
-	 * !EXEC_BACKEND case, because there are some edge cases where processes
-	 * are started with an alternative stack (e.g. starting bgworkers when
-	 * running postgres using the rr debugger, as bgworkers are launched fro=
m
-	 * signal handlers).
+	 * Set reference point for stack-depth checking.  This might seem
+	 * redundant in !EXEC_BACKEND builds; but it's not because the postmaste=
r
+	 * launches its children from signal handlers, so we might be running on
+	 * an alternative stack.  To avoid the notational cruft of having
+	 * InitPostmasterChild callers pass a reference variable, we assume that
+	 * one declared here is close enough to the stack base.
 	 */
-	set_stack_base();
+	set_stack_base_from(&stack_reference);
 =

 	InitProcessGlobals();
 =

diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 0abc3ad540..24c17d5517 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -287,6 +287,7 @@ typedef struct
 typedef char *pg_stack_base_t;
 #endif
 =

+extern void set_stack_base_from(char *stack_reference);
 extern pg_stack_base_t set_stack_base(void);
 extern void restore_stack_base(pg_stack_base_t base);
 extern void check_stack_depth(void);

------- =_aaaaaaaaaa0--