[libc-commits] [clang] [libc] [llvm] [NVPTX] Implement variadic functions using IR lowering (PR #96015)

Joseph Huber via libc-commits libc-commits at lists.llvm.org
Wed Jul 24 10:44:20 PDT 2024


jhuber6 wrote:

> The change to `NVPTXTargetInfo::getBuiltinVaListKind()` in `clang/lib/Basic/Targets/NVPTX.h` caused a regression in Intel's downstream Clang-based compiler when compiling SYCL code with device compilation targeting NVPTX. CUDA might be similarly impacted, but I haven't verified. There is no need to revert the change; we've backed out the relevant part of the change in our downstream fork for now. But we need to figure out a proper solution for the problem as described below.
> 
> SYCL compilation uses the host standard library headers for both host and device compilation. Microsoft's standard library headers define `va_list` as `char*` as shown at line 72 below:
> 
> ```
> ...\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\vadefs.h:
>  67 #ifndef _VA_LIST_DEFINED
>  68    #define _VA_LIST_DEFINED
>  69    #ifdef _M_CEE_PURE
>  70        typedef System::ArgIterator va_list;
>  71    #else
>  72        typedef char* va_list;
>  73    #endif
>  74 #endif
>  ..
>  97     void  __cdecl __va_start(va_list*, ...);
>  98     void* __cdecl __va_arg(va_list*, ...);
>  99     void  __cdecl __va_end(va_list*);
> 100 
> 101     #define __crt_va_start_a(ap, v) ((void)(__va_start(&ap, _ADDRESSOF(v), _SLOTSIZEOF(v), __alignof(v), _ADDRESSOF(v))))
> 102     #define __crt_va_arg(ap, t)     (*(t *)__va_arg(&ap, _SLOTSIZEOF(t), _APALIGN(t,ap), (t*)0))
> 103     #define __crt_va_end(ap)        ((void)(__va_end(&ap)))
> ```
> 
> The Clang driver interposes on Microsoft's `vadefs.h` header file by placing its own `vadefs.h` header file earlier in the header search path. This header overrides some of the macros defined by Microsoft's `vadefs.h` header file with ones that are intended to work with Clang's builtin variadic function support.
> 
> ```
> <llvm-project>/clang/lib/Headers/vadefs.h:
>  18 #include_next <vadefs.h>
>  19
>  20 /* Override macros from vadefs.h with definitions that work with Clang. */
>  ..
>  34 /* VS 2015 switched to double underscore names, which is an improvement, but now
>  35  * we have to intercept those names too.
>  36  */
>  37 #ifdef __crt_va_start
>  38 #undef __crt_va_start
>  39 #define __crt_va_start(ap, param) __builtin_va_start(ap, param)
>  40 #endif
>  41 #ifdef __crt_va_end
>  42 #undef __crt_va_end
>  43 #define __crt_va_end(ap)          __builtin_va_end(ap)
>  44 #endif
>  45 #ifdef __crt_va_arg
>  46 #undef __crt_va_arg
>  47 #define __crt_va_arg(ap, type)    __builtin_va_arg(ap, type)
>  48 #endif
> ```
> 
> The result is that invocations of the `__crt_va_start`, `__crt_va_end`, and `__crt_va_arg` macros in Microsoft standard library headers end up passing objects of the Microsoft defined `va_list` type (aka, `char*`) to builtin functions like `__builtin_va_start()` that expect objects of type `__builtin_va_list` (aka, `void*`) thus leading to compilation failures when compiling in C++ modes.
> 
> There are at least a few options available to try to address this.
> 
>     1. Revert the change to `NVPTXTargetInfo::getBuiltinVaListKind()` so that `TargetInfo::CharPtrBuiltinVaList` is returned. I don't know what motivated that particular change, so I don't know how feasible this option is.
> 
>     2. Modify `NVPTXTargetInfo::getBuiltinVaListKind()` to conditionally return a value based on which standard library is being used. I'm not sure how feasible this is either. There are currently two targets that conditionally return different values from their `getBuiltinVaListKind()` implementations; Hexagon (see [here](https://github.com/llvm/llvm-project/blob/8fd9624cf729dd722a170a9dfd8f725966515231/clang/lib/Basic/Targets/Hexagon.h#L106-L110)] and ARM (see [here](https://github.com/llvm/llvm-project/blob/8fd9624cf729dd722a170a9dfd8f725966515231/clang/lib/Basic/Targets/ARM.cpp#L1096-L1101)).
> 
>     3. Modify Clang's `vadefs.h` header file to also interpose on Microsoft's definition of `va_list` with a change like the following. This could still lead to problems if `_VA_LIST_DEFINED` is already defined, if `va_list` is already declared (as `char*`), or in code that assumes that `va_list` is defined as `char*` in Microsoft environments.
> 
> 
> ```
>  +  #ifndef _VA_LIST_DEFINED
>  +      #define _VA_LIST_DEFINED
>  +      typedef __builtin_va_list va_list;
>  +  #endif
>  18 #include_next <vadefs.h>
> ```
> 
> Any thoughts on the above are much appreciated!

Is it just the `char *` vs `void *`? We can probably change that back, in fact I thought I did that in the patch but I might've forgotten to push the update.

https://github.com/llvm/llvm-project/pull/96015


More information about the libc-commits mailing list