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

XrPath behavior incorrectly described as "undefined" #22

Open
Ralith opened this issue Jul 30, 2019 · 7 comments

Comments

@Ralith
Copy link

commented Jul 30, 2019

The definition of XrPath states:

An XrPath that is received from one XrInstance may not be used with another. Such an invalid use may be detected and result in an error being returned, or it may result in undefined behavior.

I understand that Khronos' intention is that the behavior is, in fact, defined to be either an error or as an unspecified valid path had been used. However, the term "undefined behavior" is widely recognized, and indeed used elsewhere in the spec, to mean completely arbitrary consequences, such as dereferencing an invalid pointer. The language should be corrected to something like "behavior as if an unspecified valid path was used," thereby providing a closed set of possible outcomes. "Unspecified behavior" is another possibility, but the meaning is less clear and it may be useful to guarantee that e.g. aborting the process isn't permitted, if that's indeed the intention.

@rpavlik

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

would it be useful to enumerate the cases?

@rpavlik rpavlik added the bug label Jul 30, 2019

@Ralith

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

An explicit enumeration of cases of undefined behavior, similar to the enumeration of cases requiring external synchronization, would absolutely be useful for auditing the correctness of downstream code.

e: Oh, you meant the possible outcomes for using an arbitrary bitpattern as an XrPath. Yeah, if the set of outcomes is intended to be small and closed that seems natural.

@rpavlik

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

I mean, both are possible, and the externsync-like enumeration seems useful if a bit tricky (those lists are generated with XML markup, so we'd have to extend the registry schema in such a way to make it easy to mark up potential UB), but yeah, the listing of xrpath bitpattern cases seems like an easy fix:

a bit pattern supplied as an XrPath

  • does not correspond to a known XrPath-path name string association: runtime must return XR_ERROR_PATH_INVALID
  • does correspond to a known XrPath-path name string association:
    • the path it corresponds to is supported for the parameter/field it is passed as: "normal" behavior of the function (though unspecified what it's acting on, since there is no fixed meaning across instances for that bit pattern - might be /user/hand/left now, /user/hand/right tomorrow)
    • the path it corresponds to is not supported for the parameter/field it is passed as: (e.g. /user/hand/left in a interaction profile path field) runtime must return XR_ERROR_PATH_UNSUPPORTED
@Ralith

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

Is that stronger than necessary? I feel like it would be sufficient to permit either of the latter two cases for bitpatterns that do not correspond to an existing association, e.g. if a runtime does not inspect all bits of an XrPath.

@rpavlik

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

I think it's what we have right now - at least, I think that's how the WG interprets the spec: if a bit pattern can't round-trip thru xrStringToPath/PathToString, it needs to be detected as XR_ERROR_PATH_INVALID.

@oddhack

This comment has been minimized.

Copy link

commented Jul 31, 2019

BTW @rpavlik I'm not sure if you're aware, but a while back we went through an exercise in the Vulkan spec to make sure that every use of 'undefined' was either 'undefined behavior' or 'undefined results', the latter in the case of query operations for the most part. Then we marked all the uses of the word 'undefined' as the undefined: macro so we'd be able to detect new cases in the future. Might be useful for XR as well.

@rpavlik

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

ah, that's a good thought. Probably should do that, yes.

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