GraphicsSpy Layer Fixes for Q and Beyond #2611
Conversation
Queries everything via the native system properties API instead. This does, however, remove the querying of the serial number, which is OK, since we always override it on the host side anyways. The only place where it is now missing is the trace file, but it's not really useful there anyways. Also, the way we looked for the serial number via JNI is also deprecated and returns "unknown" since O.
Renames `libVkLayerGraphicsSpy.so` to `libVkLayer_GraphicsSpy.so`, since that is the format the virtual swapchain layer uses as well.
This is to be more consistent with the library file name and the virtual swap chain layer. Also, "Vk" in "VkLayer_VkGraphicsSpy" is redundant.
We only need it and build it for Android.
The layer enumeration functions are called by the loader. The Android loader calls them to figure out what layer is in a library and can thus call them, well, anytime, even when we are not tracing. This hasn't been a problem, since if libgapii isn't loaded, we just pretend like we are not a layer. This will change with Q however, so this fixes it, so we always look like a layer, but don't need libgapii until we actually are loaded as a layer.
| CHECK(build.get_field("MODEL", model)); | ||
| CHECK(build.get_field("HARDWARE", gContext.mHardware)); | ||
| CHECK(build.get_field("DISPLAY", gContext.mOSBuild)); | ||
| GET_STRING_LIST_PROP("ro.product.cpu.abilist", gContext.mSupportedABIs); |
There was a problem hiding this comment.
Be sure to test this on older versions of Android and with 3rd party devices. IIRC, there was something preventing me from using system properties to get this info, hence the JNI.
There was a problem hiding this comment.
According to https://developer.android.com/about/versions/nougat/android-7.0-changes __system_property_get is the function to call. The warning there is simply that there is no standard specifying the names and values of the existing properties. However, the properties I'm querying here are the same as the ones queries by android.os.Build. BTW, the native code behind android.os.Build also uses the __system_property_* functions.
There was a problem hiding this comment.
I don't think the issue was with the property names, more that the function was simply not available on all versions of Android.
To be honest, I'm slightly surprised that this is accessible at all, as I thought there was an effort to classify this API as private (non-NDK-public). These release notes appear to suggest otherwise though.
There was a problem hiding this comment.
I think you are probably referring to this: https://stackoverflow.com/questions/28413530/api-to-get-android-system-properties-is-removed-in-arm64-platforms
As far as I can make out, it may at one point have been the idea to remove them, but that idea seems to have been overturned. There is a lot of Android code that depends on those functions to be present and accessible to apps. With those release notes, they are now guaranteed at the API level we build for.
There was a problem hiding this comment.
from the 7.0 docs, I would agree with Pascal that these are now officially supported only because of the wording:
Use __system_property_get instead of the private property_get
Which implies that __system_property_get is public. So I think this should be fine.
| #define GET_PROP(name, trans) \ | ||
| do { \ | ||
| char _v[PROP_VALUE_MAX] = {0}; \ | ||
| if (__system_property_get(name, _v) == 0) { \ |
There was a problem hiding this comment.
IIRC this is typically a method we shouldn't get calling?
There was a problem hiding this comment.
I just say ben's comment below, this is basically the same.
|
Other than the question about __getproperty, looks ok to me. |
Prepares us for a JDWP free tracing in Q and beyond.