Thread: [PATCH] Guard `CLOBBER_FREED_MEMORY` & `MEMORY_CONTEXT_CHECKING`
From 73dbe66c0ed68ae588223639a2ba93f6a727b704 Mon Sep 17 00:00:00 2001 From: Samuel Marks <807580+SamuelMarks@users.noreply.github.com> Date: Fri, 23 Aug 2024 16:03:41 -0500 Subject: [PATCH] [src/include/pg_config_manual.h] Guard `CLOBBER_FREED_MEMORY` & `MEMORY_CONTEXT_CHECKING` --- src/include/pg_config_manual.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h index e799c298..1f28d585 100644 --- a/src/include/pg_config_manual.h +++ b/src/include/pg_config_manual.h @@ -261,7 +261,7 @@ * facilitate catching bugs that refer to already-freed values. * Right now, this gets defined automatically if --enable-cassert. */ -#ifdef USE_ASSERT_CHECKING +#if defined(USE_ASSERT_CHECKING) && !defined(CLOBBER_FREED_MEMORY) #define CLOBBER_FREED_MEMORY #endif @@ -270,7 +270,8 @@ * bytes than were allocated). Right now, this gets defined * automatically if --enable-cassert or USE_VALGRIND. */ -#if defined(USE_ASSERT_CHECKING) || defined(USE_VALGRIND) +#if (defined(USE_ASSERT_CHECKING) || defined(USE_VALGRIND)) && \ + !defined(MEMORY_CONTEXT_CHECKING) #define MEMORY_CONTEXT_CHECKING #endif -- 2.43.0 Samuel Marks, PhD https://linkedin.com/in/samuelmarks
Samuel Marks <samuelmarks@gmail.com> writes:
> Subject: [PATCH] [src/include/pg_config_manual.h] Guard `CLOBBER_FREED_MEMORY`
> & `MEMORY_CONTEXT_CHECKING`
Why is this a good idea?
regards, tom lane
It will resolve the large number of these warnings from https://github.com/pgcentralfoundation/pgrx/blob/6dfb9d1/cargo-pgrx/src/command/init.rs#L411-L412: <command-line>: note: this is the location of the previous definition ./../../src/include/pg_config_manual.h:274: warning: "MEMORY_CONTEXT_CHECKING" redefined | #define MEMORY_CONTEXT_CHECKING | <command-line>: note: this is the location of the previous definition In file included from ../../../src/include/c.h:55, from ../../../src/include/postgres.h:46, from xactdesc.c:15: ../../../src/include/pg_config_manual.h:265: warning: "CLOBBER_FREED_MEMORY" redefined | #define CLOBBER_FREED_MEMORY and yes will be sending them a patch also. But there's no harm in not redefining symbols, so not sure why this is a controversial patch. On Fri, Aug 23, 2024 at 4:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Samuel Marks <samuelmarks@gmail.com> writes: > > Subject: [PATCH] [src/include/pg_config_manual.h] Guard `CLOBBER_FREED_MEMORY` > > & `MEMORY_CONTEXT_CHECKING` > > Why is this a good idea? > > regards, tom lane
Samuel Marks <samuelmarks@gmail.com> writes: > It will resolve the large number of these warnings from > https://github.com/pgcentralfoundation/pgrx/blob/6dfb9d1/cargo-pgrx/src/command/init.rs#L411-L412: Hmm, that seems like their problem not ours. It's not very clear to me why they'd want to force these flags from the compiler command line in the first place, but if they do they should be consistent with the more usual ways to set them. > and yes will be sending them a patch also. But there's no harm in not > redefining symbols, so not sure why this is a controversial patch. The reason I'm resistant to changing it is that the code you want to touch has been unchanged since 2003 in the first case, and 2013 in the second. It's fairly unclear what external code might have grown dependencies on the current behavior, but with that much history I'm not eager to bet that the answer is "none". Also, the present setup makes it clear that you are supposed to test "#ifdef CLOBBER_FREED_MEMORY" not "#if CLOBBER_FREED_MEMORY". If we stop locking down the expected contents of the macro, bugs of that sort could sneak in. regards, tom lane
Hmm… so you're worried about people setting the definition to 0 for disabled and nonzero || present for enabled? - I suppose all the `#define`s in PostgreSQL could be guarded and checked for both presence and either empty value or value. But maybe you just don't want to touch this side of the codebase? https://gcc.gnu.org/onlinedocs/cpp/If.html https://learn.microsoft.com/en-us/cpp/preprocessor/hash-if-hash-elif-hash-else-and-hash-endif-directives-c-cpp?view=msvc-170 (assuming similar on clang and other compilers; I'm sure this behaviour predated C89) It would be odd for people to depend on behaviour like not defining CLOBBER_FREED_MEMORY at the CPPFLAGS level. Anyway, I assume it's your call, so should I withdraw this PATCH then? - Or make a more comprehensive PATCH checking for definition and (emptiness or nonzero)? PS: I did send through a PR to that build-PostgreSQL-extensions-with-Rust project https://github.com/pgcentralfoundation/pgrx/pull/1826 Samuel Marks, PhD https://linkedin.com/in/samuelmarks On Fri, Aug 23, 2024 at 4:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Samuel Marks <samuelmarks@gmail.com> writes: > > It will resolve the large number of these warnings from > > https://github.com/pgcentralfoundation/pgrx/blob/6dfb9d1/cargo-pgrx/src/command/init.rs#L411-L412: > > Hmm, that seems like their problem not ours. It's not very clear > to me why they'd want to force these flags from the compiler > command line in the first place, but if they do they should be > consistent with the more usual ways to set them. > > > and yes will be sending them a patch also. But there's no harm in not > > redefining symbols, so not sure why this is a controversial patch. > > The reason I'm resistant to changing it is that the code you want > to touch has been unchanged since 2003 in the first case, and 2013 > in the second. It's fairly unclear what external code might have > grown dependencies on the current behavior, but with that much > history I'm not eager to bet that the answer is "none". Also, > the present setup makes it clear that you are supposed to test > "#ifdef CLOBBER_FREED_MEMORY" not "#if CLOBBER_FREED_MEMORY". > If we stop locking down the expected contents of the macro, bugs > of that sort could sneak in. > > regards, tom lane
> Hmm, that seems like their problem not ours. It's not very clear > to me why they'd want to force these flags from the compiler > command line in the first place, but if they do they should be > consistent with the more usual ways to set them. There is no particular need to merge this patch for our sakes, as the only reason it is this way was haste to build valgrind support. Clang often finds some issue with the Postgres code style, so I ignore its lint output, which is only regurgitated for errors anyway. At least, in most build configurations. Happy to take any patches for build stuff.