Trace plugin compatibility is broken

[Noremos]

My tracing plugin works fine with the FB 6.0 interface when running FB6, but it throws a segmentation fault when running FB5 when calling getExplainPlan(). I think the problem is in the new getPerfStats method (added in 62a2a15)
because it's in the middle of other functions that mess up the offsets (but I could be wrong).

Reproduction:

1. Put the Firebird 6 version of the libfbtrace.so/fbtrace.dll plugin to Firebird 5.
2. Setup trace:

Firebird.conf:

AuditTraceConfigFile = fbtrace.conf

fbtace.conf

database
{
 enabled = true
 print_plan = true
 explain_plan = true
}

3. Attach to any DB and execute a select SQL:

isql -user SYSDBA -p masterkey employee
select * from rdb$database;

[Noremos]

I can confirm that the problem is indeed due to incorrect inerface generation. The following patch fixes the issue.

diff --git a/src/include/firebird/FirebirdInterface.idl b/src/include/firebird/FirebirdInterface.idl
index 579d851c92..e227edc685 100644
--- a/src/include/firebird/FirebirdInterface.idl
+++ b/src/include/firebird/FirebirdInterface.idl
@@ -1383,8 +1383,6 @@ interface TraceStatement : Versioned
 	int64 getStmtID();
 	PerformanceInfo* getPerf();
 
-version: // 5.0 -> 6.0
-	PerformanceStats getPerfStats();
 }
 
 interface TraceSQLStatement : TraceStatement
@@ -1394,6 +1392,8 @@ interface TraceSQLStatement : TraceStatement
 	TraceParams getInputs();
 	const string getTextUTF8();
 	const string getExplainedPlan();
+version: // 5.0 -> 6.0
+	PerformanceStats getPerfStats();
 }
 
 interface TraceBLRStatement : TraceStatement
@@ -1401,6 +1401,8 @@ interface TraceBLRStatement : TraceStatement
 	const uchar* getData();
 	uint getDataLength();
 	const string getText();
+version: // 5.0 -> 6.0
+	PerformanceStats getPerfStats();
 }
 
 interface TraceDYNRequest : Versioned

I'll create a pull request with the proposed fix. I also think it would be better to disallow the
inheritance of interfaces with versioned methods

[aafemt]

It would be better to fix the generator to take into account versioning of parent objects...

[Noremos]

It would be better to fix the generator to take into account versioning of parent objects...

If I understand the interface correctly, I see the following situations:

interface A
{
  int64 A1();
  int64 A2();
}

interface B : A
{
   int64 B1();
}

interface C : A
{
  int64 C1();
version:
  int64 C2_v1();
}

Then we modify A:

interface A
{
  int64 A1();
  int64 A2();
version:
  int64 A3_v1();
}

In the current code, a new method will be added to the top.

B:
  int64 A1();
  int64 A2();
  int64 A3_v1();
  int64 B1();

C:
  int64 A1();
  int64 A2();
  int64 A3_v1();
  int64 C1();
  int64 C2_v1();

And that's wrong.
You could try adding a new base versioned method at the and:

B:
  int64 A1();
  int64 A2();
  int64 B1();
  int64 A3_v1();

C:
  int64 A1();
  int64 A2();
  int64 C1();
  int64 C2_v1();
  int64 A1_v1();

But adding a new version to C will break the offsets:

interface C : A
{
  int64 C1();
version:
  int64 C2_v1();
version:
  int64 C3_v2();
}

C:
  int64 A1();
  int64 A2();
  int64 C1();
  int64 C2_v1();
  int64 C3_v2();
  int64 A3_v1();

We can add the base version methods to the end of the base C methods and before the versioned methods

C:
  int64 A1();
  int64 A2();
  int64 C1();
  int64 A3_v1();
  int64 C2_v1();
  int64 C3_v2();

But this will again result in an incorrect offset when a new version method is added to the base code:

interface A
{
  int64 A1();
  int64 A2();
version:
  int64 A3_v1();
version:
  int64 A4_v2();
}

C:
  int64 A1();
  int64 A2();
  int64 C1();
  int64 A1_v1();
  int64 A2_v2();
  int64 C2_v1();
  int64 C3_v2();

So, I don't know how to solve the resolve the versions of the inherence.
And I think until a solution is found, the best I can do is help prevent similar problems by adding strict check in the cloop.

[hvlad]

Just a raw idea: inherited object should know separate "start index" in the common virtual methods table for every base class and for the itself. And impl code should use it, of course. In your case:

interface A
{
  int64 A1();
  int64 A2();
version:
  int64 A3_v1();
}

interface C : A
{
  int64 C1();
version:
  int64 C2_v1();
version:
  int64 C3_v2();
}

VMT for C:
  int64 A1();    <--- start of A, = 0
  int64 A2();
  int64 A3_v1();
  int64 C1();    <--- start of C, = start of A + size of A
  int64 C2_v1();
  int64 C3_v2();

It allows to not change layout of VMT but makes codegen a bit more complex.

[aafemt]

VMT for C:
  int64 A1();    <--- start of A, = 0
  int64 A2();
  int64 A3_v1();
  int64 C1();    <--- start of C, = start of A + size of A
  int64 C2_v1();
  int64 C3_v2();

And in the previous version of the interface size of A was smaller so C1 had smaller offset.

Indeed, the only solution is to prohibit modification of the base object completely.

[Noremos]

I had an idea to not merge functions into one table , but rather points to the base object:

B:
  A* baseVtable;
  int64 B1();

C:
  A* baseVtable;
  int64 C2_v1();

But this will break backward compatibility and create additional overhead.

[hvlad]

I had an idea to not merge functions into one table , but rather points to the base object:

B:
  A* baseVtable;
  int64 B1();

C:
  A* baseVtable;
  int64 C2_v1();

But this will break backward compatibility and create additional overhead.

This is why I speak about single VMT

[hvlad]

VMT for C:
  int64 A1();    <--- start of A, = 0
  int64 A2();
  int64 A3_v1();
  int64 C1();    <--- start of C, = start of A + size of A
  int64 C2_v1();
  int64 C3_v2();

And in the previous version of the interface size of A was smaller so C1 had smaller offset.

Then use not offsets but count of items.