[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 std::string, 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).

[RossBrunton]

@jhuber6 Looking at this again, as far as I can tell this field is used in at least the following places:

• When building an AMDGPU kernel.
• When generating an error when launching kernels in the CUDA and AMDGPU adapters.
• When printing an error from a kernel trap.
• Debug messages when launching a kernel.
• Used in the Record/Replay interface to identify kernels.

Each of these locations could create their own copy of the kernel name or change their API to allow passing it in, but I'm not sure the benefit of that over just having a copy of the name stored in GenericKernelTy.

[jhuber6]

Can we just turn this into a StringRef so we don't need to be paranoid about null termination?

[RossBrunton]

We have code in several places like this:

    Res = cuModuleGetFunction(&Func, CUDAImage.getModule(), getName());
    if (auto Err = Plugin::check(Res, "error in cuModuleGetFunction('%s'): %s",
                                 getName()))
      return Err;

Which expect getName() to return a c-string.

[jhuber6]

I see, most of its uses require a C-string anyway so it would be a pain to convert it at the site every time.

[jhuber6]

I see, most of its uses require a C-string anyway so it would be a pain to convert it at the site every time.

[jhuber6]

Honestly, I don't know if this is necessary. If the pointer isn't null terminated then this will just explode before we even get here, right? Surely the constructor to SmallString is calling strlen somewhere.

[RossBrunton]

When SmallString copies it into its internal buffer, it only copies strlen bytes, which excludes the null terminator.

[jhuber6]

Then make a StringRef manually with strlen() + 1.

[jhuber6]

Honestly I'm not sure if we need to bother popping back the null terminator, we don't expose the size at all.

[RossBrunton]

I'm weary of some code in the future that does end up using the size or SmallString directly in the future.

Honestly though, with the amount of complications this is causing, I'm wondering if it's just better to use a std::string rather than a SmallString and not have to deal with it.

[jhuber6]

For that I'd just make a variant that returns a string ref with the correct size, if it's a private variable it's reasonable to hide some details from users. You can use a std::string if you want, up to you.

[RossBrunton]

It is now a std::string.

[jhuber6]

std::string has SSO as well so I don't think it'll change too much.