Mixing CC and a different CLANG seems like a bad idea

On 2021-11-18 17:56, Tom Lane wrote:

I noticed that, a week after Michael pushed 9ff47ea41 to silence
-Wcompound-token-split-by-macro warnings, buildfarm member sidewinder
is still spewing them. Investigation shows that it's building with

configure: using compiler=cc (nb4 20200810) 7.5.0
configure: using CLANG=ccache clang

Hm, actually it's:

CC => "ccache cc",
CXX => "ccache c++",
CLANG => "ccache clang",

want me to change it to:

CC => "ccache clang",
CXX => "ccache c++",
CLANG => "ccache clang",

?

/Mikael

=?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= <mikael.kjellstrom@mksoft.nu> writes:

Hm, actually it's:

CC => "ccache cc",
CXX => "ccache c++",
CLANG => "ccache clang",

Right.

want me to change it to:

CC => "ccache clang",
CXX => "ccache c++",
CLANG => "ccache clang",

What I actually think is we should get rid of the separate CLANG
variable. But don't do anything to the animal's configuration
till that's settled.

BTW, that would presumably lead to wanting to use CXX = "ccache clang++",
too. I think we don't really want inconsistent CC/CXX either ...
although at least configure knows it needs to probe CXX's flags
separately. I suppose another way to resolve this would be for
configure to make a third set of probes to see what switches CLANG
has --- but ick.

regards, tom lane

Hi,

On 2021-11-18 11:56:59 -0500, Tom Lane wrote:

I noticed that, a week after Michael pushed 9ff47ea41 to silence
-Wcompound-token-split-by-macro warnings, buildfarm member sidewinder
is still spewing them. Investigation shows that it's building with

configure: using compiler=cc (nb4 20200810) 7.5.0
configure: using CLANG=ccache clang

and the system cc doesn't know -Wcompound-token-split-by-macro,
so we don't use it, but the modules that are built into bytecode
still produce the warnings because they're built with clang.

I think this idea of using clang with switches selected for some other
compiler is completely horrid, and needs to be nuked from orbit before
it causes problems worse than mere warnings.

We can test separately for flags, see BITCODE_CFLAGS/BITCODE_CXXFLAGS.

Why did we not simply insist that if you want to use --with-llvm, the
selected compiler must be clang? I cannot see any benefit of mix-and-match
here.

It seemed like a problematic restriction at the time. And still does to
me.

For one, gcc does generate somewhat better code. For another, extensions could
still compile with a different compiler, and generate bitcode with another
compiler, so it's good to have that easily testable.

It also just seems architecturally wrong: People pressed for making the choice
of JIT runtime replaceable, and it now is, at some pain. And forcing the main
compiler seems problematic from that angle. With the meson port jit
compilation actually kinda works on windows - but it seems we shouldn't force
people to not use visual studio there, just for that?

I think the issue is more with trying to be miserly in the choice of compiler
flag tests to duplicate and how many places to change to choose the right flag
variable. It's taken a while for this to become a real issue, so it perhaps
was the right choice at the time.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2021-11-18 11:56:59 -0500, Tom Lane wrote:

Why did we not simply insist that if you want to use --with-llvm, the
selected compiler must be clang? I cannot see any benefit of mix-and-match
here.

It also just seems architecturally wrong: People pressed for making the choice
of JIT runtime replaceable, and it now is, at some pain. And forcing the main
compiler seems problematic from that angle.

OK, I concede that's a reasonable concern. So we need to look more
carefully at how the switches for CLANG are being selected.

I think the issue is more with trying to be miserly in the choice of compiler
flag tests to duplicate and how many places to change to choose the right flag
variable. It's taken a while for this to become a real issue, so it perhaps
was the right choice at the time.

Yeah. I'm inclined to think we ought to just bite the bullet and fold
CLANG/CLANGXX into the main list of compiler switch probes, so that we
check every interesting one four times. That sounds fairly horrid,
but as long as you are using an accache file it's not really going
to cost that much. (BTW, does meson have any comparable optimization?
If it doesn't, I bet that is going to be a problem.)

regards, tom lane

I wrote:

Yeah. I'm inclined to think we ought to just bite the bullet and fold
CLANG/CLANGXX into the main list of compiler switch probes, so that we
check every interesting one four times.

After studying configure's list more closely, that doesn't seem like
a great plan either. There's a lot of idiosyncrasy in the tests,
such as things that only apply to C or to C++.

More, I think (though this ought to be documented in a comment) that
the policy is to not bother turning on extra -W options in the bitcode
switches, on the grounds that warning once in the main build is enough.
I follow that idea --- but what we missed is that we still need to
turn *off* the warnings we're actively disabling. I shall go do that,
if no objections.

regards, tom lane

Hi,

On 2021-11-18 12:43:15 -0500, Tom Lane wrote:

Yeah. I'm inclined to think we ought to just bite the bullet and fold
CLANG/CLANGXX into the main list of compiler switch probes, so that we
check every interesting one four times. That sounds fairly horrid,
but as long as you are using an accache file it's not really going
to cost that much. (BTW, does meson have any comparable optimization?
If it doesn't, I bet that is going to be a problem.)

regards, tom lane

Greetings,

Andres Freund

On Thu, Nov 18, 2021 at 3:43 PM Andres Freund <andres@anarazel.de> wrote:

Hi,

Greetings,

Andres Freund

Greetings to you too, Andres. :-)

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2021-11-18 13:39:04 -0500, Tom Lane wrote:

After studying configure's list more closely, that doesn't seem like
a great plan either. There's a lot of idiosyncrasy in the tests,
such as things that only apply to C or to C++.

Yea. It seems doable, but not really worth it for now.

More, I think (though this ought to be documented in a comment) that
the policy is to not bother turning on extra -W options in the bitcode
switches, on the grounds that warning once in the main build is enough.
I follow that idea --- but what we missed is that we still need to
turn *off* the warnings we're actively disabling. I shall go do that,
if no objections.

Thanks for doing that, that does sounds like a good way, at least for now.

On 2021-11-18 13:39:04 -0500, Tom Lane wrote:

That sounds fairly horrid, but as long as you are using an accache file it's
not really going to cost that much.

(BTW, does meson have any comparable optimization?
If it doesn't, I bet that is going to be a problem.)

Yes - imo in a nicer, more reliable way. It caches most test results, but with
the complete input, including the commandline (i.e. compiler flags) as the
cache key. So no more errors about compile flags changing...

Greetings,

Andres Freund

Hi,

On 2021-11-18 16:13:50 -0500, Robert Haas wrote:

On Thu, Nov 18, 2021 at 3:43 PM Andres Freund <andres@anarazel.de> wrote:

Hi,

Greetings,

Andres Freund

Greetings to you too, Andres. :-)

Oops I sent the email that I copied text from, rather than the one I wanted to
send...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2021-11-18 13:39:04 -0500, Tom Lane wrote:

More, I think (though this ought to be documented in a comment) that
the policy is to not bother turning on extra -W options in the bitcode
switches, on the grounds that warning once in the main build is enough.
I follow that idea --- but what we missed is that we still need to
turn *off* the warnings we're actively disabling. I shall go do that,
if no objections.

Thanks for doing that, that does sounds like a good way, at least for now.

Cool, thanks for confirming.

For the archives' sake: I thought originally that this was triggered
by having CC different from CLANG, and even wrote that in the commit
message; but I was mistaken. I was misled by the fact that sidewinder
is the only animal still reporting the compound-token-split-by-macro
warnings, and jumped to the conclusion that its unusual configuration
was the cause. But actually that't not it, because the flags we
feed to CLANG are *not* dependent on what CC will take. I now think
the actual uniqueness is that sidewinder is the only animal that is
using clang >= 12 and has --with-llvm enabled.

(BTW, does meson have any comparable optimization?
If it doesn't, I bet that is going to be a problem.)

Yes - imo in a nicer, more reliable way. It caches most test results, but with
the complete input, including the commandline (i.e. compiler flags) as the
cache key. So no more errors about compile flags changing...

Nice!

regards, tom lane