[libomptarget] Fail on missing symbols in device image

[JonChesterfield]

Summary

[libomptarget] Fail on missing symbols in device image

_exec_mode is unconditionally emitted by codegen. It missing implies a corrupt
device image, which may have further problems.

omptarget_device_environment is unconditionally present in deviceRTL. It
missing implies that some of the deviceRTL has not been linked into the
application.

[JonChesterfield]

Cuda reads from _exec_mode, and writes to omptarget_device_environment, after the image has been loaded onto the gpu. Amdgpu read/writes the device image before it is loaded onto the gpu to avoid the extra round trip. This means amdgpu has an elf symbol table parsing function and makes a possibly avoidable copy of the device image (I haven't checked whether libomptarget handles a plugin mutating the image gracefully). Do we want the same optimisation for cuda?

[jdoerfert]

This makes sense, can we test this?

[JonChesterfield]

Manually, yeah - jury rig clang to elide one symbol, rename the one in deviceRTL. Automatically? Not sure it's worth the setup.

[jdoerfert]

In D91746#2408074, @JonChesterfield wrote:

Manually, yeah - jury rig clang to elide one symbol, rename the one in deviceRTL. Automatically? Not sure it's worth the setup.

Hm, I thought we could prepare an image. Or maybe we can build an offload test, strip a symbol via existing llvm tools, then run and observe the result?
You think that can be done reasonably easily?

[JonChesterfield]

llvm-objcopy is willing to strip the symbols from the device image. Can get the name of exec_mode from llvm-nm. I'm not sure we have tooling for pulling apart the openmp image and reassembling it, or for injecting an external tool to mangle the device image on the way into the binary. Expressing the result in lit/filecheck form means long pipelines and/or several temporary files, but is doable.

I would say the test would be substantially more complicated than the change, and much more likely to be buggy. If we had better testing infra - stuff for making binary files and dicing them - I'd be more in favour.

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp

 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
 #include <dlfcn.h>
 #include <elf.h>
 #include <ffi.h>
 #include <fstream>
 #include <iostream>
 #include <libelf.h>
 #include <list>
 #include <memory>
 #include <mutex>
 #include <shared_mutex>
 #include <thread>
 #include <unordered_map>
 #include <vector>
 
#endif
   auto on_deserialized_data = [&](void *data, size_t size) -> atmi_status_t {
     const char *device_env_Name = "omptarget_device_environment";
     symbol_info si;
     int rc = get_symbol_info_without_loading((char *)image->ImageStart,
                                              img_size, device_env_Name, &si);
     if (rc != 0) {
       DP("Finding global device environment '%s' - symbol missing.\n",
          device_env_Name);
-      // no need to return FAIL, consider this is a not a device debug build.
-      return ATMI_STATUS_SUCCESS;
+      return ATMI_STATUS_ERROR;
     }
     if (si.size != sizeof(host_device_env)) {
       return ATMI_STATUS_ERROR;
     }
     DP("Setting global device environment %lu bytes\n", si.size);
     uint64_t offset = (char *)si.addr - (char *)image->ImageStart;
     void *pos = (char *)data + offset;
     memcpy(pos, &host_device_env, sizeof(host_device_env));
if (err == ATMI_STATUS_SUCCESS) {
 
         if (ExecModeVal < 0 || ExecModeVal > 1) {
           DP("Error wrong exec_mode value specified in HSA code object file: "
              "%d\n",
              ExecModeVal);
           return NULL;
         }
       } else {
-        DP("Loading global exec_mode '%s' - symbol missing, using default "
-           "value "
-           "GENERIC (1)\n",
-           ExecModeName);
+        DP("Loading global exec_mode '%s' - symbol missing\n", ExecModeName);
+        return NULL;
       }
       check("Loading computation property", err);
 
       // Flat group size
       std::string WGSizeNameStr(e->name);
       WGSizeNameStr += "_wg_size";
       const char *WGSizeName = WGSizeNameStr.c_str();
 

openmp/libomptarget/plugins/cuda/src/rtl.cpp

 //===----RTLs/cuda/src/rtl.cpp - Target RTLs Implementation ------- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // RTL for CUDA machine
 //
 //===----------------------------------------------------------------------===//
 
 #include <cassert>
 #include <cstddef>
 #include <cuda.h>
 #include <list>
 #include <memory>
 #include <mutex>
 #include <string>
 #include <vector>
 
 #include "Debug.h"
 #include "omptargetplugin.h"
for (const __tgt_offload_entry *E = HostBegin; E != HostEnd; ++E) {
         }
 
         if (ExecModeVal < 0 || ExecModeVal > 1) {
           DP("Error wrong exec_mode value specified in cubin file: %d\n",
              ExecModeVal);
           return nullptr;
         }
       } else {
-        DP("Loading global exec_mode '%s' - symbol missing, using default "
-           "value GENERIC (1)\n",
-           ExecModeName);
+        DP("Loading global exec_mode '%s' - symbol missing\n", ExecModeName);
         CUDA_ERR_STRING(Err);
+        return nullptr;
       }
 
       KernelsList.emplace_back(Func, ExecModeVal);
 
       __tgt_offload_entry Entry = *E;
       Entry.addr = &KernelsList.back();
       addOffloadEntry(DeviceId, Entry);
     }
#endif
           CUDA_ERR_STRING(Err);
           return nullptr;
         }
 
         DP("Sending global device environment data %zu bytes\n", CUSize);
       } else {
         DP("Finding global device environment '%s' - symbol missing.\n",
            DeviceEnvName);
-        DP("Continue, considering this is a device RTL which does not accept "
-           "environment setting.\n");
+        return nullptr;
       }
     }
 
     return getOffloadEntriesTable(DeviceId);
   }
 
   void *dataAlloc(const int DeviceId, const int64_t Size) const {
     if (Size == 0)