Re: [PATCH] widl: Handle C++ aggregate returns in a MSVC compatible way.
Hi Henri, On 20.07.2017 14:18, Henri Verbeet wrote:
This mainly affects 64-bit winelib applications, and potentially mingw-w64 usage of the Wine headers. As explained in commit fabfa59aea5c5b3201142382038beb3e76cb7567, MSVC returns aggregates through an implicit parameter immediately after the this/interface pointer. GCC's "ms_abi" attribute is supposed to match this, but unfortunately current versions of g++ (confirmed up to at least g++ 6.3) place the implicit return pointer before the this/interface pointer. See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52792.
FWIW, here is a bug about 32-bit problem: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64384
This commit takes an approach similar to the earlier commit fabfa59aea5c5b3201142382038beb3e76cb7567: For ABI compatibility the return pointer is listed as an explicit parameter in the vtbl entry, while an inline helper is provided for source code compatibility.
While it fixes the problem for C++ callers, it breaks building classes implementing those interfaces. Implementation will try to override inline function instead of virtual one. Given that the workaround has bad side effects, I'd say that we should at least try harder to not use it. For example it could be #ifdefed to not be used by compilers doing the right thing. With that, using our headers would require modifications in compiled code compatible with MSVC, so there would be a point in having a nice to use macro that interface implementations could use to decide if they should apply needed changes. Obviously fixing compilers sounds much better. Jacek
On 20 July 2017 at 15:00, Jacek Caban <jacek(a)codeweavers.com> wrote:
This commit takes an approach similar to the earlier commit fabfa59aea5c5b3201142382038beb3e76cb7567: For ABI compatibility the return pointer is listed as an explicit parameter in the vtbl entry, while an inline helper is provided for source code compatibility.
While it fixes the problem for C++ callers, it breaks building classes implementing those interfaces. Implementation will try to override inline function instead of virtual one. I does mean C++ implementations would need to explicitly list the return pointer as well, yes. I'm not aware of a way around that, aside from using a fixed compiler.
Given that the workaround has bad side effects, I'd say that we should at least try harder to not use it. For example it could be #ifdefed to not be used by compilers doing the right thing. That's fine with me in principle, but see below. It also implies writing detection code for the bug.
With that, using our headers would require modifications in compiled code compatible with MSVC, so there would be a point in having a nice to use macro that interface implementations could use to decide if they should apply needed changes. But if you have to make (minor) modifications to implementations, is that really any better than just explicitly listing the return pointer in the implementation? (Regardless of how easy/hard/ugly such a macro would need to be.)
On 20.07.2017 15:32, Henri Verbeet wrote:
On 20 July 2017 at 15:00, Jacek Caban <jacek(a)codeweavers.com> wrote:
This commit takes an approach similar to the earlier commit fabfa59aea5c5b3201142382038beb3e76cb7567: For ABI compatibility the return pointer is listed as an explicit parameter in the vtbl entry, while an inline helper is provided for source code compatibility. While it fixes the problem for C++ callers, it breaks building classes implementing those interfaces. Implementation will try to override inline function instead of virtual one. I does mean C++ implementations would need to explicitly list the return pointer as well, yes. I'm not aware of a way around that, aside from using a fixed compiler.
Agreed.
Given that the workaround has bad side effects, I'd say that we should at least try harder to not use it. For example it could be #ifdefed to not be used by compilers doing the right thing. That's fine with me in principle, but see below. It also implies writing detection code for the bug.
Yes, proper detection code would be a problem. However I would expect something like following (could be emitted in header prologue) to be accurate for recent compilers and allow overriding settings with compiler arguments: #ifndef WIDL_EXPLICIT_AGGREGATED_RETURN #if defined(__GNUC__) && !defined(__clang__) #define WIDL_EXPLICIT_AGGREGATED_RETURN 1 #else #define WIDL_EXPLICIT_AGGREGATED_RETURN 0 #endif #endif That said, I don't really like it. I'd be happier to see a better solution, but if we have none, it's better than nothing.
With that, using our headers would require modifications in compiled code compatible with MSVC, so there would be a point in having a nice to use macro that interface implementations could use to decide if they should apply needed changes. But if you have to make (minor) modifications to implementations, is that really any better than just explicitly listing the return pointer in the implementation? (Regardless of how easy/hard/ugly such a macro would need to be.)
You can't unconditionally do that if you want to keep sources compatible with MSVC. What I meant is that such code could use WIDL_EXPLICIT_AGGREGATED_RETURN for required #ifdefs (just like we can use it to generate declarations both with and without the workaround). Jacek
On 20 July 2017 at 15:55, Jacek Caban <jacek(a)codeweavers.com> wrote:
With that, using our headers would require modifications in compiled code compatible with MSVC, so there would be a point in having a nice to use macro that interface implementations could use to decide if they should apply needed changes. But if you have to make (minor) modifications to implementations, is that really any better than just explicitly listing the return pointer in the implementation? (Regardless of how easy/hard/ugly such a macro would need to be.)
You can't unconditionally do that if you want to keep sources compatible with MSVC. It's not really a property of the compiler though. An application using the Wine headers should compile correctly with MSVC as well. On the other hand, it does imply an incompatibility with the PSDK headers. Unfortunately no amount of macros we add is going to avoid that, they simply wouldn't be available when using the PSDK headers.
What I meant is that such code could use WIDL_EXPLICIT_AGGREGATED_RETURN for required #ifdefs (just like we can use it to generate declarations both with and without the workaround).
I'm fine with hiding these behind WIDL_EXPLICIT_AGGREGATED_RETURN, although I'd prefer WIDL_EXPLICIT_AGGREGATE_RETURNS. I'm less convinced about automatically enabling or disabling that based on the compiler used. I think I'd rather have the user of the header explicitly enable them, not unlike how WIDL_C_INLINE_WRAPPERS works.
participants (2)
-
Henri Verbeet -
Jacek Caban