GEOS 3.13 and GEOSMessageHandler

[Paul Ramsey]

> On Aug 22, 2024, at 5:33 AM, Sandro Santilli <strk at kbt.io> wrote:
> 
> I've added these attributes when fixing a segfault that was really
> caused by a misused printf format string, so those warnings ARE useful.
> 
> What was again the error you got by redefining it, for which  you
> provided the __STDC_VERSION__ version check ?

```
In file included from rt_band.c:37:
In file included from ./librtcore_internal.h:35:
In file included from ../../liblwgeom/lwgeom_geos.h:31:
/usr/local/include/geos_c.h:105:16: warning: redefinition of typedef 'GEOSMessageHandler' is a C11 feature [-Wtypedef-redefinition]
typedef void (*GEOSMessageHandler)(const char *fmt, ...);

../../liblwgeom/lwgeom_geos.h:28:16: note: previous definition is here
typedef void (*GEOSMessageHandler)(const char *fmt, ...) __attribute__ (( format(printf, 1, 0) ));
```

A fairly reasonable warning, IMO, since redefining something we are pulling in from another library is quite a bit of a hack, and dangerous in its own right.


> May I suggest to file a ticket to deal with the problem ?
> CI seems happy about the redefinition, right ?

My own dev system is not happy about the redefinition, and yeah, I think it’s philosophically the wrong answer.

Feel free to start a ticket, I just want to come to a conclusion.

ATB,

P


> 
> --strk;
> 
> On Wed, Aug 21, 2024 at 03:02:16PM -0700, Paul Ramsey wrote:
>> OK, so the lwgeom_geos.h header currently has this abomination in it
>> 
>> ```
>> #if POSTGIS_GEOS_VERSION < 31300
>> #if __STDC_VERSION__ >= 201112L
>> /* See https://github.com/libgeos/geos/pull/1097 */
>> /* Only redefine the typedef if compiler is C11 or higher */
>> typedef void (*GEOSMessageHandler)(const char *fmt, ...) __attribute__ (( format(printf, 1, 0) ));
>> #endif
>> #endif
>> ```
>> 
>> The bit in the middle is us overriding the GEOS typedef for message handler, not for any purpose but to quiet a warning. I’m not sure when we started getting it, or when GEOS started getting it, but it resulted in GEOS’s definition of the message handler including some __attribute__ information. That cleans up warnings for anything using GEOS >= 3.13.
>> 
>> But for older GEOS, there’s still a warning where we *use* the message handler, not just where we define it. So we’re getting warnings in the PostGIS build too. Again, I don’t know why we have this warning now. But I’d say, having read up on what we get from adding the __attribute__ information (a theoretical improvement in avoiding making mistakes using format strings?) quieting the warnings in our code seems like the wrong answer. I’d say we should just ignore the warnings by stripping the check, at least for versions of GEOS (<3.13) that we know have the old, non-attributed message handler typedef.
>> 
>> So I propose adding this to the PostGIS configure.ac
>> 
>> ```
>> dnl Turn off attribute warnings for GEOS < 3.13
>> if test "$POSTGIS_GEOS_VERSION" -lt "31300"; then
>> 	_LT_COMPILER_OPTION([if $compiler supports -Wno-suggest-attribute=format], [_cv_nosuggestattribute], [-Wno-suggest-attribute=format], [], [CFLAGS="$CFLAGS -Wno-suggest-attribute=format"], [])
>> fi
>> ```
>> Just tell the compiler to shut up about this warning, except for places where we have some control over it, namely with GEOS >= 3.13
>> 
>> P