Skip to content

[Coverity][UR] Confusing error codes definition of Memory Export Extension UR APIs #20462

@AlexeySachkov

Description

@AlexeySachkov

This was reported by intel/llvm Coverity scan available at https://scan.coverity.com/projects/intel-llvm?tab=overview

CIDs: 535434, 535466 and 535401

exp-memory-export.yml defines 3 API end points with the same pattern of error checking:

- $X_RESULT_ERROR_INVALID_CONTEXT
- $X_RESULT_ERROR_INVALID_DEVICE
- $X_RESULT_ERROR_INVALID_NULL_HANDLE:
- "`(hDevice == nullptr) || (hContext == nullptr)`"

- $X_RESULT_ERROR_INVALID_CONTEXT
- $X_RESULT_ERROR_INVALID_DEVICE
- $X_RESULT_ERROR_INVALID_NULL_HANDLE:
- "`(hDevice == nullptr) || (hContext == nullptr)`"

- $X_RESULT_ERROR_INVALID_CONTEXT
- $X_RESULT_ERROR_INVALID_DEVICE
- $X_RESULT_ERROR_INVALID_NULL_HANDLE:
- "`(hDevice == nullptr) || (hContext == nullptr)`"

This leads to the following error checking code being generated:

if (NULL == hContext)
  return UR_RESULT_ERROR_INVALID_NULL_HANDLE;
if (NULL == hDevice)
  return UR_RESULT_ERROR_INVALID_NULL_HANDLE;
if ((hDevice == nullptr) || (hContext == nullptr))
  return UR_RESULT_ERROR_INVALID_NULL_HANDLE;

Where the return is logically dead code, because it is unreachable. The code isn't a problem by itself, but I think that it is still worth to address this, because it is not clear which exact error code a caller should expect from this API in case any of the handles are invalid.

Perhaps I misunderstand the definition of X_RESULT_ERROR_INVALID_CONTEXT and it should do a deeper check than just comparing a handle to nulltpr? BTW, why in the YAML file we have 3 distinct error codes (assuming that ERROR_INVALID_CONTEXT and ERROR_INVALID_DEVICE aren't aliases), while in the implementation we only ever emit ERROR_INVALID_NULL_HANDLE?

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions