Re: Mark all GUC variable as PGDLLIMPORT

Hi,

On 2022-02-15 08:58:05 -0500, Robert Haas wrote:
> On Mon, Feb 14, 2022 at 8:53 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > An alternative rule which would dodge that particular issue would be
> > > to just slap PGDLLIMPORT on every global variable in every header
> > > file. That would arguably be a simpler rule, though it means even more
> > > PGDLLIMPORT declarations floating around.
> >
> > That would have the advantage of being comparatively easy to check in an
> > automated way. Might even be cheap enough to just make it part of the build.
>
> I wasn't able to quickly write something that was smart enough to use
> as part of the build, but I wrote something dumb that I think works
> well enough to use with a little bit of human intelligence alongside.
> See attached.

Cool.

> > But it seems like it'd be a fair amount of work and cause a lot of patch
> > rebasing pain? If we end up going that way, we should schedule this to happen
> > just after the feature freeze, I think.
>
> We could do that. I'd sort of rather get it done. We still have two
> weeks before the last CommitFest officially starts, and it's not as if
> there won't be tons of uncommitted patches floating around after that.

Breaking close to every patch 6-7 weeks before freeze doesn't seem
great. Particularly because this is a mostly automatically generated patch, so
I don't really see a forcing function to do this now, rather than at a more
opportune moment.

The more I think about it the more I'm convinced that if we want to do this,
we should do it for variables and functions.

> > If we consider doing this for all extern variables, we should think about
> > doing this for headers *and* functions. That'd allow us to get rid of the
> > fairly complicated logic to generate the .def file for the postgres binary on
> > windows (src/tools/gendef.pl). And maybe also the related thing on AIX
> > (src/backend/port/aix/mkldexport.sh)
>
> I don't know what you mean by this.

We only need gendef.pl because we don't mark functions with visibility
information. If we attach PGDLLIMPORT to functions and variables we can a fair
bit of complexity. And I think not just for windows, but also AIX (there's a
whole section in src/backend/Makefile about AIX oddity), but I'm not sure
about it.

> diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
> index a4b5dc853b..13849a3790 100644
> --- a/src/include/common/relpath.h
> +++ b/src/include/common/relpath.h
> @@ -56,7 +56,7 @@ typedef enum ForkNumber
>
> #define FORKNAMECHARS 4 /* max chars for a fork name */
>
> -extern const char *const forkNames[];
> +extern PGDLLIMPORT const char *const forkNames[];
>
> extern ForkNumber forkname_to_number(const char *forkName);
> extern int forkname_chars(const char *str, ForkNumber *fork);

I think we might need a new form of PGDLLIMPORT to mark these correctly - I
don't think they should be marked PGDLLIMPORT in frontend environment.

> diff --git a/src/include/fe_utils/print.h b/src/include/fe_utils/print.h
> index 836b4e29a8..5caf9e2d08 100644
> --- a/src/include/fe_utils/print.h
> +++ b/src/include/fe_utils/print.h
> @@ -177,10 +177,10 @@ typedef struct printQueryOpt
> } printQueryOpt;
>
>
> -extern volatile sig_atomic_t cancel_pressed;
> +extern PGDLLIMPORT volatile sig_atomic_t cancel_pressed;
>
> -extern const printTextFormat pg_asciiformat;
> -extern const printTextFormat pg_asciiformat_old;
> +extern PGDLLIMPORT const printTextFormat pg_asciiformat;
> +extern PGDLLIMPORT const printTextFormat pg_asciiformat_old;
> extern printTextFormat pg_utf8format; /* ideally would be const, but... */

Are these right? I don't think we should make frontend stuff exported without
a very clear reason.

> extern void jit_reset_after_error(void);
> diff --git a/src/include/jit/llvmjit.h b/src/include/jit/llvmjit.h
> index 66143afccc..4541f9a2c4 100644
> --- a/src/include/jit/llvmjit.h
> +++ b/src/include/jit/llvmjit.h
> @@ -56,30 +56,30 @@ typedef struct LLVMJitContext
> } LLVMJitContext;
>
> /* llvm module containing information about types */
> -extern LLVMModuleRef llvm_types_module;
> +extern PGDLLIMPORT LLVMModuleRef llvm_types_module;

These are wrong I think, this is a shared library.

Greetings,

Andres Freund