Opt into unstable libcxx ABI and add a custom namespace.

[chinmaygarde]

We use a statically linked libcxx and don't expose any of the internal symbols. This theoretically allows us to namespace everything to avoid accidentally using the wrong version libcxx. Also, not having to worry about ABI stability allows libcxx to opt into optimized routines.

But, since libcxx uses CMake and we don't, the mechanism to generate the __config_site file doesn't exist in GN. Instead, we check in a file that would have been generated for us by CMake. The file we check in though is a default with additional configuration. This version of the file makes it so that we opt into the unstable ABI (and get optimizations) and also namespace everything for Flutter so collisions are immediately flagged.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@text-exemption-reviewers" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[chinmaygarde]

Noticed this while answering this thread and seeing _LIBCPP_ABI_NAMESPACE unexpanded in the symbol name. The configuration options change all the time. These options were created from the version of libcxx in our DEPS using guidance mentioned in the README.

[chinmaygarde]

This should also make the output of llvm-cxxfilt less verbose because the macro _LIBCPP_ABI_NAMESPACE will not be expanded.

I don't know how we decided the default __config_site was fine but git blame says I checked it in. So 🤷🏽

[chinmaygarde]

FWIW, the optimizations this opts into are here. Though, the primary reason for the change is the namespacing and avoiding collisions. I don't expect this to move the needle on benchmarks.

[zanderso]

Are the ASAN and test failures related, or does this need a rebase past a bad commit?

Is there any way to test this? Is there some way to make the build fail if these changes are accidentally backed out?

[jason-simmons]

The flutter_linux_unittests tests are failing because the Linux embedder is placing std::function instances inside GTK GObjects.

Creating a GObject apparently initializes the object with zeros and does not run the C++ constructors for the class members. It looks like the old implementation of libcxx std::function could work with this, but the new one does not.

As a hack workaround I was able to get these tests to run by using placement new to force construction of the std::function instances in the GObject initializers. I'm not familiar with GTK and do not know what the preferred solution is for this issue.

[cbracken]

/cc @robert-ancell for GTK-side expertise. Any advice on recommended approaches here? Does what @jason-simmons is proposing sound reasonable?

[chinmaygarde]

From what @jason-simmons said, it seems like we were depending undefined behavior in the ABI instead mixing libcxxs (which is what I was primarily concerned about). There are number optimizations that are enabled by when the ABI can be unstable. But we can check this by setting _LIBCPP_ABI_VERSION to 1 and seeing if the tests pass. But we should fix the undefined behavior still though.

[chinmaygarde]

Trying a canary run with version set to 1.

[chinmaygarde]

That did it. @jason-simmons If you had to manually perform a placement new, that also means that the delete was not happening right? So we are leaking std::function guts because the destructors are not being called? We got away with the ctors not being called earlier because presumably the zero initialization was a find ctor.

[chinmaygarde]

FWIW, this version of the patch fixes the namespacing without changing the ABI. Should I land just this with a followup?

[jason-simmons]

The change to ABI version 1 LGTM

Filed flutter/flutter#140183

[robert-ancell]

If the Linux tests still have issues I think we should just change the use of std::function to function pointers. GTK is a C library so the mixing of C++ code can be somewhat problematic I expect. My C++ knowledge is not extensive but forcing the construction as proposed by @jason-simmons seems like it would be necessary if we want to continue to use C++ objects inside GObject.