1612461 - clang-plugin must be built in c++14 mode to match clang

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(Away)]

There is a change in clang 10 that makes DenseMaps use a flexible allocator. This is relevant to our plugin situation because CFG objects contain DenseMaps.

When clang_plugin.dll calls buildCFG, clang.exe gives us a CFG whose DenseMap was allocated with whatever aligned-new settings clang.exe had, but when the CFG goes out of scope, clang_plugin.dll cleans it up, according to whatever aligned-new settings clang_plugin.dll has. If these don't match, we crash.

A translation unit's aligned-new setting depends on whether or not you're built in C++17 mode, so the conclusion is that the C++17-ness of both clang.exe and clang_plugin.dll need to match.

froydnj: I'm looking for a sanity check, does this make sense? I thought about maybe pushing against upstream for this "regression" in compatibility, but on second thought I'm thinking the burden should be on us. In general it seems like it's a bad idea to compile two different binaries against the same headers and not agree on settings.

Comment 1

[Mike Hommey [:glandium]]

If the C++ standard matters when building plugins, then llvm-config should be the one to tell about it. Not all versions of clang are using the same C++ standard version, and there's other way than llvm-config for the build system to know which one to use.

Comment 2

[(Away)]

Not all versions of clang are using the same C++ standard version,

Can you point me to examples?

and there's other way than llvm-config for the build system to know which one to use.

Is this sentence missing a "no" ? I'm having trouble understanding it otherwise.

Anyway, llvm-config --cxxflags doesn't say anything about standard on my machine. Does that mean assume c++14?

Comment 3

[Andi [:andi]]

I will answer to your last question, every clang version comes with a default standard that it supports, of course you can specify -std=c++17 or whatever but without it, it supports a default dialect according to it's version. Starting to clang-6 the default is gnu++-14 prior to that it was gnu++-98. In your case the default is also gnu++-14.
You can find the default change from gnu++-98 to gnu++-14 announcement here.
In order to use the c++17 as a standard we specify it here.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #3)

You can find the default change from gnu++-98 to gnu++-14 announcement here.

So if we compile our clang toolchain binaries with gnu++17, everything will just work out? I guess that causes problems for people who for whatever reason want to use their own clang?

Comment 5

[Andi [:andi]]

(In reply to Nathan Froyd [:froydnj] from comment #4)

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #3)

You can find the default change from gnu++-98 to gnu++-14 announcement here.

So if we compile our clang toolchain binaries with gnu++17, everything will just work out? I guess that causes problems for people who for whatever reason want to use their own clang?

This makes me wonder if we really want to support this, since we use gnu++17 to build our tree but for some reason we don't want this to be enforced on the static-analysis that checks for issues in the tree.

Comment 6

[Nathan Froyd [:froydnj]]

I would be OK with editing our compiler flags for host code to be C++14.

Comment 7

[(Away)]

So. It turns out that we do consult llvm-config for this information, and the root cause of this bug is that llvm-config doesn't include the standard flag on Windows. Filed https://bugs.llvm.org/show_bug.cgi?id=44840.

Comment 8

[(Away)]

Nope, my previous assessment was completely wrong, there was something strange on my machine that wasn't doing what I expected.

Here's the real reason: llvm-config does give us -std:c++14 on Windows, which makes our command lines look like:
-Xclang -std=c++17 ... -std:c++14
and I guess those end up going to two different places in clang, because it thinks c++17 is still the winner.

If we change our general flag to -std:c++17 then the override happens correctly.

Comment 9

[Andi [:andi]]

I just did a build-64/debug and clang-plugin is built with -std=c++17.

Comment 10

[(Away)]

Attachment: Translate LLVM_CXXFLAGS's `-std:` to `-std=` for clang-cl

clang-plugin.dll links against clang.exe so they both need to use the same C++ standard. This is achieved by having build/clang-plugin/moz.build use the flags from llvm-config. However, llvm-config uses the -std:c++14 format, so our flags end up looking like:
-Xclang -std=c++17 ... -std:c++14
and apparently the former wins out in clang's option plumbing, so the compiler still thinks we requested c++17.

By using the : form consistently, the override happens as desired.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/867db08a6fee
Translate LLVM_CXXFLAGS's `-std:` to `-std=` for clang-cl r=froydnj,glandium

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/867db08a6fee