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

Add a way to name objects for validation messages #34

Closed
karl-lunarg opened this issue May 14, 2018 · 22 comments
Closed

Add a way to name objects for validation messages #34

karl-lunarg opened this issue May 14, 2018 · 22 comments
Labels
Enhancement New feature or request
Milestone

Comments

@karl-lunarg
Copy link
Contributor

Issue by Jasper-Bekkers (MIGRATED)
Friday Jun 16, 2017 at 11:42 GMT
Originally opened as KhronosGroup/Vulkan-LoaderAndValidationLayers#1880


Hi,

Right now often you'll get validation errors like this Cannot submit cmd buffer using image (0x9a) [sub-resource: aspectMask 0x1 array layer 0, mip level 0], with layout .... In our engine we have a policy of naming all objects we create for debugging purposes so it would be extremely useful to pass these along to the validation layers for easier debugging so we can get a message like Cannot submit cmd buffer using image (LinearRenderTarget1) [sub-resource: aspectMask 0x1 array layer 0, mip level 0], with layout ... instead.

Can we make validation layers respect VK_EXT_debug_marker or have the validation layers expose some other kind of extension to provide names to it?

@karl-lunarg karl-lunarg added this to the P1 milestone May 14, 2018
@karl-lunarg karl-lunarg added the Enhancement New feature or request label May 14, 2018
@karl-lunarg
Copy link
Contributor Author

Comment by Tony-LunarG (MIGRATED)
Thursday Jun 29, 2017 at 19:04 GMT


Addressed by 1f53907

@karl-lunarg
Copy link
Contributor Author

Comment by Jasper-Bekkers (MIGRATED)
Thursday Jun 29, 2017 at 20:04 GMT


Does this also fix up all validation warnings to print the named object
instead of the id?

On Thu, Jun 29, 2017, 21:04 Tony-LunarG [email protected] wrote:

Addressed by 1f53907
KhronosGroup/Vulkan-LoaderAndValidationLayers@1f53907


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
KhronosGroup/Vulkan-LoaderAndValidationLayers#1880 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADBuuxn8WNlqiw1d-wT9SfaWBVtrtelks5sI_VDgaJpZM4N8W6c
.

@karl-lunarg
Copy link
Contributor Author

Comment by Tony-LunarG (MIGRATED)
Thursday Jun 29, 2017 at 20:14 GMT


It does to an extent. Anything logged through debug_report_log_message will prepend a string "SrcObject name = ". Other objects referenced in the message will still only have their handles printed.

@karl-lunarg
Copy link
Contributor Author

Comment by Jasper-Bekkers (MIGRATED)
Thursday Jun 29, 2017 at 20:30 GMT


Cool, that's already a pretty nice improvement over what was done before. Though it was something that you could easily do app side as well by just keeping the name somewhere and matching it against the object passed back to you in the debug callback.

However the other objects referenced in the message are also very important (case in point: the message that started this big report), so I would like to include the other referenced objects within the scope of this issue as well.

@karl-lunarg
Copy link
Contributor Author

Comment by mark-lunarg (MIGRATED)
Monday Jul 03, 2017 at 15:11 GMT


@Jasper-Bekkers, this family of capabilities is meant to be addressed through a new debug report extension which is currently under development. It will allow the output of multiple objects through the main interface (among many other things). This change is what we can reasonably do in the short-term. Thanks for prodding us to get this in!

@karl-lunarg
Copy link
Contributor Author

Comment by Jasper-Bekkers (MIGRATED)
Wednesday Aug 09, 2017 at 11:00 GMT


@mark-lunarg what's the status of that new extension?

@karl-lunarg
Copy link
Contributor Author

Comment by mark-lunarg (MIGRATED)
Wednesday Aug 09, 2017 at 15:20 GMT


Still winding it's way through Khronos towards ratification. Hey @Jasper-Bekkers, if you compile a summary of the messages you'd like to see reworked, we can probably get that done sooner rather than later. Even after the new extension becomes available work will still need to be done to expand the message output and have neither the bandwidth nor inclination to reformat every validation layer error message. Point out the ones which'll help you and we'll work to get this issue closed.

@karl-lunarg
Copy link
Contributor Author

Comment by Jasper-Bekkers (MIGRATED)
Wednesday Aug 09, 2017 at 15:45 GMT


I just did a quick few greps through the code base and it looks like PRIxLEAST64 might be the best indication of what to replace (there are some false positives when printing sizes / alignments instead of object IDs). This yields about ~160 results, which TBH it a lot less then I expected.

I'm fine with this issue staying open until all (or most) of these are fixed. If at some point there is an easy replacement for HandleToUint64 that returns a string (either debug name or object ID) we can probably work together and do the menial task of fixing up the messages.

@karl-lunarg
Copy link
Contributor Author

Comment by gwihlidal (MIGRATED)
Friday Aug 25, 2017 at 09:09 GMT


+1 - this would be great to have!

@karl-lunarg
Copy link
Contributor Author

Comment by chaoticbob (MIGRATED)
Thursday Sep 07, 2017 at 15:17 GMT


+1 comes up pretty frequently in discussions

@karl-lunarg
Copy link
Contributor Author

Comment by Jasper-Bekkers (MIGRATED)
Monday Nov 27, 2017 at 15:43 GMT


Since this seems to still be an issue every day for pretty much anybody using the validation layer, could we get a status update on this? It's been a few months and all that happened is that seemingly this issue got down-promoted even though there is quite a bit of demand for this.

@karl-lunarg
Copy link
Contributor Author

Comment by mark-lunarg (MIGRATED)
Monday Nov 27, 2017 at 16:27 GMT


@Jasper-Bekkers, the extension has been approved and will probably wind up in the release after 1.0.66. However, using the new extension will require modifying the current log messages. If you can point to some favorites we can move these to the top of the list, as I don't think we'll implement blanket coverage, at least to begin with.

@karl-lunarg
Copy link
Contributor Author

Comment by Jasper-Bekkers (MIGRATED)
Friday Apr 20, 2018 at 09:34 GMT


It's been a while and it looks like VK_EXT_debug_utils (which I'm assuming is the right extension) has been out for a while too. What's the status of the error messages that need conversion?

@karl-lunarg
Copy link
Contributor Author

Comment by mark-lunarg (MIGRATED)
Friday Apr 20, 2018 at 14:02 GMT


@Jasper-Bekkers, we're going to need help with identifying the error messages that need conversion. It is impractical (at least at this time) to convert all of the log_msg calls -- but we will certainly work to cover the ones where it'd provide the most benefit.

@karl-lunarg
Copy link
Contributor Author

Comment by Jasper-Bekkers (MIGRATED)
Friday Apr 20, 2018 at 14:14 GMT


Have any been covered at all? Like I mentioned before, one could easily go over the ~160 or so instances of PRIxLEAST64 and patch them by hand. This is - other then vague / unclear messages - probably the number one productivity killer in the Vulkan ecosystem so it's sad to see such a response.

@karl-lunarg
Copy link
Contributor Author

Comment by mark-lunarg (MIGRATED)
Friday Apr 20, 2018 at 14:34 GMT


@Jasper-Bekkers, I apologize -- my response was relating to outputting multiple objects to the debug callback. We have already begun an effort to revamp log_msg() which would automatically provide the labeling functionality along with many other improvements, see PR #2475. It is coming!

@mark-lunarg mark-lunarg self-assigned this May 14, 2018
@jzulauf-lunarg
Copy link
Contributor

Triage notes: Looking for HandleToUint64, PRIxLEAST64 and PRIx64 replacing these with something like HandleToMessageString(handle).c_str() and %s for all handles reported in the text of log messages would be the approach. OTOF 100-200 replacements.

@aejsmith
Copy link
Contributor

aejsmith commented Jan 3, 2019

I'd really like to see this done mainly for image layout errors as per the original report - display the debug name of the image rather than just a handle. It's difficult to trace back to which image is affected for the errors that are reported at submit time rather than during the command that's at fault, having the debug name there would make the error a lot more useful.

@karl-lunarg
Copy link
Contributor Author

@aejsmith Are you using vkSetDebugUtilsObjectNameEXT to set the debug names?

@karl-lunarg
Copy link
Contributor Author

Additional triage/design notes:

The validation layer developers had intended for the log_msg() function to accept a list of objects, instead of the single object it currently accepts. This list of objects is printed out after the "main" message string. These objects are printed with their handle, type, and name (if provided via vkSetDebugUtilsObjectNameEXT).

Since this interface supports passing only one object for this list, you'll only ever see one object displayed. I think the intent was to go ahead and have the main message remain as-is, where only the handles are embedded in the message. But then there would be the "decoder" list of objects after the main message where the reader would go to find the additional object info. However, the current interface supports only one object.

So one approach (JohnZ's) would be to replace something like:

log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_IMAGE_EXT, HandleToUint64(image),
    "VUID-VkRenderPassBeginInfo-framebuffer-parameter",
    "Render Pass begin with renderpass 0x%" PRIx64 " uses framebuffer 0x%" PRIx64 " where pAttachments[%" PRIu32
    "] = image view 0x%" PRIx64 ", which refers to an invalid image",
    HandleToUint64(renderpass), HandleToUint64(framebuffer), attachment_index, HandleToUint64(image_view));

with

log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_IMAGE_EXT, HandleToUint64(image),
    "VUID-VkRenderPassBeginInfo-framebuffer-parameter",
    "Render Pass begin with renderpass %s uses framebuffer %s where pAttachments[%" PRIu32
    "] = image view %s, which refers to an invalid image",
    HandleToMessageString(renderpass).c_str(), HandleToMessageString(framebuffer).c_str(), attachment_index, HandleToMessageString(image_view).c_str());

where HandleToMessageString() would return "0x12345678" if there was no Object Name and "0x12345678 (MyObject)" if there was a name.

The other approach would be to replace the 4th argument (HandleToUint64(image)) with an array of objects [image, renderpass, framebuffer, image_view ] and otherwise leave the first form of the call to log_msg() alone.

Both approaches involve a lot of manual editing of a large number of log_msg() calls.

@aejsmith
Copy link
Contributor

aejsmith commented Jan 3, 2019

@karl-lunarg No, I'm currently still using the older vkDebugMarkerSetObjectNameEXT.

@mark-lunarg
Copy link
Contributor

This has been subsumed by tracking issue #679.

@shannon-lunarg shannon-lunarg removed this from the P1 milestone Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants