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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
