[libc] Remove the optional arguments for NVPTX constructors

[jhuber6]

Summary:
We call the global constructors by function pointer. For whatever reason
the NVPTX architecture relies very specifically on the arguments to the
function pointer invocation matching what the function is implemented
as. This is problematic as most of these constructors are generated
with no arguments. This patch removes the extended arguments that GNU
and LLVM use for the constructors optionally so that it can support the
common case.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Joseph Huber (jhuber6)

Changes

Summary:
We call the global constructors by function pointer. For whatever reason
the NVPTX architecture relies very specifically on the arguments to the
functoin pointer invocation matching what the function is implemented
as. This is problembatic as most of these constructors are generated
with no arguments. This patch removes the extended arguments that GNU
and LLVM use for the constructors optionally so that it can support the
common case.

---

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

2 Files Affected:

• (modified) libc/startup/gpu/nvptx/start.cpp (+5-3)
• (modified) libc/test/integration/startup/gpu/init_fini_array_test.cpp (+2-2)

diff --git a/libc/startup/gpu/nvptx/start.cpp b/libc/startup/gpu/nvptx/start.cpp
index 1ff187a577789f1..554c2ac385f0141 100644
--- a/libc/startup/gpu/nvptx/start.cpp
+++ b/libc/startup/gpu/nvptx/start.cpp
@@ -24,13 +24,15 @@ uintptr_t *__fini_array_start [[gnu::visibility("protected")]];
 uintptr_t *__fini_array_end [[gnu::visibility("protected")]];
 }
 
-using InitCallback = void(int, char **, char **);
+// Nvidia requires that the signature of the function pointers match. This means
+// we cannot support the extended constructor arguments.
+using InitCallback = void(void);
 using FiniCallback = void(void);
 
-static void call_init_array_callbacks(int argc, char **argv, char **env) {
+static void call_init_array_callbacks(int, char **, char **) {
   size_t init_array_size = __init_array_end - __init_array_start;
   for (size_t i = 0; i < init_array_size; ++i)
-    reinterpret_cast<InitCallback *>(__init_array_start[i])(argc, argv, env);
+    reinterpret_cast<InitCallback *>(__init_array_start[i])();
 }
 
 static void call_fini_array_callbacks() {
diff --git a/libc/test/integration/startup/gpu/init_fini_array_test.cpp b/libc/test/integration/startup/gpu/init_fini_array_test.cpp
index ead05f0240585a4..ceedd5fc813589c 100644
--- a/libc/test/integration/startup/gpu/init_fini_array_test.cpp
+++ b/libc/test/integration/startup/gpu/init_fini_array_test.cpp
@@ -37,11 +37,11 @@ A global(GLOBAL_INDEX, INITVAL_INITIALIZER);
 int initval = 0;
 int before = 0;
 
-__attribute__((constructor(101))) void run_before(int, char **, char **) {
+__attribute__((constructor(101))) void run_before() {
   before = BEFORE_INITIALIZER;
 }
 
-__attribute__((constructor(65535))) void run_after(int, char **, char **) {
+__attribute__((constructor(65535))) void run_after() {
   ASSERT_EQ(before, BEFORE_INITIALIZER);
 }
 

[Artem-B]

I'm not familiar enough with the conventions for calling the initializers. Added @MaskRay who'd have a better idea.

[jhuber6]

I'm not familiar enough with the conventions for calling the initializers. Added @MaskRay who'd have a better idea.

I think @AaronBallman was also working with this recently. Basically there's an optional form that glibc and some others use that allow passing the arugments to main to the constructor. But the standard version is no arguments. The problem is that NVPTX needs to have the signature matching exactly so we apparently can't support the extended form.

[AntonRydahl]

Maybe this is a stupid question, but is it still ok that the function signature of main is int main(int argc, char **argv, char **envp)?

[jhuber6]

Maybe this is a stupid question, but is it still ok that the function signature of main is int main(int argc, char **argv, char **envp)?

Yes, so long as the same signature is used every time you invoke it, which is currently the case for the tests.

[AaronBallman]

I'm not familiar enough with the conventions for calling the initializers. Added @MaskRay who'd have a better idea.

I think @AaronBallman was also working with this recently. Basically there's an optional form that glibc and some others use that allow passing the arugments to main to the constructor. But the standard version is no arguments. The problem is that NVPTX needs to have the signature matching exactly so we apparently can't support the extended form.

Yeah, that was this work: #67673 (landed and then reverted and then I ran out of time to get it across the finish line -- if someone wanted to pick that up, I would be happy to review/help).

[jhuber6]

Yeah, that was this work: #67673 (landed and then reverted and then I ran out of time to get it across the finish line -- if someone wanted to pick that up, I would be happy to review/help).

This patch basically defaults to the more common form of these constructors since we cannot support both on NVPTX it seems. Since this is to do with their binary tools those are all proprietary so it's out of my hands. We just need to be careful not to use the extended form for any of the tests.

[JonChesterfield]

Supporting the extended form doesn't seem fundamentally important. It's not accessible without the attribute(constructor) syntax which is itself a compiler extension. It should have a sema check - looks like that's the above wip.

I'd be happy to accept this if you applied the same simplification to amdgpu and fix the typos in the commit message. We won't have users in the wild that depend on the extended constructor syntax (yet), and we can leave an issue around for either getting nvidia to improve their tooling or implementing a workaround (e.g. we could stash the arguments in global state and retrieve them)

[jhuber6]

Supporting the extended form doesn't seem fundamentally important. It's not accessible without the attribute(constructor) syntax which is itself a compiler extension. It should have a sema check - looks like that's the above wip.

Yes, that was Aaron's work which shouldn't affect too much.

I'd be happy to accept this if you applied the same simplification to amdgpu and fix the typos in the commit message. We won't have users in the wild that depend on the extended constructor syntax (yet), and we can leave an issue around for either getting nvidia to improve their tooling or implementing a workaround (e.g. we could stash the arguments in global state and retrieve them)

AMDGPU doesn't need to worry about this, it can call either form in a similar manner to calling main with and without arguments.

[jhuber6]

Ping

[AntonRydahl]

Considering the arguments discussed here, I believe this PR should be safe.