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

clarify lifetime of objects passed to Debug Utils label functions #152

Open
3b opened this issue Apr 17, 2023 · 4 comments
Open

clarify lifetime of objects passed to Debug Utils label functions #152

3b opened this issue Apr 17, 2023 · 4 comments
Labels
clarification Reflects a spec clarification request synced to gitlab A corresponding issue has been filed in the Khronos internal GitLab vendor-extension waiting for reporter Need more info or a revision

Comments

@3b
Copy link

3b commented Apr 17, 2023

Example 3 in 12.26. XR_EXT_debug_utils describes the data passed to the callback at two different points as

  • [0] = individual_label with labelName = "WaitFrame"
  • [1] = session_active_region_label with labelName = "Session active"
    and
  • [0] = individual_label with labelName = "BeginFrame"
  • [1] = session_frame_region_label with labelName = "Session Frame 123"
  • [2] = session_active_region_label with labelName = "Session active"

which suggests the callback data contains pointers to the original XrDebugUtilsLabelEXT structures passed to pfnSessionBeginDebugUtilsLabelRegionEXT or pfnSessionInsertDebugUtilsLabelEXT, rather than a copy owned by the runtime.

If that is the case, it should be reconsidered or made very explicit and the example should probably be changed to allocate individual_label on the heap rather than stack since in general Inserted labels will probably last longer than the function that added them.

If the mention of the specific structures was just illustrative and not intended to mean it was actually the same structure, it would be nice to clarify that the pointers to the XrDebugUtilsLabelEXT struct and the contained string do not need to remain valid after the call to Begin or Insert the label (and that those functions must not store them indefinitely.)

Many languages can't easily build long-lived C struct on the stack as in the example, and might need to translate their own native string types, so can be expected to allocate and free both around each call. Even in C, though, I'd probably wrap those functions in something that accepts a string and passes it in a stack-allocated struct local to the wrapper function.

Not sure if any other functions accept similar potentially long-lived data, if so it might be better to put something like that in the implicit validity section.

@rpavlik-bot
Copy link
Collaborator

An issue (number 2016) has been filed to correspond to this issue in the internal Khronos GitLab (Khronos members only: KHR:openxr/openxr#2016 ), to facilitate working group processes.

This GitHub issue will continue to be the main site of discussion.

@rpavlik-bot rpavlik-bot added the synced to gitlab A corresponding issue has been filed in the Khronos internal GitLab label May 17, 2023
@dynamicashuu
Copy link

assign this to me plz

@rpavlik rpavlik added clarification Reflects a spec clarification request vendor-extension labels Jun 8, 2023
@rpavlik
Copy link
Contributor

rpavlik commented Jun 8, 2023

The debug utils extension is implemented by the loader (in addition to some runtimes). I believe the intent is that the loader/runtime keeps a copy of the submitted label data, and it is not actually returning the original structures passed to the label functions.

(the loader implementation of the extension copies the struct, copies the string to a std::string owned by the loader, then updates the copy of the struct to use that string instead.)

Does this make sense to you? If so, a clarification change would be good to see. If you can make it, we'll review it, otherwise it will go on my backlog.

@dynamicashuu if you wish to contribute a clarification you can open a pull request, no need to assign anything. In any case, a change would need approval from the working group.

@rpavlik rpavlik added the waiting for reporter Need more info or a revision label Jun 8, 2023
@3b
Copy link
Author

3b commented Jun 8, 2023

Yeah, that matches my expectations. The API shouldn't retain pointers to user data unless it is explicit in the spec (or otherwise obvious, like callback functions and their user data arguments).

Not sure how I would clarify it though, and unlikely to have any time to think about it any time soon.

Maybe just remove the struct names from the discussion after the code, and instead add comments in the code for each label creation and refer to those instead? Not sure if it is worth also adding a note that the returned strings are copies or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Reflects a spec clarification request synced to gitlab A corresponding issue has been filed in the Khronos internal GitLab vendor-extension waiting for reporter Need more info or a revision
Projects
None yet
Development

No branches or pull requests

4 participants