Static Polymorphic Construction: A Flaw in Extensible Classes

[mgaudet]

While running UBSan across the compiler codebase, an error was pointed out to me that is a flaw in the way that we've implemented extensible classes.

The issue is the static-polymorphic equivalent to "When my base class’s constructor calls a virtual function on its this object, why doesn’t my derived class’s override of that virtual function get invoked?"

Specifically: If self() is invoked inside of a constructor, we have created a situation where we are calling a method on an un-constructed object.

We can see exactly this happening in the compiler codebase. For example,

https://github.com/eclipse/omr/blob/8595d1c3b98b623cf696f9e1cb8a8ae1c94e0925/compiler/codegen/OMRCodeGenerator.cpp#L203

is a call to self() inside the construction of OMR::CodeGenerator. This downcasts to the concrete type; however, the downcast is detected by UBSan to be invalid at the point in time it occurs:

../compiler/codegen/OMRCodeGenerator_inlines.hpp:30:11: runtime error: downcast of address 0x00010d006d60 which does not point to an object of type 'TR::CodeGenerator'
0x00010d006d60: note: object is of type 'OMR::CodeGenerator'
 01 00 00 00  d0 3d fb 05 01 00 00 00  f0 ad bf 5f ff 7f 00 00  d8 ab bf 5f ff 7f 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'OMR::CodeGenerator'

Fortuitiously, I suspect we've yet to be bitten in a bad way by this.

I think the correct answer here might be to

1. Alter the linter to emit warnings if an override exists and is called in a constructor, and
2. Remove the requiremernt to call through self() in constructors, allowing the natural (and safe) binding to occur.
3. Audit calls for errors. There is a tool in development called OMRStatistics by @samasri that may aid in detecting these errors.

[mstoodle]

This problem isn't specific to our extensible class mechanism, is it? It's just a dangerous corner of any kind of static polymorphism in C++ (e.g. the more well known CRTP form) ?

[mgaudet]

Correct.

[0xdaryl]

We had some discussions within IBM on possible solutions and workarounds to this pattern (and more broadly in the context of calling a virtual function of a class from within a constructor of that class as @mgaudet referenced above). @rwy0717, would you care to summarize those discussions here as a starting point so that the community has insight and input on the solution please?

[samasri]

As part of the Centre of Academic studies, a research group (myself, @snadi , @rwy0717 , @xliang6 , @nbhuiyan , @mgaudet ) is working on changing the variability implementation in OMR to use dynamic polymorphism. While applying that through PRs in order to test if there will be any degradation in the runtime performance, I started virtualizing functions in the CodeGenerator hierarchy. While virtualizing all functions in that hierarchy, we found out cases as mentioned by @mgaudet and would like to fix them to continue virtualizing.

Specifically, we got Java to crash when virtualizing these 2 functions due to the reason mentioned above:

• createLinkageForCompilation().
• createLinkage(TR_LinkageConventions).

More specifically, the constructor calls initialize(TR::Compilation*) which calls initializeLinkage() which calls both: createLinkage(TR_LinkageConventions) and createLinkageForCompilation().

We wanted to check if anyone here has a suggested solution to fix this in a hypothetical variability implementation (that we are applying) that uses dynamic polymorphism. The solution we have in mind so far is: initializeLinkage basically creates a linkage object and then set it as the object's body linkage object. The suggested solution is to create the linkage object outside the constructor and pass it to the constructor as a parameter so that it can set its own body linkage object to the one passed in the parameter.

Please let us know about any other solutions you might have in mind.