Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix invokeMethod issues with Qt 6.5 #778

Merged
merged 1 commit into from
Apr 19, 2023

Conversation

vimpostor
Copy link
Member

@vimpostor vimpostor commented Apr 17, 2023

Since https://codereview.qt-project.org/c/qt/qtbase/+/422745 there is a new variadic version of QMetaObject::invokeMethod().

However this new variadic version is not compatible with our usage in Endpoint::invokeObjectLocal(), as we just dump all 10 parameters at once without caring about the actual function arity.
Theoretically this could be fixed by manually adding 10 if-branches for the correct arity.

However the second much bigger issue is that we don't know the type at compile time, and thus would end up hardcoding all parameter types to MethodArgument.

Hence to fix this, we revert back to the non-variadic version by explicitly calling QMetaObject::invokeMethod() with QGenericArgument as parameters.

Please see the commit message for more details.

Fixes #777

common/endpoint.cpp Outdated Show resolved Hide resolved
@Waqar144
Copy link
Contributor

The only better fix I can think of is to use the private invokeMethodImpl and pass in our arguments.

Since Qt 6.5 there is a new variadic version of the
QMetaObject::invokeMethod() method [0].
Generally this is a good idea, as this fixes the problem of being able
to pass a maximum number of only 10 arguments to invokeMethod().

However for usage in GammaRay this poses a problem:
When we call invokeObjectLocal() from within the inspected process, we
can no longer just dump all arguments, as the variadic invokeMethod()
version will then always attempt to call a version of the method with
exactly 10 arguments of type GammaRay::MethodArgument [1].

The fact that the argument number is fixed (and usually wrong) is not
the main problem here, as it could easily be worked around by adding 10
if branches that select the appropriate invokeMethod() with the correct
arity.

The more serious problem is that the argument type is always fixed to
GammaRay::MethodArgument [2].
We can't easily work around this as Q_ARG() wrappers and similar
shenanigans need the type at compile time, but we don't know the type at
compile time.

Luckily the non-variadic version of QMetaObject::invokeMethod still
exists, so as a workaround we force the usage of the non-variadic
version by explicitly passing the arguments as QGenericArgument.

Note that this version of QMetaObject::invokeMethod is obsolete since Qt
6.5 and might be removed with Qt 7.0, in which case we need to find a
better fix.

As for the actual implementation in this patch, note that we need to
store the MethodArgument first and can't just construct the
QGenericArgument directly.
Alternatively we could wrap every parameter in the function call to
invokeMethod() with QGenericArgument(), but using a second array is a
little bit cleaner code-wise.

Also convert the QVector to a C-style array, as we always allocate and
need all 10 elements anyway.

Fixes KDAB#777
Fixes KDAB#761

[0] https://codereview.qt-project.org/c/qt/qtbase/+/422745
[1] see KDAB#777 (comment)
[2] e.g.: QMetaObject::invokeMethod: No such method GammaRay::ToolManagerClient::availableToolsResponse(GammaRay::MethodArgument)
	Candidates are: availableToolsResponse(QList<GammaRay::ToolData>)
@vimpostor
Copy link
Member Author

vimpostor commented Apr 19, 2023

The only better fix I can think of is to use the private invokeMethodImpl and pass in our arguments.

Not sure if it's really better to use private API here. With this current patch we have at least the guarantee that it will continue working until at least Qt 7.0. If it changes then, invokeMethodImpl will probably break too and we have to revisit this anyway.

@Waqar144
Copy link
Contributor

The only better fix I can think of is to use the private invokeMethodImpl and pass in our arguments.

Not sure if it's really better to use private API here. With this current patch we have at least the guarantee that it will continue working until at least Qt 7.0. If it changes then, we probably have to revisit this anyway.

For now yes, as the public API essentially does that for us internally

@Waqar144 Waqar144 merged commit ac80241 into KDAB:master Apr 19, 2023
@vimpostor
Copy link
Member Author

vimpostor commented Apr 20, 2023

Another thing to keep in mind (although this is unrelated to the change in this PR) is that for some strange reason with Qt 6.5 the signal QuickInspectorClient::features() fails to fire with the following error message during runtime:

QMetaObject::invokeMethod: No such method GammaRay::QuickInspectorClient::features(QFlags<GammaRay::QuickInspectorInterface::Feature>)
Candidates are:
    features(GammaRay::QuickInspectorInterface::Features)

Not sure if this is a bug in Qt, because GammaRay::QuickInspectorInterface::Features is a literal typedef for QFlags<GammaRay::QuickInspectorInterface::Feature> so it should work.

As a workaround the following diff fixes the problem in GammaRay:

diff --git a/plugins/quickinspector/quickinspectorinterface.h b/plugins/quickinspector/quickinspectorinterface.h
index a1d19474b..c98af8438 100644
--- a/plugins/quickinspector/quickinspectorinterface.h
+++ b/plugins/quickinspector/quickinspectorinterface.h
@@ -86,7 +86,7 @@ public slots:
     virtual void setSlowMode(bool slow) = 0;
 
 signals:
-    void features(GammaRay::QuickInspectorInterface::Features features);
+    void features(QFlags<GammaRay::QuickInspectorInterface::Feature> features);
     void serverSideDecorationChanged(bool enabled);
     void overlaySettings(const GammaRay::QuickDecorationsSettings &settings);
     void slowModeChanged(bool slow);

But perhaps this is better fixed in Qt.

@Waqar144
Copy link
Contributor

But perhaps this is better fixed in Qt.

I think this is https://codereview.qt-project.org/c/qt/qtbase/+/399035

I have already put some work arounds for our property view to make this work.

Perhaps we can register the alias metatype and it will work?

@vimpostor
Copy link
Member Author

vimpostor commented Apr 20, 2023

Perhaps we can register the alias metatype and it will work?

There already is a Q_DECLARE_METATYPE(GammaRay::QuickInspectorInterface::Features) and a StreamOperators::registerOperators<Features>();.
When I try to additionally register Q_DECLARE_METATYPE(QFlags<GammaRay::QuickInspectorInterface::Feature>), I get an duplicate metatype id error and if I replace it instead of adding, then it still doesn't work.

It also doesn't work if I qRegisterMetaType<QFlags<GammaRay::QuickInspectorInterface::Feature>>() .

Any other ideas?

@Waqar144
Copy link
Contributor

Waqar144 commented Apr 20, 2023

To register an alias, I think it should be
qRegisterMetaType<QFlags<GammaRay::QuickInspectorInterface::Feature>>("alias")

@vimpostor
Copy link
Member Author

To register an alias, I think it should be qRegisterMetaType<QFlags<GammaRay::QuickInspectorInterface::Feature>>("alias")

That unfortunately doesn't work either. :(

@vimpostor
Copy link
Member Author

It looks like this problem is not just with QFlags, but with all kinds of typedefs and using declarations, e.g. using ObjectIds = QVector<class ObjectId>; is affected as well, which causes the Quick scene object picker to not work anymore:

QMetaObject::invokeMethod: No such method GammaRay::RemoteViewClient::elementsAtReceived(QList<GammaRay::ObjectId>,int)
Candidates are:
    elementsAtReceived(GammaRay::ObjectIds,int)

// ^ above type mismatch is literally the `using` declaration from above

Perhaps we can send some more type info, so that we can construct the type for invokeMethod more correctly at the receiver side, but I am not sure if it's possible.

@Waqar144
Copy link
Contributor

A simpler and likely more robust solution for the above is to just use the real type. Aliases are just sugar anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GammaRay hangs at splash screen with Qt 6.5.0
2 participants