Обсуждение: Mixing CC and a different CLANG seems like a bad idea
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. 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.
regards, tom lane
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