OpenXR-SDK-Source: Validation layer: fall through/loss of error in two-call idiom

The generated code for two-call idiom calls is producing a static analysis warning “Value stored to ‘xr_result’ is never read”. which is because it’s just falling through after setting xr_result instead of returning it or otherwise accumulating the error.

The code in question looks like:

        // Optional array must be non-NULL when viewCapacityInput is non-zero
        if (0 != viewCapacityInput && nullptr == views) {
            CoreValidLogMessage(gen_instance_info, "VUID-xrEnumerateViewConfigurationViews-views-parameter",
                                VALID_USAGE_DEBUG_SEVERITY_ERROR, "xrEnumerateViewConfigurationViews",
                                objects_info,
                                "Command xrEnumerateViewConfigurationViews param views is NULL, but viewCapacityInput is greater than 0");
            xr_result = XR_ERROR_VALIDATION_FAILURE;
        }
        // Non-optional pointer/array variable that needs to not be NULL
        if (nullptr == viewCountOutput) {
            CoreValidLogMessage(gen_instance_info, "VUID-xrEnumerateViewConfigurationViews-viewCountOutput-parameter",
                                VALID_USAGE_DEBUG_SEVERITY_ERROR, "xrEnumerateViewConfigurationViews", objects_info,
                                "Invalid NULL for uint32_t \"viewCountOutput\" which is not "
                                "optional and must be non-NULL");
            return XR_ERROR_VALIDATION_FAILURE;
        }

This is also triggering a later null dereference, when e.g. in the GenValidUsageInputsXrEnumerateSwapchainImages function, that warning is printed, but then we go through to iterate the null container, etc.

About this issue

  • Original URL
  • State: closed
  • Created 4 years ago
  • Comments: 18 (7 by maintainers)

Commits related to this issue

Most upvoted comments

I said:

I’m not sure why it doesn’t just return xr_result right away. Maybe I should just change any case that sets the result to just return instead.

You said:

so I think now we should return here instead of just setting xr_result

I’m glad you agree with me. I’ll look into the generator.