[UR] Improve handling of error cases in urProgramLink

RossBrunton · RossBrunton · commit 25c75c06999d · 2024-07-09T16:05:32.000+01:00
Note that this change includes a specification change:
urProgramLink now requires the output parameter to contain either
nullptr or some unspecified binary on failure.

As well as this change, a number of bugs have been fixed:
* The Level Zero adapter now correctly returns
  `UR_RESULT_ERROR_PROGRAM_LINK_FAILURE` when linking fails, rather
  than `UR_RESULT_ERROR_UNKNOWN`.
* A workaround has been added for some OpenCL devices that return
  `CL_INVALID_BINARY` rather than `CL_LINK_PROGRAM_FAILURE` on
  linker failure.
* The `phProgram` handle is wrapped in a loader handle by the
  loader even if an error would be returned. This is required by
  Level Zero, which outputs a "dummy" program to store the linker
  log.

Conformance tests have also been added.
diff --git a/include/ur_api.h b/include/ur_api.h
@@ -4277,6 +4277,11 @@ urProgramCompile(
 ///       in `phProgram` will contain a binary of the
 ///       ::UR_PROGRAM_BINARY_TYPE_EXECUTABLE type for each device in
 ///       `hContext`.
+///     - If a non-success code is returned and `phProgram` is not `nullptr`, it
+///       will contain an unspecified program or `nullptr`. Implementations may
+///       use the build log of this program (accessible via
+///       ::urProgramGetBuildInfo) to provide an error log for the linking
+///       failure.
 ///
 /// @remarks
 ///   _Analogues_
@@ -9278,6 +9283,11 @@ urProgramCompileExp(
 ///       in `phProgram` will contain a binary of the
 ///       ::UR_PROGRAM_BINARY_TYPE_EXECUTABLE type for each device in
 ///       `phDevices`.
+///     - If a non-success code is returned and `phProgram` is not `nullptr`, it
+///       will contain an unspecified program or `nullptr`. Implementations may
+///       use the build log of this program (accessible via
+///       ::urProgramGetBuildInfo) to provide an error log for the linking
+///       failure.
 ///
 /// @remarks
 ///   _Analogues_
diff --git a/scripts/core/exp-multi-device-compile.yml b/scripts/core/exp-multi-device-compile.yml
@@ -94,6 +94,7 @@ analogue:
 details:
     - "The application may call this function from simultaneous threads."
     - "Following a successful call to this entry point the program returned in `phProgram` will contain a binary of the $X_PROGRAM_BINARY_TYPE_EXECUTABLE type for each device in `phDevices`."
+    - "If a non-success code is returned and `phProgram` is not `nullptr`, it will contain an unspecified program or `nullptr`. Implementations may use the build log of this program (accessible via $xProgramGetBuildInfo) to provide an error log for the linking failure."
 params:
     - type: $x_context_handle_t
       name: hContext
diff --git a/scripts/core/program.yml b/scripts/core/program.yml
@@ -223,6 +223,7 @@ analogue:
 details:
     - "The application may call this function from simultaneous threads."
     - "Following a successful call to this entry point the program returned in `phProgram` will contain a binary of the $X_PROGRAM_BINARY_TYPE_EXECUTABLE type for each device in `hContext`."
+    - "If a non-success code is returned and `phProgram` is not `nullptr`, it will contain an unspecified program or `nullptr`. Implementations may use the build log of this program (accessible via $xProgramGetBuildInfo) to provide an error log for the linking failure."
 params:
     - type: $x_context_handle_t
       name: hContext
diff --git a/scripts/templates/helper.py b/scripts/templates/helper.py
@@ -1248,6 +1248,30 @@ def get_pfntables(specs, meta, namespace, tags):
 
     return tables
 
+"""
+Public:
+    returns an expression setting required output parameters to null on entry
+"""
+def get_initial_null_set(obj):
+    cname = obj_traits.class_name(obj)
+    lvalue = {
+        ('$xProgram', 'Link'): 'phProgram',
+        ('$xProgram', 'LinkExp'): 'phProgram',
+    }.get((cname, obj['name']))
+    if lvalue is not None:
+        return 'if (nullptr != {0}) {{*{0} = nullptr;}}'.format(lvalue)
+    return ""
+
+"""
+Public:
+    returns true if the function always wraps output pointers in loader handles
+"""
+def always_wrap_outputs(obj):
+    cname = obj_traits.class_name(obj)
+    return (cname, obj['name']) in [
+        ('$xProgram', 'Link'),
+        ('$xProgram', 'LinkExp'),
+    ]
 
 """
 Private:
diff --git a/scripts/templates/ldrddi.cpp.mako b/scripts/templates/ldrddi.cpp.mako
@@ -49,7 +49,7 @@ namespace ur_loader
     {
         ${x}_result_t result = ${X}_RESULT_SUCCESS;<%
         add_local = False
-    %>
+    %>${th.get_initial_null_set(obj)}
 
         %if re.match(r"\w+AdapterGet$", th.make_func_name(n, tags, obj)):
         
@@ -271,7 +271,7 @@ namespace ur_loader
         del add_local
         %>
         %for i, item in enumerate(epilogue):
-        %if 0 == i and not item['release']:
+        %if 0 == i and not item['release'] and not th.always_wrap_outputs(obj):
         if( ${X}_RESULT_SUCCESS != result )
             return result;
 
@@ -309,7 +309,7 @@ namespace ur_loader
                     ${item['factory']}.getInstance( ${item['name']}[ i ], dditable ) );
             %else:
             // convert platform handle to loader handle
-            %if item['optional']:
+            %if item['optional'] or th.always_wrap_outputs(obj):
             if( nullptr != ${item['name']} )
                 *${item['name']} = reinterpret_cast<${item['type']}>(
                     ${item['factory']}.getInstance( *${item['name']}, dditable ) );
diff --git a/scripts/templates/libapi.cpp.mako b/scripts/templates/libapi.cpp.mako
@@ -77,7 +77,7 @@ try {
 %elif th.obj_traits.is_loader_only(obj):
     return ur_lib::${th.make_func_name(n, tags, obj)}(${", ".join(th.make_param_lines(n, tags, obj, format=["name"]))} );
 %else:
-    auto ${th.make_pfn_name(n, tags, obj)} = ${x}_lib::context->${n}DdiTable.${th.get_table_name(n, tags, obj)}.${th.make_pfn_name(n, tags, obj)};
+    ${th.get_initial_null_set(obj)}auto ${th.make_pfn_name(n, tags, obj)} = ${x}_lib::context->${n}DdiTable.${th.get_table_name(n, tags, obj)}.${th.make_pfn_name(n, tags, obj)};
     if( nullptr == ${th.make_pfn_name(n, tags, obj)} )
         return ${X}_RESULT_ERROR_UNINITIALIZED;
 
diff --git a/scripts/templates/nullddi.cpp.mako b/scripts/templates/nullddi.cpp.mako
@@ -38,6 +38,7 @@ namespace driver
         )
     try {
         ${x}_result_t result = ${X}_RESULT_SUCCESS;
+        ${th.get_initial_null_set(obj)}
 
         // if the driver has created a custom function, then call it instead of using the generic path
         auto ${th.make_pfn_name(n, tags, obj)} = d_context.${n}DdiTable.${th.get_table_name(n, tags, obj)}.${th.make_pfn_name(n, tags, obj)};
diff --git a/scripts/templates/trcddi.cpp.mako b/scripts/templates/trcddi.cpp.mako
@@ -36,7 +36,7 @@ namespace ur_tracing_layer
         ${line}
         %endfor
         )
-    {
+    {${th.get_initial_null_set(obj)}
         auto ${th.make_pfn_name(n, tags, obj)} = context.${n}DdiTable.${th.get_table_name(n, tags, obj)}.${th.make_pfn_name(n, tags, obj)};
 
         if( nullptr == ${th.make_pfn_name(n, tags, obj)} )
diff --git a/scripts/templates/valddi.cpp.mako b/scripts/templates/valddi.cpp.mako
@@ -46,7 +46,7 @@ namespace ur_validation_layer
         ${line}
         %endfor
         )
-    {
+    {${th.get_initial_null_set(obj)}
         auto ${th.make_pfn_name(n, tags, obj)} = context.${n}DdiTable.${th.get_table_name(n, tags, obj)}.${th.make_pfn_name(n, tags, obj)};
 
         if( nullptr == ${th.make_pfn_name(n, tags, obj)} ) {
diff --git a/source/adapters/cuda/program.cpp b/source/adapters/cuda/program.cpp
@@ -272,7 +272,10 @@ UR_APIEXPORT ur_result_t UR_APICALL urProgramBuild(ur_context_handle_t hContext,
 
 UR_APIEXPORT ur_result_t UR_APICALL urProgramLinkExp(
     ur_context_handle_t, uint32_t, ur_device_handle_t *, uint32_t,
-    const ur_program_handle_t *, const char *, ur_program_handle_t *) {
+    const ur_program_handle_t *, const char *, ur_program_handle_t *phProgram) {
+  if (nullptr != phProgram) {
+    *phProgram = nullptr;
+  }
   return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
 }
 
@@ -284,6 +287,10 @@ urProgramLink(ur_context_handle_t hContext, uint32_t count,
               const ur_program_handle_t *phPrograms, const char *pOptions,
               ur_program_handle_t *phProgram) {
   ur_result_t Result = UR_RESULT_SUCCESS;
+  if (nullptr != phProgram) {
+    *phProgram = nullptr;
+  }
+
   // All programs must be associated with the same device
   for (auto i = 1u; i < count; ++i)
     UR_ASSERT(phPrograms[i]->getDevice() == phPrograms[0]->getDevice(),
diff --git a/source/adapters/hip/program.cpp b/source/adapters/hip/program.cpp
@@ -326,14 +326,19 @@ UR_APIEXPORT ur_result_t UR_APICALL urProgramBuild(ur_context_handle_t,
 
 UR_APIEXPORT ur_result_t UR_APICALL urProgramLinkExp(
     ur_context_handle_t, uint32_t, ur_device_handle_t *, uint32_t,
-    const ur_program_handle_t *, const char *, ur_program_handle_t *) {
+    const ur_program_handle_t *, const char *, ur_program_handle_t *phProgram) {
+  if (nullptr != phProgram) {
+    *phProgram = nullptr;
+  }
   return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
 }
 
-UR_APIEXPORT ur_result_t UR_APICALL urProgramLink(ur_context_handle_t, uint32_t,
-                                                  const ur_program_handle_t *,
-                                                  const char *,
-                                                  ur_program_handle_t *) {
+UR_APIEXPORT ur_result_t UR_APICALL
+urProgramLink(ur_context_handle_t, uint32_t, const ur_program_handle_t *,
+              const char *, ur_program_handle_t *phProgram) {
+  if (nullptr != phProgram) {
+    *phProgram = nullptr;
+  }
   return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
 }
 
diff --git a/source/adapters/level_zero/common.cpp b/source/adapters/level_zero/common.cpp
@@ -60,6 +60,8 @@ ur_result_t ze2urResult(ze_result_t ZeResult) {
     return UR_RESULT_ERROR_OUT_OF_HOST_MEMORY;
   case ZE_RESULT_ERROR_UNSUPPORTED_FEATURE:
     return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
+  case ZE_RESULT_ERROR_MODULE_LINK_FAILURE:
+    return UR_RESULT_ERROR_PROGRAM_LINK_FAILURE;
   default:
     return UR_RESULT_ERROR_UNKNOWN;
   }
diff --git a/source/adapters/level_zero/program.cpp b/source/adapters/level_zero/program.cpp
@@ -300,6 +300,9 @@ UR_APIEXPORT ur_result_t UR_APICALL urProgramLinkExp(
     ur_program_handle_t
         *phProgram ///< [out] pointer to handle of program object created.
 ) {
+  if (nullptr != phProgram) {
+    *phProgram = nullptr;
+  }
   for (uint32_t i = 0; i < numDevices; i++) {
     UR_ASSERT(hContext->isValidDevice(phDevices[i]),
               UR_RESULT_ERROR_INVALID_DEVICE);
@@ -445,11 +448,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urProgramLinkExp(
       // because the ZeBuildLog tells which symbols are unresolved.
       if (ZeResult == ZE_RESULT_SUCCESS) {
         ZeResult = checkUnresolvedSymbols(ZeModule, &ZeBuildLog);
-        if (ZeResult == ZE_RESULT_ERROR_MODULE_LINK_FAILURE) {
-          UrResult =
-              UR_RESULT_ERROR_UNKNOWN; // TODO:
-                                       // UR_RESULT_ERROR_PROGRAM_LINK_FAILURE;
-        } else if (ZeResult != ZE_RESULT_SUCCESS) {
+        if (ZeResult != ZE_RESULT_SUCCESS) {
           return ze2urResult(ZeResult);
         }
       }
diff --git a/source/adapters/native_cpu/program.cpp b/source/adapters/native_cpu/program.cpp
@@ -119,11 +119,13 @@ UR_APIEXPORT ur_result_t UR_APICALL
 urProgramLink(ur_context_handle_t hContext, uint32_t count,
               const ur_program_handle_t *phPrograms, const char *pOptions,
               ur_program_handle_t *phProgram) {
+  if (nullptr != phProgram) {
+    *phProgram = nullptr;
+  }
   std::ignore = hContext;
   std::ignore = count;
   std::ignore = phPrograms;
   std::ignore = pOptions;
-  std::ignore = phProgram;
 
   DIE_NO_IMPLEMENTATION
 }
@@ -144,7 +146,10 @@ UR_APIEXPORT ur_result_t UR_APICALL urProgramBuildExp(ur_program_handle_t,
 
 UR_APIEXPORT ur_result_t UR_APICALL urProgramLinkExp(
     ur_context_handle_t, uint32_t, ur_device_handle_t *, uint32_t,
-    const ur_program_handle_t *, const char *, ur_program_handle_t *) {
+    const ur_program_handle_t *, const char *, ur_program_handle_t *phProgram) {
+  if (nullptr != phProgram) {
+    *phProgram = nullptr;
+  }
   return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
 }
 
diff --git a/source/adapters/null/ur_nullddi.cpp b/source/adapters/null/ur_nullddi.cpp
@@ -1920,6 +1920,9 @@ __urdlllocal ur_result_t UR_APICALL urProgramLink(
         *phProgram ///< [out] pointer to handle of program object created.
     ) try {
     ur_result_t result = UR_RESULT_SUCCESS;
+    if (nullptr != phProgram) {
+        *phProgram = nullptr;
+    }
 
     // if the driver has created a custom function, then call it instead of using the generic path
     auto pfnLink = d_context.urDdiTable.Program.pfnLink;
@@ -5728,6 +5731,9 @@ __urdlllocal ur_result_t UR_APICALL urProgramLinkExp(
         *phProgram ///< [out] pointer to handle of program object created.
     ) try {
     ur_result_t result = UR_RESULT_SUCCESS;
+    if (nullptr != phProgram) {
+        *phProgram = nullptr;
+    }
 
     // if the driver has created a custom function, then call it instead of using the generic path
     auto pfnLinkExp = d_context.urDdiTable.ProgramExp.pfnLinkExp;
diff --git a/source/adapters/opencl/program.cpp b/source/adapters/opencl/program.cpp
@@ -215,6 +215,13 @@ urProgramLink(ur_context_handle_t hContext, uint32_t count,
                     pOptions, cl_adapter::cast<cl_uint>(count),
                     cl_adapter::cast<const cl_program *>(phPrograms), nullptr,
                     nullptr, &CLResult));
+
+  if (CL_INVALID_BINARY == CLResult) {
+    // Some OpenCL drivers incorrectly return CL_INVALID_BINARY here, convert it
+    // to CL_LINK_PROGRAM_FAILURE
+    CLResult = CL_LINK_PROGRAM_FAILURE;
+  }
+
   CL_RETURN_ON_FAILURE(CLResult);
 
   return UR_RESULT_SUCCESS;
@@ -236,7 +243,10 @@ UR_APIEXPORT ur_result_t UR_APICALL urProgramBuildExp(ur_program_handle_t,
 
 UR_APIEXPORT ur_result_t UR_APICALL urProgramLinkExp(
     ur_context_handle_t, uint32_t, ur_device_handle_t *, uint32_t,
-    const ur_program_handle_t *, const char *, ur_program_handle_t *) {
+    const ur_program_handle_t *, const char *, ur_program_handle_t *phProgram) {
+  if (nullptr != phProgram) {
+    *phProgram = nullptr;
+  }
   return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
 }
 
diff --git a/source/loader/layers/tracing/ur_trcddi.cpp b/source/loader/layers/tracing/ur_trcddi.cpp
@@ -2486,6 +2486,9 @@ __urdlllocal ur_result_t UR_APICALL urProgramLink(
     ur_program_handle_t
         *phProgram ///< [out] pointer to handle of program object created.
 ) {
+    if (nullptr != phProgram) {
+        *phProgram = nullptr;
+    }
     auto pfnLink = context.urDdiTable.Program.pfnLink;
 
     if (nullptr == pfnLink) {
@@ -7639,6 +7642,9 @@ __urdlllocal ur_result_t UR_APICALL urProgramLinkExp(
     ur_program_handle_t
         *phProgram ///< [out] pointer to handle of program object created.
 ) {
+    if (nullptr != phProgram) {
+        *phProgram = nullptr;
+    }
     auto pfnLinkExp = context.urDdiTable.ProgramExp.pfnLinkExp;
 
     if (nullptr == pfnLinkExp) {
diff --git a/source/loader/layers/validation/ur_valddi.cpp b/source/loader/layers/validation/ur_valddi.cpp
@@ -2849,6 +2849,9 @@ __urdlllocal ur_result_t UR_APICALL urProgramLink(
     ur_program_handle_t
         *phProgram ///< [out] pointer to handle of program object created.
 ) {
+    if (nullptr != phProgram) {
+        *phProgram = nullptr;
+    }
     auto pfnLink = context.urDdiTable.Program.pfnLink;
 
     if (nullptr == pfnLink) {
@@ -9285,6 +9288,9 @@ __urdlllocal ur_result_t UR_APICALL urProgramLinkExp(
     ur_program_handle_t
         *phProgram ///< [out] pointer to handle of program object created.
 ) {
+    if (nullptr != phProgram) {
+        *phProgram = nullptr;
+    }
     auto pfnLinkExp = context.urDdiTable.ProgramExp.pfnLinkExp;
 
     if (nullptr == pfnLinkExp) {
diff --git a/source/loader/ur_ldrddi.cpp b/source/loader/ur_ldrddi.cpp
@@ -2556,6 +2556,9 @@ __urdlllocal ur_result_t UR_APICALL urProgramLink(
         *phProgram ///< [out] pointer to handle of program object created.
 ) {
     ur_result_t result = UR_RESULT_SUCCESS;
+    if (nullptr != phProgram) {
+        *phProgram = nullptr;
+    }
 
     // extract platform's function pointer table
     auto dditable = reinterpret_cast<ur_context_object_t *>(hContext)->dditable;
@@ -2578,14 +2581,12 @@ __urdlllocal ur_result_t UR_APICALL urProgramLink(
     result =
         pfnLink(hContext, count, phProgramsLocal.data(), pOptions, phProgram);
 
-    if (UR_RESULT_SUCCESS != result) {
-        return result;
-    }
-
     try {
         // convert platform handle to loader handle
-        *phProgram = reinterpret_cast<ur_program_handle_t>(
-            ur_program_factory.getInstance(*phProgram, dditable));
+        if (nullptr != phProgram) {
+            *phProgram = reinterpret_cast<ur_program_handle_t>(
+                ur_program_factory.getInstance(*phProgram, dditable));
+        }
     } catch (std::bad_alloc &) {
         result = UR_RESULT_ERROR_OUT_OF_HOST_MEMORY;
     }
@@ -7909,6 +7910,9 @@ __urdlllocal ur_result_t UR_APICALL urProgramLinkExp(
         *phProgram ///< [out] pointer to handle of program object created.
 ) {
     ur_result_t result = UR_RESULT_SUCCESS;
+    if (nullptr != phProgram) {
+        *phProgram = nullptr;
+    }
 
     // extract platform's function pointer table
     auto dditable = reinterpret_cast<ur_context_object_t *>(hContext)->dditable;
@@ -7938,14 +7942,12 @@ __urdlllocal ur_result_t UR_APICALL urProgramLinkExp(
     result = pfnLinkExp(hContext, numDevices, phDevicesLocal.data(), count,
                         phProgramsLocal.data(), pOptions, phProgram);
 
-    if (UR_RESULT_SUCCESS != result) {
-        return result;
-    }
-
     try {
         // convert platform handle to loader handle
-        *phProgram = reinterpret_cast<ur_program_handle_t>(
-            ur_program_factory.getInstance(*phProgram, dditable));
+        if (nullptr != phProgram) {
+            *phProgram = reinterpret_cast<ur_program_handle_t>(
+                ur_program_factory.getInstance(*phProgram, dditable));
+        }
     } catch (std::bad_alloc &) {
         result = UR_RESULT_ERROR_OUT_OF_HOST_MEMORY;
     }
diff --git a/source/loader/ur_libapi.cpp b/source/loader/ur_libapi.cpp
diff --git a/source/ur_api.cpp b/source/ur_api.cpp
diff --git a/test/conformance/device_code/CMakeLists.txt b/test/conformance/device_code/CMakeLists.txt
diff --git a/test/conformance/device_code/linker_error.cpp b/test/conformance/device_code/linker_error.cpp
diff --git a/test/conformance/program/program_adapter_native_cpu.match b/test/conformance/program/program_adapter_native_cpu.match
diff --git a/test/conformance/program/urProgramLink.cpp b/test/conformance/program/urProgramLink.cpp

Original file line number	Diff line number	Diff line change
`@@ -38,6 +38,7 @@ namespace driver`
`38`	`38`	`)`
`39`	`39`	`try {`
`40`	`40`	`${x}_result_t result = ${X}_RESULT_SUCCESS;`
	`41`	`+ ${th.get_initial_null_set(obj)}`
`41`	`42`
`42`	`43`	`// if the driver has created a custom function, then call it instead of using the generic path`
`43`	`44`	`auto ${th.make_pfn_name(n, tags, obj)} = d_context.${n}DdiTable.${th.get_table_name(n, tags, obj)}.${th.make_pfn_name(n, tags, obj)};`
Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ namespace ur_tracing_layer`
`36`	`36`	`${line}`
`37`	`37`	`%endfor`
`38`	`38`	`)`
`39`		`- {`
	`39`	`+ {${th.get_initial_null_set(obj)}`
`40`	`40`	`auto ${th.make_pfn_name(n, tags, obj)} = context.${n}DdiTable.${th.get_table_name(n, tags, obj)}.${th.make_pfn_name(n, tags, obj)};`
`41`	`41`
`42`	`42`	`if( nullptr == ${th.make_pfn_name(n, tags, obj)} )`
Original file line number	Diff line number	Diff line change
`@@ -46,7 +46,7 @@ namespace ur_validation_layer`
`46`	`46`	`${line}`
`47`	`47`	`%endfor`
`48`	`48`	`)`
`49`		`- {`
	`49`	`+ {${th.get_initial_null_set(obj)}`
`50`	`50`	`auto ${th.make_pfn_name(n, tags, obj)} = context.${n}DdiTable.${th.get_table_name(n, tags, obj)}.${th.make_pfn_name(n, tags, obj)};`
`51`	`51`
`52`	`52`	`if( nullptr == ${th.make_pfn_name(n, tags, obj)} ) {`