[Feature]: Make the options parameter to hiprtcCompileProgram const char* const *`

[al42and]

Suggestion Description

Currently, the function has the following signature:

hiprtcResult hiprtcCompileProgram 	(
        hiprtcProgram prog,
		int numOptions,
		const char ** options 
	)

However, this triggers -Wcast-qual warnings with a typical usage:

#include <hip/hiprtc.h>
#include <string.h>

int main()
{
    char *opts[2];
    opts[0] = strdup("-O3");
    opts[1] = strdup("-fno");
    hiprtcProgram prog = NULL;
    // TODO: initialize prog
    hiprtcCompileProgram(prog, 2, opts);
    return 0;
}

$ hipcc --offload-arch=gfx90a -D__HIP_PLATFORM_AMD__ -Wcast-qual constchar.c 
constchar.c:11:35: warning: passing 'char *[2]' to parameter of type 'const char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers]

We cannot simply use (const char **)opts either: https://c-faq.com/ansi/constmismatch.html

$ hipcc --offload-arch=gfx90a -D__HIP_PLATFORM_AMD__ -Wcast-qual constchar.c 
constchar.c:11:49: warning: cast from 'char **' to 'const char **' must have all intermediate pointers const qualified to be safe [-Wcast-qual]
    hiprtcCompileProgram(prog, 2, (const char**)opts);
                                                ^
1 warning generated.

Since the function accepts a constant pointer to constant strings, it is reasonable to make its signature read

hiprtcResult hiprtcCompileProgram 	(
        hiprtcProgram prog,
		int numOptions,
		const char * const* options 
	)

This will both explicitly communicate what the function does and also allow to write the code like above safely by using and explicit cast to (const char* const*)opts

Operating System

Ubuntu

GPU

MI50

ROCm Component

HIP RTC

[yxsamliu]

one concern is that this will change the mangled name therefore old HIP apps won't be able to resolve the symbol in the new HIPRTC shared library. One solution might be to keep the old API as an overloaded function that calls the new API.

[al42and]

one concern is that this will change the mangled name therefore old HIP apps won't be able to resolve the symbol in the new HIPRTC shared library.

At least in ROCm 5.7 and 6.0, the HIP libraries use C linkage, so there should be no problem with mangled names (but also no way to have an overload):

$ nm -D /opt/rocm-5.7.1/hip/lib/libhiprtc.so | grep hiprtcCompileProgram
0000000000043420 T hiprtcCompileProgram

$ nm -D /opt/rocm-6.0.0/lib/libhiprtc.so.6 | grep hiprtcCompileProgram
000000000000fee0 T hiprtcCompileProgram

[satyanveshd]

@yxsamliu Currently, we have a similar task internally to match the signature of hiprtcCreateProgram with nvrtc. i.e, to change the argument types from const char ** to const char* const*. But this was considered breaking backward compatibility and put on hold until 7.0.

@al42and I believe this change will need to wait until next major release ROCm 7.0.

CC @mangupta

[al42and]

Okay, that works for us.

Why, though? It is not breaking ABI, and I don't think the API change breaks any backward compatibility in this case.

[yxsamliu]

one concern is that this will change the mangled name therefore old HIP apps won't be able to resolve the symbol in the new HIPRTC shared library.

At least in ROCm 5.7 and 6.0, the HIP libraries use C linkage, so there should be no problem with mangled names (but also no way to have an overload):

$ nm -D /opt/rocm-5.7.1/hip/lib/libhiprtc.so | grep hiprtcCompileProgram
0000000000043420 T hiprtcCompileProgram

$ nm -D /opt/rocm-6.0.0/lib/libhiprtc.so.6 | grep hiprtcCompileProgram
000000000000fee0 T hiprtcCompileProgram

You are right. This API is actually declared with extern "C" (https://github.com/ROCm/HIP/blob/develop/include/hip/hiprtc.h), therefore it is not mangled.

[yxsamliu]

Okay, that works for us.

Why, though? It is not breaking ABI, and I don't think the API change breaks any backward compatibility in this case.

There are some HIP apps or libraries take address of this function. If they hardcode the function type, changing the function signature may break them.

[keryell]

There are some HIP apps or libraries take address of this function. If they hardcode the function type, changing the function signature may break them.

On the other hand, as you said this is in extern "C" linkage so it is unclear to me how argument types would have anything influence here.

[yxsamliu]

they could write code like:

    // Define the function pointer
    hiprtcResult (*compileProgramPtr)(hiprtcProgram, int, const char**) = nullptr;

    // Assign it to point to hiprtcCompileProgram
    compileProgramPtr = &hiprtcCompileProgram;

If the signature changes, it breaks (https://godbolt.org/z/fGqKxKbbf)

[keryell]

If the signature changes, it breaks (godbolt.org/z/fGqKxKbbf)

Yes, but this was not defined in an extern "C" { } context. So I am confused...

[al42and]

Yes, but this was not defined in an extern "C" { } context. So I am confused...

Per my understanding, the problem is not with linkage, but with C++ type system: a pointer to int f(const char**) is not the same type as a pointer to int f(const char* const*). And there is no implicit cast from one type to the other.

And extern "C" makes things worse. With C++ name mangling, we could have had f(const char**) and f(const char* const*) at the same time. But since we need C linking, we can't have both 😿

[keryell]

Sure, but in that case it is an API issue, not an ABI issue. Once you ship the new headers with this updated extern "C" { } declaration, you need to recompile all the stack with the new headers and there will be no missing implicit conversion. Of course all the API users will have to be updated. :-)
But the interesting point, if you have another user using the old API, it will still work because the function with the same name will be linked from the library, even if behind the scene there is an ODR violation and this should be undefined behavior, but this seems under control.

[keryell]

And extern "C" makes things worse. With C++ name mangling, we could have had f(const char**) and f(const char* const*) at the same time. But since we need C linking, we can't have both 😿

In C++ mode, it would be reasonable to add an API with namespaces like hip::rtc::compiler_program, but that is another story. ;-)

[al42and]

Of course all the API users will have to be updated. :-)

Which means waiting for ROCm 7 (as long as semantic versioning is followed) :(

In C++ mode, it would be reasonable to add an API with namespaces like hip::rtc::compiler_program, but that is another story. ;-)

A story about runtime compilers with a nice C++ API would be interesting indeed.