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

Export only core symbols #98

Open
wants to merge 3 commits into
base: master
from

Conversation

@rpavlik
Copy link
Contributor

commented Aug 2, 2019

This adjusts the loader, on all platforms, to only export core functions, which is what I think we say in the docs we do. (aside from static linking, possibly only on windows, not sure)

This did require some changes to hello_xr, but I think only in accordance with the spec.

Not sure if we wanted to mark some of the graphics binding functions as exported too.

The other thing I did was "fix" the XRAPI_ATTR on Windows: it had gotten a declspec(dllexport) at some point, which I didn't find anywhere in Vulkan and I think was a mis-use of that attribute. We already had LOADER_EXPORT, it was just being inconsistently used and configured.

Have only tried this on Linux.

Fixes #45

@rpavlik rpavlik requested review from daveh-lunarg and yl-msft Aug 2, 2019

@rpavlik rpavlik added the in loader label Aug 2, 2019

@brycehutchings

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Why not export everything like it was? This sounds like a doc bug to me :-). The app is supposed to embed the loader with it, so exporting makes everything simpler. I could see why Vulkan didn't export extensions because each driver on the system might have different extensions and then you'd get linker errors, but that's avoided with OpenXR's. I really think we should keep exporting everything.

@rpavlik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

That's a good point...

Do we maybe need to bring this to the group? Since there's at least some examples that already use get proc addr for their extension code, don't remember if there's anything in the actual spec itself that says this.

If we do go that route, we can just modify these changes since we still probably don't want to export essentially loader-internal symbols 😀

Tagging in @Wallbraker and @ChristophHaag for Linux perspective

@brycehutchings

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Agreed we should definitely not export internal stuff

@brycehutchings

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

I thought I had heard that the plan on Linux was to include the loader on the system rather than embed with the application. If so, that seems pretty concerning to me because once the various platforms diverge then decisions become a lot harder because there might not be a clear correct decision. I hope I misunderstood and Linux OpenXR apps include the loader like on the other platforms like Windows and Android.

@rpavlik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

I thought that Android had a system wide loader too. It has to do with distributions - on Linux we do actually have a reasonable, centralized control so people don't step on each others libraries (distro package managers, etc). That, combined with the very rocky state of the loader on Linux early-on led us to switch the default to dynamic linking, to be able to fix breakages after shipping. This could be re-discussed, but that's probably a different issue than this one.

@rpavlik rpavlik referenced this pull request Aug 2, 2019
@daveh-lunarg

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Not sure I follow - what are the other things to export, that aren't loader internal and aren't API?

We can't export any extension symbols, so I can't think of what else there is? Also, it appears to me that these changes to hello_xr are the correct way to find/call an extension entry point.

@yl-msft

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

"LOADER_EXPORT" seems exposing too many functions, see image below
Following function shouldn't be exported for openxr_loader.dll on windows platform, IMHO.
image

@yl-msft
Copy link
Contributor

left a comment

The key of exporting functions on win32 should be avoid exporting functions that's not OpenXR APIs. Those currently decorated with XRAPI_ATTR are working fine today. However, some internal loader function with "LOADER_EXPORT" decoration should be avoided being exported when compiling for Win32.

@yl-msft

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

BTW: I also agree with Bryce that we should exporting all XR functions in openxr headers, including all extensions that's in the spec. Application shouldn't need to go through extra code to consume extensions if they are using the latest loader from this repo.

@daveh-lunarg

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

This is what I don't understand. Extensions are always optional, and at loader compile time there's no way to know which extensions will be available at run time (regardless whether KHR, EXT, or vendor extension). I don't see how the loader can export any extension-defined symbol.

Agreed the LoaderXr* functions have to go.

@yl-msft

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

The loader code is generated from the same xr.xml that generated the corresponding openxr.h. Therefore at loader compile time, it will support the same set of extensions in the openxr.h They are always in sync.

It's by design loader exposes "optional" extensions. If the runtime indicate any extension is not supported, it is invalid for app to call into those unsupported functions.

@daveh-lunarg

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

IMO it's risky to expose anything not guaranteed to exist on the platform. Vulkan's loader exposes only core API and a constrained set of guaranteed-present extensions (e.g. WSI stuff in the Win DLL). Even that's a frequent problem, because people forget to enable the extension they need to use and the linker has no idea. Having entry points in the loader that work perfectly for developer A using runtime A, but later then crash with runtime B because he forget to check, is inviting pain.

It's a very crisp and logical division that you can call core APIs directly, but for any extension (which you already have to query for and enable) you also have to GetInstanceProcAddr for it's entry points. There's little to gain by skipping the GIPA for (some) extensions, and a lot of potential downside (IMO).

@yl-msft

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

OpenXR loader is designed differently than Vulkan. The loader is not global on the system. It is included with each application. Therefore, the loader will not expose anything more than the corresponding openxr.h header already exposed, and vise versa. As long as the app is compiled with openxr header, they are taken care of by the loader.

Also an OpenXR conformed runtime must not enable any extension unless it's explicitly enabled by the application in xrCreateInstance. As long as the application is picking the extension name from the OpenXR.h, the loader must support this extension correctly.

Well if application is using an extension name outside of the OpenXR.h header, the loader cannot help them and they have to use GetInstanceProcAddr themselves and by passing loader logics. But at least the behavior is consistent.

@rpavlik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Right now, in both branches (this and #100) the loader checks to make sure you've enabled an extension before passing the call to extension-created functions thru to the runtime. We've discussed removing this check from the loader, as it seems like "too much validation" in the loader vs. the validation layer, but it may be useful if we go with this more drastic change.

@rpavlik rpavlik changed the title Change exporting Export only core symbols Aug 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.