Thread

[PATCH] Guard `CLOBBER_FREED_MEMORY` & `MEMORY_CONTEXT_CHECKING`

Samuel Marks <samuelmarks@gmail.com> — 2024-08-23T21:09:22Z

Re: [PATCH] Guard `CLOBBER_FREED_MEMORY` & `MEMORY_CONTEXT_CHECKING`

Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-23T21:11:30Z

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

Re: [PATCH] Guard `CLOBBER_FREED_MEMORY` & `MEMORY_CONTEXT_CHECKING`

Samuel Marks <samuelmarks@gmail.com> — 2024-08-23T21:25:54Z

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

Re: [PATCH] Guard `CLOBBER_FREED_MEMORY` & `MEMORY_CONTEXT_CHECKING`

Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-23T21:39:40Z

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

Re: [PATCH] Guard `CLOBBER_FREED_MEMORY` & `MEMORY_CONTEXT_CHECKING`

Samuel Marks <samuelmarks@gmail.com> — 2024-08-23T22:15:21Z

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

Re: [PATCH] Guard `CLOBBER_FREED_MEMORY` & `MEMORY_CONTEXT_CHECKING`

Jubilee Young <workingjubilee@gmail.com> — 2024-08-24T04:55:43Z

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