pg_bsd_indent compiles bytecode

On Tue, Jun 23, 2020 at 05:56:10PM -0400, Alvaro Herrera wrote:

I just noticed that when you compile pg_bsd_indent with a PG tree that
has --enable-jit (or something around that), then it compiles the source
files into bytecode.

Obviously this is not harmful since these files don't get installed, but
I wonder if our compiles aren't being excessively generous.

Are you saying pg_bsd_indent indents the JIT output files? I assumed
people only ran pg_bsd_indent on dist-clean trees.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

Bruce Momjian <bruce@momjian.us> writes:

On Tue, Jun 23, 2020 at 05:56:10PM -0400, Alvaro Herrera wrote:

I just noticed that when you compile pg_bsd_indent with a PG tree that
has --enable-jit (or something around that), then it compiles the source
files into bytecode.
Obviously this is not harmful since these files don't get installed, but
I wonder if our compiles aren't being excessively generous.

Are you saying pg_bsd_indent indents the JIT output files? I assumed
people only ran pg_bsd_indent on dist-clean trees.

I think what he means is that when pg_bsd_indent absorbs the CFLAGS
settings that PG uses (because it uses the pgxs build infrastructure),
it ends up also building .bc files.

I wouldn't care about this particularly for pg_bsd_indent itself,
but it suggests that we're probably building .bc files for client-side
files, which seems like a substantial waste of time. Maybe we need
different CFLAGS for client and server?

regards, tom lane

On Sat, Jun 27, 2020 at 05:12:57PM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Tue, Jun 23, 2020 at 05:56:10PM -0400, Alvaro Herrera wrote:

I just noticed that when you compile pg_bsd_indent with a PG tree that
has --enable-jit (or something around that), then it compiles the source
files into bytecode.
Obviously this is not harmful since these files don't get installed, but
I wonder if our compiles aren't being excessively generous.

Are you saying pg_bsd_indent indents the JIT output files? I assumed
people only ran pg_bsd_indent on dist-clean trees.

I think what he means is that when pg_bsd_indent absorbs the CFLAGS
settings that PG uses (because it uses the pgxs build infrastructure),
it ends up also building .bc files.

Wow, OK, I was confused then.

I wouldn't care about this particularly for pg_bsd_indent itself,
but it suggests that we're probably building .bc files for client-side
files, which seems like a substantial waste of time. Maybe we need
different CFLAGS for client and server?

Understood.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

Hi,

On 2020-06-27 17:12:57 -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Tue, Jun 23, 2020 at 05:56:10PM -0400, Alvaro Herrera wrote:

I just noticed that when you compile pg_bsd_indent with a PG tree that
has --enable-jit (or something around that), then it compiles the source
files into bytecode.
Obviously this is not harmful since these files don't get installed, but
I wonder if our compiles aren't being excessively generous.

Are you saying pg_bsd_indent indents the JIT output files? I assumed
people only ran pg_bsd_indent on dist-clean trees.

I think what he means is that when pg_bsd_indent absorbs the CFLAGS
settings that PG uses (because it uses the pgxs build infrastructure),
it ends up also building .bc files.

Hm. Yea, I think I see the problem. OBJS should only be expanded if
MODULE_big is set.

I wouldn't care about this particularly for pg_bsd_indent itself,
but it suggests that we're probably building .bc files for client-side
files, which seems like a substantial waste of time. Maybe we need
different CFLAGS for client and server?

I don't think it'll apply to most in-tree client side programs, so it
shouldn't be too bad currently. Still should fix it, of course.

I can test that with another program, but for some reason pg_bsd_indent
fails to build against 13/HEAD, but builds fine against 12. Not sure yet
what's up:

/usr/bin/ld.gold: error: indent.o: multiple definition of 'input'
/usr/bin/ld.gold: args.o: previous definition here
/usr/bin/ld.gold: error: indent.o: multiple definition of 'output'
/usr/bin/ld.gold: args.o: previous definition here
/usr/bin/ld.gold: error: indent.o: multiple definition of 'labbuf'
/usr/bin/ld.gold: args.o: previous definition here
...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

I can test that with another program, but for some reason pg_bsd_indent
fails to build against 13/HEAD, but builds fine against 12. Not sure yet
what's up:

Huh. Works here on RHEL8 ... what platform are you using?

regards, tom lane

Andres Freund <andres@anarazel.de> writes:

On 2020-06-27 17:12:57 -0400, Tom Lane wrote:

I wouldn't care about this particularly for pg_bsd_indent itself,
but it suggests that we're probably building .bc files for client-side
files, which seems like a substantial waste of time. Maybe we need
different CFLAGS for client and server?

I don't think it'll apply to most in-tree client side programs, so it
shouldn't be too bad currently. Still should fix it, of course.

Having now checked, there isn't any such problem. No .bc files are
getting built except in src/backend and in other modules that feed
into the backend, such as src/timezone and most of contrib.

I do see .bc files getting built for pg_bsd_indent, as Alvaro reported.
Seems like it must be a bug in the pgxs make logic, not anything more
generic.

regards, tom lane

On Sat, Jun 27, 2020 at 06:54:04PM -0400, Tom Lane wrote:

Having now checked, there isn't any such problem. No .bc files are
getting built except in src/backend and in other modules that feed
into the backend, such as src/timezone and most of contrib.

I do see .bc files getting built for pg_bsd_indent, as Alvaro reported.
Seems like it must be a bug in the pgxs make logic, not anything more
generic.

Yeah, and I think that it is caused by the following bit:
ifeq ($(with_llvm), yes)
all: $(addsuffix .bc, $(MODULES)) $(patsubst %.o,%.bc, $(OBJS))
endif

Shouldn't the latter part be ignored if PROGRAM is used?
--
Michael

Hi,

On 2020-06-27 18:43:40 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I can test that with another program, but for some reason pg_bsd_indent
fails to build against 13/HEAD, but builds fine against 12. Not sure yet
what's up:

Huh. Works here on RHEL8 ... what platform are you using?

That was on Debian unstable, but I don't think it's really related. The
issue turns out to be that gcc 10 changed the default from -fno-common
to -fcommon, and I had 13/HEAD set up to use gcc 10, but 12 to use gcc
9.

The way that pg_bsd_indent defines its variables isn't standard C, as
far as I can tell, which explains the errors I was getting. All the
individual files include indent_globs.h, which declares/defines a bunch
of variables. Since it doesn't use extern, they'll all end up as full
definitions in each .o when -fno-common is used (the default now), hence
the multiple definition errors. The only reason it works with -fcommon
is that they'll end up processed as weak symbols and 'deduplicated' at
link time.

Ick.

Greetings,

Andres Freund

Hi,

On 2020-06-27 18:54:04 -0400, Tom Lane wrote:

Having now checked, there isn't any such problem. No .bc files are
getting built except in src/backend and in other modules that feed
into the backend, such as src/timezone and most of contrib.

I do see .bc files getting built for pg_bsd_indent, as Alvaro reported.
Seems like it must be a bug in the pgxs make logic, not anything more
generic.

Yea. The issue is in pgxs.mk. So it does affect contrib/ modules that
use PROGRAM (e.g. contrib/pg_standby/pg_standby.bc is built), but not
other parts of the tree.

It's easy enough to fix by just adding a ifndef PROGRAM around the piece
adding the depency to the .bc files:

ifeq ($(with_llvm), yes)
ifndef PROGRAM
all: $(addsuffix .bc, $(MODULES)) $(patsubst %.o,%.bc, $(OBJS))
endif # PROGRAM
endif # with_llvm

but it's not particularly pretty. But given the way pgxs.mk is
structured, I'm not sure there's really a great answer?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

The way that pg_bsd_indent defines its variables isn't standard C, as
far as I can tell, which explains the errors I was getting. All the
individual files include indent_globs.h, which declares/defines a bunch
of variables. Since it doesn't use extern, they'll all end up as full
definitions in each .o when -fno-common is used (the default now), hence
the multiple definition errors. The only reason it works with -fcommon
is that they'll end up processed as weak symbols and 'deduplicated' at
link time.

Ugh. I agree that's pretty bogus, even if there's anything in the
C standard that allows it. I'll put it on my to-do list.

regards, tom lane

I wrote:

Andres Freund <andres@anarazel.de> writes:

The way that pg_bsd_indent defines its variables isn't standard C, as
far as I can tell, which explains the errors I was getting. All the
individual files include indent_globs.h, which declares/defines a bunch
of variables. Since it doesn't use extern, they'll all end up as full
definitions in each .o when -fno-common is used (the default now), hence
the multiple definition errors. The only reason it works with -fcommon
is that they'll end up processed as weak symbols and 'deduplicated' at
link time.

Ugh. I agree that's pretty bogus, even if there's anything in the
C standard that allows it. I'll put it on my to-do list.

I pushed the attached patch to the pg_bsd_indent repo. Perhaps Piotr
would like to absorb it into upstream.

I don't intend to mark pg_bsd_indent with a new release number for
this --- for people who successfully compiled, it's the same as before.

regards, tom lane

Andres Freund <andres@anarazel.de> writes:

It's easy enough to fix by just adding a ifndef PROGRAM around the piece
adding the depency to the .bc files:

ifeq ($(with_llvm), yes)
ifndef PROGRAM
all: $(addsuffix .bc, $(MODULES)) $(patsubst %.o,%.bc, $(OBJS))
endif # PROGRAM
endif # with_llvm

but it's not particularly pretty. But given the way pgxs.mk is
structured, I'm not sure there's really a great answer?

Yeah. The only other plausible alternative I see is like this:

ifeq ($(with_llvm), yes)
ifdef MODULES
all: $(addsuffix .bc, $(MODULES))
endif # MODULES
ifdef MODULE_big
all: $(patsubst %.o,%.bc, $(OBJS))
endif # MODULE_big
endif # with_llvm

which might be a little nicer because it squares better with,
eg, the install/uninstall rules. But it's not much better.

regards, tom lane

Hi,

On 2020-06-29 21:27:48 -0400, Tom Lane wrote:

I wrote:

Andres Freund <andres@anarazel.de> writes:

The way that pg_bsd_indent defines its variables isn't standard C, as
far as I can tell, which explains the errors I was getting. All the
individual files include indent_globs.h, which declares/defines a bunch
of variables. Since it doesn't use extern, they'll all end up as full
definitions in each .o when -fno-common is used (the default now), hence
the multiple definition errors. The only reason it works with -fcommon
is that they'll end up processed as weak symbols and 'deduplicated' at
link time.

Ugh. I agree that's pretty bogus, even if there's anything in the
C standard that allows it. I'll put it on my to-do list.

I pushed the attached patch to the pg_bsd_indent repo. Perhaps Piotr
would like to absorb it into upstream.

Thanks!

I don't intend to mark pg_bsd_indent with a new release number for
this --- for people who successfully compiled, it's the same as before.

Makes sense to me.

Greetings,

Andres Freund