[PATCH 0/1] MR935: widl: Generate "static inline" instead of "static FORCEINLINE" for COM inline wrappers.
mingw-w64 defines __forceinline (and therefore FORCEINLINE) as "extern __inline__ __attribute__((__always_inline__,__gnu_inline__)). This means that COM inline wrappers specify multiple storage classes and hence cannot be compiled. Wine defines FORCEINLINE simply as "inline" (and uses "static" everywhere), so this is a non-issue for Wine. However, since Wine and mingw-w64 share the source code of widl and of most IDL headers, this patch changes the definition for both projects. There's no reason to force inlining here, especially since the wrappers need to be manually enabled, and we don't need to match PSDK semantics where these wrappers don't even exist. In practice, use "__inline__" instead of "inline" for GNU C targets, to preserve compatibility with C89 in mingw-w64 headers. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/935
From: Zebediah Figura <zfigura(a)codeweavers.com> mingw-w64 defines __forceinline (and therefore FORCEINLINE) as "extern __inline__ __attribute__((__always_inline__,__gnu_inline__)). This means that COM inline wrappers specify multiple storage classes and hence cannot be compiled. Wine defines FORCEINLINE simply as "inline" (and uses "static" everywhere), so this is a non-issue for Wine. However, since Wine and mingw-w64 share the source code of widl and of most IDL headers, this patch changes the definition for both projects. There's no reason to force inlining here, especially since the wrappers need to be manually enabled, and we don't need to match PSDK semantics where these wrappers don't even exist. In practice, use "__inline__" instead of "inline" for GNU C targets, to preserve compatibility with C89 in mingw-w64 headers. --- tools/widl/header.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tools/widl/header.c b/tools/widl/header.c index 8b9c64e1e38..90c014192d0 100644 --- a/tools/widl/header.c +++ b/tools/widl/header.c @@ -1315,7 +1315,7 @@ static void write_inline_wrappers(FILE *header, const type_t *iface, const type_ if (!is_callas(func->attrs)) { const var_t *arg; - fprintf(header, "static FORCEINLINE "); + fprintf(header, "__WIDL_INLINE "); write_type_decl_left(header, type_function_get_ret(func->declspec.type)); fprintf(header, " %s_%s(", name, get_name(func)); write_args(header, type_function_get_args(func->declspec.type), name, 1, FALSE, NAME_C); @@ -2155,6 +2155,14 @@ void write_header(const statement_list_t *stmts) fprintf(header, "#ifndef __%s__\n", header_token); fprintf(header, "#define __%s__\n\n", header_token); + fprintf(header, "#ifndef __WIDL_INLINE\n"); + fprintf(header, "#if defined(__cplusplus) || defined(_MSC_VER)\n"); + fprintf(header, "#define __WIDL_INLINE static inline\n"); + fprintf(header, "#elif defined(__GNUC__)\n"); + fprintf(header, "#define __WIDL_INLINE static __inline__\n"); + fprintf(header, "#endif\n\n"); + fprintf(header, "#endif\n\n"); + fprintf(header, "/* Forward declarations */\n\n"); write_forward_decls(header, stmts); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/935
It looks either like a reason to change our `FORCEINLINE` definition instead to match mingw-w64, and remove the static qualifiers everywhere, or an issue on the mingw-w64 side: Windows SDK also uses FORCEINLINE / __forceinline with `static` qualifier in some places. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/935#note_9352
Huw Davies (@huw) commented about tools/widl/header.c:
fprintf(header, "#ifndef __%s__\n", header_token); fprintf(header, "#define __%s__\n\n", header_token);
+ fprintf(header, "#ifndef __WIDL_INLINE\n"); + fprintf(header, "#if defined(__cplusplus) || defined(_MSC_VER)\n"); + fprintf(header, "#define __WIDL_INLINE static inline\n"); + fprintf(header, "#elif defined(__GNUC__)\n"); + fprintf(header, "#define __WIDL_INLINE static __inline__\n"); + fprintf(header, "#endif\n\n"); I don't think we want the double '\n' on this line (but we do on the next line).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/935#note_9354
On Tue Sep 27 11:10:22 2022 +0000, Rémi Bernon wrote:
It looks either like a reason to change our `FORCEINLINE` definition instead to match mingw-w64, and remove the static qualifiers everywhere, or an issue on the mingw-w64 side: Windows SDK also uses FORCEINLINE / __forceinline with `static` qualifier in some places. Yeah, it's an issue in mingw-w64 which is hard to change there without breaking things. Anyway, I don't think we want to match mingw-w64 in Wine in this case.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/935#note_9376
On 9/27/22 06:10, Jacek Caban (@jacek) wrote:
On Tue Sep 27 11:10:22 2022 +0000, Rémi Bernon wrote:
It looks either like a reason to change our `FORCEINLINE` definition instead to match mingw-w64, and remove the static qualifiers everywhere, or an issue on the mingw-w64 side: Windows SDK also uses FORCEINLINE / __forceinline with `static` qualifier in some places. Yeah, it's an issue in mingw-w64 which is hard to change there without breaking things. Anyway, I don't think we want to match mingw-w64 in Wine in this case.
The problem isn't really that FORCEINLINE is broken so much as that non-static inline is broken. MSVC "inline" has semantics that are impossible to match with current gcc. There's a more complete explanation at [1]. MSVC __forceinline is just MSVC inline plus an equivalent of the "always_inline" attribute, i.e. it inlines even without -finline. I'm not opposed to adding always_inline here, but it doesn't seem necessary. Incidentally, I remember searching for FORCEINLINE in the Windows SDK, and I never saw it used with static or extern. What examples did you see there? [1] https://sourceforge.net/p/mingw-w64/mailman/message/37685650/
Jacek Caban (@jacek) commented about tools/widl/header.c:
fprintf(header, "#ifndef __%s__\n", header_token); fprintf(header, "#define __%s__\n\n", header_token);
+ fprintf(header, "#ifndef __WIDL_INLINE\n"); + fprintf(header, "#if defined(__cplusplus) || defined(_MSC_VER)\n"); + fprintf(header, "#define __WIDL_INLINE static inline\n"); + fprintf(header, "#elif defined(__GNUC__)\n"); + fprintf(header, "#define __WIDL_INLINE static __inline__\n"); + fprintf(header, "#endif\n\n"); Do we really need __WIDL_INLINE? It seems to me that we could just use static inline unconditionally.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/935#note_9380
On 9/27/22 06:14, Jacek Caban (@jacek) wrote:
Jacek Caban (@jacek) commented about tools/widl/header.c:
fprintf(header, "#ifndef __%s__\n", header_token); fprintf(header, "#define __%s__\n\n", header_token);
+ fprintf(header, "#ifndef __WIDL_INLINE\n"); + fprintf(header, "#if defined(__cplusplus) || defined(_MSC_VER)\n"); + fprintf(header, "#define __WIDL_INLINE static inline\n"); + fprintf(header, "#elif defined(__GNUC__)\n"); + fprintf(header, "#define __WIDL_INLINE static __inline__\n"); + fprintf(header, "#endif\n\n");
Do we really need __WIDL_INLINE? It seems to me that we could just use static inline unconditionally.
For Wine, we can, but "inline" isn't C89, and my understanding is that mingw-w64 would like to maintain c89 compatiblity (and widl is shared with mingw-w64). Cf. [1]. [1] https://sourceforge.net/p/mingw-w64/mailman/message/37685824/
On Tue Sep 27 15:44:59 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 9/27/22 06:14, Jacek Caban (@jacek) wrote:
Jacek Caban (@jacek) commented about tools/widl/header.c:
fprintf(header, "#ifndef __%s__\n", header_token); fprintf(header, "#define __%s__\n\n", header_token);
+ fprintf(header, "#ifndef __WIDL_INLINE\n"); + fprintf(header, "#if defined(__cplusplus) || defined(_MSC_VER)\n"); + fprintf(header, "#define __WIDL_INLINE static inline\n"); + fprintf(header, "#elif defined(__GNUC__)\n"); + fprintf(header, "#define __WIDL_INLINE static __inline__\n"); + fprintf(header, "#endif\n\n");
Do we really need __WIDL_INLINE? It seems to me that we could just use static inline unconditionally.
For Wine, we can, but "inline" isn't C89, and my understanding is that mingw-w64 would like to maintain c89 compatiblity (and widl is shared with mingw-w64). Cf. [1]. [1] https://sourceforge.net/p/mingw-w64/mailman/message/37685824/ ``` I would agree if it would be essential for generated headers. Since the feature is optional in the first place, someone needing -std=c89 can simply not use those wrappers. Also note that -std=gnu89 is fine, so I don't think it's needed in practice.
Anyway, it's not really a strong opinion, having the macro is not a big deal. However, then I would suggest to move `static` out of it, that is `#define __WIDL_INLINE inline` or `__inline__` and then have wrappers as `static __WIDL_INLINE ...`. Maybe we could also define the macro only when `WIDL_C_INLINE_WRAPPERS` is defined. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/935#note_9413
On 9/27/22 11:24, Jacek Caban (@jacek) wrote:
On Tue Sep 27 15:44:59 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 9/27/22 06:14, Jacek Caban (@jacek) wrote:
Jacek Caban (@jacek) commented about tools/widl/header.c:
fprintf(header, "#ifndef __%s__\n", header_token); fprintf(header, "#define __%s__\n\n", header_token);
+ fprintf(header, "#ifndef __WIDL_INLINE\n"); + fprintf(header, "#if defined(__cplusplus) || defined(_MSC_VER)\n"); + fprintf(header, "#define __WIDL_INLINE static inline\n"); + fprintf(header, "#elif defined(__GNUC__)\n"); + fprintf(header, "#define __WIDL_INLINE static __inline__\n"); + fprintf(header, "#endif\n\n");
Do we really need __WIDL_INLINE? It seems to me that we could just use static inline unconditionally.
For Wine, we can, but "inline" isn't C89, and my understanding is that mingw-w64 would like to maintain c89 compatiblity (and widl is shared with mingw-w64). Cf. [1]. [1] https://sourceforge.net/p/mingw-w64/mailman/message/37685824/ ``` I would agree if it would be essential for generated headers. Since the feature is optional in the first place, someone needing -std=c89 can simply not use those wrappers. Also note that -std=gnu89 is fine, so I don't think it's needed in practice.
Indeed. Both viewpoints seem reasonable to me, but both have been expressed by mingw-w64 maintainers, so in lieu of any agreement I'll just keep the patch as is.
Anyway, it's not really a strong opinion, having the macro is not a big deal. However, then I would suggest to move `static` out of it, that is `#define __WIDL_INLINE inline` or `__inline__` and then have wrappers as `static __WIDL_INLINE ...`. Maybe we could also define the macro only when `WIDL_C_INLINE_WRAPPERS` is defined.
Sure.
On 9/27/22 22:48, Zebediah Figura wrote:
Maybe we could also define the macro only when `WIDL_C_INLINE_WRAPPERS` is defined.
Sure.
It turns out this part is not trivial, since we define WIDL_C_INLINE_WRAPPERS in the middle of the idl in unknwn.idl. Since it seems less ugly to define __WIDL_INLINE near the top always, and there doesn't seem to be any harm in always defining it, I've left that aspect of the patch alone for now.
participants (6)
-
Huw Davies (＠huw) -
Jacek Caban (＠jacek)
-
Rémi Bernon
-
Zebediah Figura
-
Zebediah Figura -
Zebediah Figura (＠zfigura)