gdb/amd64-windows: return non-trivial C++ types via sret on MSVC ABI

[czidev-amd]

The MSVC x64 ABI requires that any struct or union with a non-trivial copy constructor or non-trivial destructor be returned via a hidden output pointer (sret), regardless of the object's size. The callee writes the return value to the address passed in RCX; RAX is not used for the object itself.

GDB's amd64_windows_return_value() did not account for this. For small non-trivially-copyable types (size 1, 2, 4, or 8 bytes) it fell through to the default: branch and read the return value from RAX as if the Itanium ABI were in effect. This caused finish to read garbage, and p func() to crash or return wrong values when the inferior was compiled with clang --target=x86_64-pc-windows-msvc.

The fix queries GDB's existing language_pass_by_reference() machinery, which already inspects DWARF constructor and destructor attributes (DW_AT_defaulted, DW_AT_deleted, DW_AT_artificial) to determine whether a type is trivially copyable and trivially destructible. If either flag is false the function redirects to
RETURN_VALUE_ABI_RETURNS_ADDRESS, reading the result from the address in RAX rather than from RAX directly.

Trivially copyable+destructible structs (plain PODs) are unaffected: they continue to be returned in RAX for sizes 1/2/4/8 bytes, matching the MSVC ABI.

Test Result

Tested using gdb.cp/non-trivial-retval.exp: all 19 tests pass. Broader sanity check over gdb.cp/*.exp shows zero regressions.

[palves]

Isn't this going to break the GNU ABI support? We'll need the WINDOWS_MSVC ABI I'm adding upstream to distinguish, I'd think?

[czidev-amd]

Isn't this going to break the GNU ABI support? We'll need the WINDOWS_MSVC ABI I'm adding upstream to distinguish, I'd think?

I hope it doesn't, my mods are done in the amd64-windows-tdep.c file which, AFAIK, is only triggered when the windows target is required. The patch also relays on the language_pass_by_reference machinery.
As far as I can see MinGW gcc produces a PE32+ executable, which on its turn, triggers the Windows OS ABI.

My testing is simple, run the non-trivial-retval.exp test (make check-gdb TESTS="gdb.cp/non-trivial-retval.exp") with the default board (i.e, GCC), followed by running the same test using clang+DWARF+Microsoft ABI.

For clang I use these options:

exec "$CLANG" --target=x86_64-pc-windows-msvc -fuse-ld=lld-link \
  -Wno-unused-command-line-argument -Wno-deprecated-non-prototype 

N.B. The clang script is a bit more complex and it takes into consideration if one needs debug (i.e., DWARF) output or not.

[palves]

Isn't this going to break the GNU ABI support? We'll need the WINDOWS_MSVC ABI I'm adding upstream to distinguish, I'd think?

I hope it doesn't, my mods are done in the amd64-windows-tdep.c file which, AFAIK, is only triggered when the windows target is required. The patch also relays on the language_pass_by_reference machinery. As far as I can see MinGW gcc produces a PE32+ executable, which on its turn, triggers the Windows OS ABI.

I meant the GNU ABI on Windows, i.e,. MinGW (GNU/Itanium for C++). amd64-windows-tdep.c is used for both MinGW (GNU) and MSVC.

triggers the Windows OS ABI.

The thing is that there is no single the Windows C++ ABI anymore to care about, there are two.

My testing is simple, run the non-trivial-retval.exp test (make check-gdb TESTS="gdb.cp/non-trivial-retval.exp") with the default board (i.e, GCC), followed by running the same test using clang+DWARF+Microsoft ABI.

Does the gdb.cp/non-trivial-retval.exp testcase pass on MinGW, both with and without the patch? How come?

[czidev-amd]

Does the gdb.cp/non-trivial-retval.exp testcase pass on MinGW, both with and without the patch? How come?

It does not, without my patch, there are 4 errors and 15 passes using GCC, and 13 errors and 6 passes while using clang board. You are right, I need to understand better what mingw gcc does.

[czidev-amd]

Does the gdb.cp/non-trivial-retval.exp testcase pass on MinGW, both with and without the patch? How come?

Checking https://gcc.gnu.org/onlinedocs/gcc/x86-Attributes.html, it seems the MSVC ABI is default when targeting Windows ("The default is to use the Microsoft ABI when targeting Windows"). This explains the patch impact when using only GCC board.

[palves]

MSVC ABI is default when targeting Windows ("The default is to use the Microsoft ABI when targeting Windows").

This is not really the full MSVC (C++) ABI, but the Windows ABI calling convention mainly at the C-level. Let's prefer using "MSVC ABI" for when we talk about the C++ ABI more broadly. For instance, GCC's ms_abi uses a different 'long double' from the MSVC ABI. So it's reasonable to assume that there could be other differences, especially when it comes to passing C++ types.

This explains the patch impact when using only GCC board.

But what is the impact? Please be explicit about this. You said earlier that "without my patch, there are 4 errors and 15 passes using GCC", but I still haven't heard the what happens to the GCC results with your patch. What were the tests that were failing with GCC before your patch? Do we get clean test results afterwards?

[czidev-amd]

But what is the impact? Please be explicit about this. You said earlier that "without my patch, there are 4 errors and 15 passes using GCC", but I still haven't heard the what happens to the GCC results with your patch. What were the tests that were failing with GCC before your patch? Do we get clean test results afterwards?

With the patch both mingw gcc and clang are passing (19 passes/no error/clean run for non-trivial-retval.exp).

[czidev-amd]

MSVC ABI is default when targeting Windows ("The default is to use the Microsoft ABI when targeting Windows").

This is not really the full MSVC (C++) ABI, but the Windows ABI calling convention mainly at the C-level. Let's prefer using "MSVC ABI" for when we talk about the C++ ABI more broadly. For instance, GCC's ms_abi uses a different 'long double' from the MSVC ABI. So it's reasonable to assume that there could be other differences, especially when it comes to passing C++ types.

Indeed, I need to guard the patch against Cygwin GCC path, I've missed that! (osabi should show cygwin)

[palves]

In the subject / commit log:

gdb/amd64-windows: return non-trivial C++ types via sret on MSVC ABI

OK, we've established this is about the Windows x64 ABI, not the MSVC C++ specifics. Please let's just drop the "on MSVC ABI" part to avoid confusion, the gdb/amd64-windows part already scopes this to Windows. MSVC is the Visual C (and C++) compiler.

The MSVC x64 ABI requires that any struct or union with a non-trivial

MSVC => Windows.

copy constructor or non-trivial destructor be returned via a hidden
output pointer (sret), regardless of the object's size. The callee
writes the return value to the address passed in RCX; RAX is not used
for the object itself.

A reference would be useful here. I assume this is it?

https://learn.microsoft.com/en-us/cpp/build/x64-calling-convention

It says:

"This definition is essentially the same as a C++03 POD type. Because the definition has changed in the C++11 standard, we don't recommend using std::is_pod for this test. "

GDB's amd64_windows_return_value() did not account for this. For
small non-trivially-copyable types (size 1, 2, 4, or 8 bytes) it fell
through to the default: branch and read the return value from RAX as
if the Itanium ABI were in effect. This caused finish to read
garbage, and p func() to crash or return wrong values when the
inferior was compiled with clang --target=x86_64-pc-windows-msvc.

We've now established this fixes things for GCC/MinGW too, so this needs some updating or generalizing. This mention of x86_64-pc-windows-msvc is part of what made me wonder what happens with GCC and whether we'd be breaking things there.

The fix queries GDB's existing language_pass_by_reference() machinery,
which already inspects DWARF constructor and destructor attributes
(DW_AT_defaulted, DW_AT_deleted, DW_AT_artificial) to determine
whether a type is trivially copyable and trivially destructible.

So this ends up consulting gnuv3_pass_by_reference, i.e., the Itanium ABI. It just happens to do the right thing here. gnu-v2-abi.c does not implement pass_by_reference, so there this would return the wrong thing. Eventually we'll need a separate msvc-abi.c file instead of abusing gnu-v3-abi.c. But this gnuv3_pass_by_reference function, and its callees, it really does look like code that probably all works on MSVC ABI too, maybe with some minor tweaks here and there. This suggests to me that gnuv3_pass_by_reference should be factored out to language.c, probably. Also, we can probably get rid of gnu-v2 nowadays. I doubt anyone cares about that. Anyhow, for now, we can continue using it like it is.

Does the patch really get everything right x64 calling-convention-wise? There's also this note in the MS doc:

"It can have no private or protected nonstatic data members"

for example, and I don't see such a check in the patch. Are we missing such tests?

If
either flag is false the function redirects to
RETURN_VALUE_ABI_RETURNS_ADDRESS, reading the result from the address
in RAX rather than from RAX directly.

Trivially copyable+destructible structs (plain PODs) are unaffected:
they continue to be returned in RAX for sizes 1/2/4/8 bytes, matching
the MSVC ABI.

Tested using gdb.cp/non-trivial-retval.exp: all 19 tests pass on both
boards.

Should mention the failures before the patch. That there were failures on GCC is totally non-obvious, as the whole commit log is written in terms of MSVC.

Broader sanity check over gdb.cp/*.exp shows zero regressions.

[palves]

Indeed, I need to guard the patch against Cygwin GCC path, I've missed that! (osabi should show cygwin)

Cygwin defaults to ms_abi, too. But in any case, it'll be great if you setup for Cygwin testing too.

[czidev-amd]

Indeed, I need to guard the patch against Cygwin GCC path, I've missed that! (osabi should show cygwin)

Cygwin defaults to ms_abi, too. But in any case, it'll be great if you setup for Cygwin testing too.

Did that, the same output like ucrt64 gcc: before the patch 4 out of 19 failures, after the patch everything passes.