[Offload] Store kernel name in GenericKernelTy

[RossBrunton]

GenericKernelTy has a pointer to the name that was used to create it.
However, the name passed in as an argument may not outlive the kernel.
Instead, GenericKernelTy now contains a SmallString, and copies the
name into there.

[llvmbot]

@llvm/pr-subscribers-offload

Author: Ross Brunton (RossBrunton)

Changes

GenericKernelTy has a pointer to the name that was used to create it.
However, the name passed in as an argument may not outlive the kernel.
Instead, GenericKernelTy now contains a SmallString, and copies the
name into there.

---

Full diff: https://github.com/llvm/llvm-project/pull/142799.diff

2 Files Affected:

• (modified) offload/plugins-nextgen/common/include/PluginInterface.h (+8-3)
• (modified) offload/plugins-nextgen/common/src/PluginInterface.cpp (+1-1)

diff --git a/offload/plugins-nextgen/common/include/PluginInterface.h b/offload/plugins-nextgen/common/include/PluginInterface.h
index d2437908a0a6f..88dd516697f57 100644
--- a/offload/plugins-nextgen/common/include/PluginInterface.h
+++ b/offload/plugins-nextgen/common/include/PluginInterface.h
@@ -256,7 +256,12 @@ class DeviceImageTy {
 struct GenericKernelTy {
   /// Construct a kernel with a name and a execution mode.
   GenericKernelTy(const char *Name)
-      : Name(Name), PreferredNumThreads(0), MaxNumThreads(0) {}
+      : Name(Name), PreferredNumThreads(0), MaxNumThreads(0) {
+    // Ensure that the name is null terminated so getName() can just return the
+    // pointer
+    this->Name.push_back('\0');
+    this->Name.pop_back();
+  }
 
   virtual ~GenericKernelTy() {}
 
@@ -277,7 +282,7 @@ struct GenericKernelTy {
                            AsyncInfoWrapperTy &AsyncInfoWrapper) const = 0;
 
   /// Get the kernel name.
-  const char *getName() const { return Name; }
+  const char *getName() const { return Name.data(); }
 
   /// Get the kernel image.
   DeviceImageTy &getImage() const {
@@ -373,7 +378,7 @@ struct GenericKernelTy {
   }
 
   /// The kernel name.
-  const char *Name;
+  SmallString<32> Name;
 
   /// The image that contains this kernel.
   DeviceImageTy *ImagePtr = nullptr;
diff --git a/offload/plugins-nextgen/common/src/PluginInterface.cpp b/offload/plugins-nextgen/common/src/PluginInterface.cpp
index f9e316adad8f4..9170a6a75d140 100644
--- a/offload/plugins-nextgen/common/src/PluginInterface.cpp
+++ b/offload/plugins-nextgen/common/src/PluginInterface.cpp
@@ -456,7 +456,7 @@ Error GenericKernelTy::init(GenericDeviceTy &GenericDevice,
     KernelEnvironment = KernelEnvironmentTy{};
     DP("Failed to read kernel environment for '%s' Using default Bare (0) "
        "execution mode\n",
-       Name);
+       getName());
   }
 
   // Max = Config.Max > 0 ? min(Config.Max, Device.Max) : Device.Max;

[jhuber6]

This is only used for debugging where this is valid since it's nested. I could see getting rid of this field otherwise. I think the user should likely be the one to keep track of symbol names, just like how dlsym() doesn't give you a pointer to a struct that contains the name.

[RossBrunton]

As well as the various INFO calls, it is also used for the Record/Replay thing. Regardless, I think the following is UB with current liboffload:

ol_kernel_handle_t kernel;
if (someOption()) {
  std::string name = getOptionValue();
  olGetKernel(program, name.c_str(), &kernel);
} else {
  olGetKernel(program, "main", &kernel);
}
olLaunchKernel(device, kernel, [...]);

Although I think it might only be UB when certain debugging flags are set (which probably is going to make it more of a pain if there are issues).