Lukas Barth created THRIFT-5682:
-----------------------------------
Summary: UB in generated C++ code, stops compiling with C++20
Key: THRIFT-5682
URL: https://issues.apache.org/jira/browse/THRIFT-5682
Project: Thrift
Issue Type: Bug
Components: C++ - Compiler
Affects Versions: 0.17.0
Reporter: Lukas Barth
The C++ compiler of Thrift 0.17.0 produces C++ code with undefined behavior,
which stops compiling on Clang 15 in C++20 mode.
For a generated class, both the default constructor and {{operator==}}
implementations reference certain member functions of the class' members. As an
example, the default constructor references (i.e., "uses") the default
constructors of its members.
If a class contains a {{{}std::vector<Foo>{}}}, and {{Foo}} has only been
_forward_ declared (which happens often in Thrift-generated code), this creates
undefined behavior: The {{std::vector}} specification states that as long as
{{Foo}} is an incomplete type, it is fine to reference
{{{}std::vector<Foo>{}}}, but not any members (such as its default constructor).
Thus, we must defer our default constructor's implementation (which references
the default constructor of std::vector) to a point where Foo is a complete
type. That is the case in the .cpp file.
The same holds for operator==.Example
{{Take this very simple Thrift file:}}
{{struct StructA {}}
{{ 1:required list<StructB> myBs}}
{{{}}{}}}{{{}{}}}
{{struct StructB}}
{{{}}
{{ 1:required string someString}}
{{}}}
{{If I compile this using thrift --gen cpp:no_skeleton -o out ./file.thrift I
get a header file that contains the following:}}
{{class StructA;}}
{{{}class StructB;{}}}{{{}{}}}{{{}{}}}{{{}{}}}
{{class StructA : … }}
{{{}}
{{public:}}
{{ …}}
{{ StructA() noexcept {}}}
{{ …}}
{{ std::vector<StructB> myBs;}}
{{ …}}
{{};}}
{{…}}
{{class StructB : …}}
{{{ … };}}
{{In this case, the default constructor for StructA references the default
constructor of std::vector<StructB> while StructB is still an incomplete type.
*This is undefined behavior.* It did apparently compile with all big compilers
until recently, but with C++20, Clang 15 stops accepting this kind of
construct, as you can see here at CompilerExplorer:
[https://godbolt.org/z/xcc4av6cb]}}
{{ }}
h2. {{Proposed Solution}}
{{I propose to move the definitions of the default constructor and operator==
from the foobar_types.h into the foobar_types.cpp file. That way, when the
definition is seen (and therefore the methods of std::vector<Foo> referenced),
Foo already is a complete type and everything works out.}}
{{The only alternative Solution (and which only works if Thrift does not allow
circular dependencies - does it?) that I can see would be to compute a DAG of
dependencies between the structures and then output them in topological order.
That would not be a minor change.}}
I guess that the reason for the definitions being in the header file is that
this allows better inlining of these two methods. Nowadays, most
compilers/linkers perform link-time optimization, which allows for inlining to
happen at link time, so my feeling is that moving the definitions into their
own translation unit inside the CPP file should not cause significant
performance loss.
I have created a Pull Request for my proposed solution here:
https://github.com/apache/thrift/pull/2755
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
