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

Do not use applicaiton's alloc in layer #27

Closed
JacobHeAMD opened this issue Feb 14, 2020 · 8 comments
Closed

Do not use applicaiton's alloc in layer #27

JacobHeAMD opened this issue Feb 14, 2020 · 8 comments

Comments

@JacobHeAMD
Copy link
Contributor

JacobHeAMD commented Feb 14, 2020

There are several CTS test cases to fail the alloc intensionally. Using application's alloc in layer will cause some problems besides checking allocation result in layers, take timeline_CreateDevice as example,
fpCreateDevice returns success, that means the icd device is added to logic device list of loader, following is the callstack,

#0  loader_add_logical_device (inst=0x53e0b30, icd_term=0x5371f70, dev=0x5bfc9b0) at /jacob/Vulkan-Loader-latest/loader/loader.c:2112
#1  0x00007ffff4f429d1 in terminator_CreateDevice (physicalDevice=0x51095d0, pCreateInfo=0x7fffffffc780, pAllocator=0x7fffffffccc8,
    pDevice=0x7fffffffc6c8) at /jacob/Vulkan-Loader-latest/loader/loader.c:6639
#2  0x00007fffed3a3d67 in timeline_CreateDevice (physicalDevice=0x51095d0, pCreateInfo=0x7fffffffc780, pAllocator=0x7fffffffccc8,
    pDevice=0x7fffffffc6c8) at /source/opensource/Vulkan-ExtensionLayer-Jacob/layers/timeline_semaphore.c:2139 

However, if device_new is failed, fpDestroyDevice can't remove the icd device from loader's logic device list because loader's destroy device function can't be gotten by fpGetInstanceProcAddr(NULL, "vkDestroyDevice"),that is to say, there is no way to get the loader's destroy device function pointer. As a result, invalid icd device is stored in loader and double free issue happens later.

This issue can easily be reproduced with dEQP-VK.api.object_management.alloc_callback_fail.device

@djdeath
Copy link
Member

djdeath commented Feb 14, 2020

That does sound like a loader issue.

There is a number of reason this kind of issue could happen in the layer (we could fail the allocation of a fence/semaphore implementing a timeline semaphore), so if the loader can't deal with that it sounds we're having an issue to implement creating internal objects to the layer.

@JacobHeAMD
Copy link
Contributor Author

Yes. I searched on Vulkan-ValidationLayers and https://github.com/baldurk/sample_layer.git , didn't find any layer is using the application's allocation.

@JacobHeAMD
Copy link
Contributor Author

I'm not sure if it is by design?

@djdeath
Copy link
Member

djdeath commented Feb 15, 2020

I'll investigate and maybe raise an issue with the loader.

The issue seems to be broader than just the allocation functions.

Let's say in timeline_CreateDevice you first create the device then create another object from the implementation like a command buffer. What are our options if the command buffer creation fails?

I think we would be in the same situation.

Maybe we're missing a handshake somewhere with the loader...

@JacobHeAMD
Copy link
Contributor Author

Ok, please try with CTS 1.2 (./deqp-vk -n dEQP-VK.api.object_management.alloc_callback_fail.device), a double free issue is raised.

@JacobHeAMD
Copy link
Contributor Author

Hi @djdeath , do you have time to look into this issue? Once this is resolved, timeline_sempahore layer is supposed to pass CTS 1.2 now.

@djdeath
Copy link
Member

djdeath commented Mar 1, 2020

Sorry for the late answer, I just filed KhronosGroup/Vulkan-Loader#339

@juan-lunarg
Copy link
Contributor

closing since this appears to have been fixed by the loader.

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

No branches or pull requests

3 participants