Closed Bug 1612461 Opened 4 years ago Closed 4 years ago

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

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

defect
Not set
normal

Tracking

(firefox75 fixed)

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: away, Assigned: away)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.

Flags: needinfo?(nfroyd)

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.

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?

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.

(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?

Flags: needinfo?(nfroyd)

(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.

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

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.

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

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.

Assignee: nobody → dmajor
Status: NEW → ASSIGNED
Attachment #9125537 - Attachment description: Use `-std:c++17` instead of `-Xclang -std=c++17` with clang-cl → Translate LLVM_CXXFLAGS's `-std:` to `-std=` for clang-cl
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/867db08a6fee
Translate LLVM_CXXFLAGS's `-std:` to `-std=` for clang-cl r=froydnj,glandium
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Blocks: clang-10
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: