[PATCH 0/1] MR935: widl: Generate "static inline" instead of "static FORCEINLINE" for COM inline wrappers.

[Zebediah Figura (＠zfigura)]

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.

[PATCH 1/1] widl: Generate "static inline" instead of "static FORCEINLINE" for COM inline wrappers.

[Zebediah Figura]

From: Zebediah Figura

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);

[Rémi Bernon (＠rbernon)]

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.

[Huw Davies (＠huw)]

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).

[Jacek Caban (＠jacek)]

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.

[Zebediah Figura]

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)]

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.

[Zebediah Figura]

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/

[Jacek Caban (＠jacek)]

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.

[Zebediah Figura]

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.

[Zebediah Figura]

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.