[ https://issues.apache.org/jira/browse/THRIFT-5682?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jens Geyer resolved THRIFT-5682. -------------------------------- Fix Version/s: 0.21.0 Assignee: Lukas Barth Resolution: Fixed Thanks! > 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 > Assignee: Lukas Barth > Priority: Major > Labels: pull-request-available > Fix For: 0.21.0 > > Time Spent: 4h 10m > Remaining Estimate: 0h > > 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)