gdb/amd64-windows: return non-trivial C++ types via sret on MSVC ABI - #253
gdb/amd64-windows: return non-trivial C++ types via sret on MSVC ABI#253czidev-amd wants to merge 1 commit into
Conversation
palves
left a comment
There was a problem hiding this comment.
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 My testing is simple, run the 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 |
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.
The thing is that there is no single the Windows C++ ABI anymore to care about, there are two.
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. |
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. |
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.
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). |
Indeed, I need to guard the patch against Cygwin GCC path, I've missed that! (osabi should show cygwin) |
palves
left a comment
There was a problem hiding this comment.
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.
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. |
The Windows x64 ABI requires that a struct or union be returned via a hidden output pointer (sret, passed in RCX, returned in RAX) rather than in registers if any of the following conditions hold: (a) has a user-defined (non-trivial) copy constructor (b) has a user-defined (non-trivial) destructor (c) has any private or protected non-static data members See https://learn.microsoft.com/en-us/cpp/build/x64-calling-convention. GDB's amd64_windows_return_value() handled none of these. For small types (size 1, 2, 4, or 8 bytes) it fell through to the default: branch and read the return value from RAX, causing finish to read garbage and p func() to crash when the inferior used sret. Conditions (a) and (b) apply to both GCC/MinGW and Clang-MSVC binaries on Windows: GCC also follows the Windows x64 calling convention here. Detect them via GDB's existing language_pass_by_reference() machinery, which inspects DWARF constructor and destructor attributes (DW_AT_defaulted, DW_AT_deleted, DW_AT_artificial). Condition (c) is an additional MSVC-ABI requirement not enforced by GCC/MinGW: GCC ignores member access specifiers for return-value classification. Since PE/COFF binaries carry no record of which compiler's C++ ABI was used, condition (c) is gated on the mangled symbol name: MSVC mangles as '?foo@@...', GNU/Itanium as '_Z...'. When the called function's linkage name starts with '?' the type's fields are walked for any private or protected non-static member. Note: once upstream's GDB_OSABI_WINDOWS_MSVC distinction (Pedro Alves, gdb-patches 2026-07, https://sourceware.org/pipermail/gdb-patches/2026-July/228640.html) is available and the sniffer can reliably distinguish the two Windows ABIs from the binary, the mangled-name heuristic for condition (c) could be replaced by gdbarch_osabi (gdbarch) == GDB_OSABI_WINDOWS_MSVC. Add a new test class G to gdb.cp/non-trivial-retval.exp: G has a private non-static data member and no user-defined copy constructor or destructor, so it is trivially copyable by the C++ standard but must be returned via sret by the MSVC ABI (condition c). This case was not covered by any existing test. Before this patch, the test had 4 failures when compiled with GCC/MinGW and 13 failures when compiled with Clang targeting the MSVC ABI. After this patch, all 22 tests pass for all three compiler configurations tested: GCC/MinGW (UCRT64), Clang with MSVC ABI (--target=x86_64-pc-windows-msvc), and GCC/Cygwin. Signed-off-by: Claudiu Zissulescu-Ianculescu <claudiu.zissulescu-ianculescu@amd.com>
de64341 to
4af7492
Compare
|
A new version that addresses the above comments is pushed. Here it is the new commit message: What is worth to mention: the new G class test is passing in Linux, no regression here. |
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.