Re: [PATCH] Add Windows support for backtrace_functions (MSVC only)

On 10/31/2025 2:07 PM, Bryan Green wrote:
> On 10/31/2025 1:46 PM, Euler Taveira wrote:
>> On Thu, Oct 30, 2025, at 11:51 AM, Bryan Green wrote:
>>> I had reservations about the value the tests were adding, and
>>> considering I am getting more concern around having the tests than not
>>> having them for this initial release I have decided to remove them.  v4
>>> patch is attached.  It is the same as the initial 0001-* patch.
>>>
>>
>> I spent some time on this patch. Here are some comments and suggestions.
>>
> 
> Thanks for the review.
> 
>> +#ifdef _MSC_VER
>> +#include <windows.h>
>> +#include <dbghelp.h>
>> +static bool win32_backtrace_symbols_initialized = false;
>> +static HANDLE win32_backtrace_process = NULL;
>> +#endif
>>
>> We usually a different style. Headers go to the top on the same 
>> section as
>> system headers and below postgres.h. It is generally kept sorted. The 
>> same
> 
> Will fix. I will rework the style (error and otherwise) to follow the 
> project tradition.
> 
>> applies to variables. Add them near the other static variables. BTW 
>> does it need
>> the win32_ prefix for a Window-only variable?
>>
>> +        wchar_t        buffer[sizeof(SYMBOL_INFOW) + MAX_SYM_NAME * 
>> sizeof(wchar_t)];
>> +        PSYMBOL_INFOW symbol;
>>
>> According to [1], SYMBOL_INFO is an alias that automatically selects 
>> ANSI vs
>> UNICODE. Shouldn't we use it instead of SYMBOL_INFOW?
>>
> 
> Good point. I was being overly explicit about wanting wide chars, but 
> you're right that the generic versions are the way to go.
> 
>> +                elog(WARNING, "SymInitialize failed with error %lu", 
>> error);
>>
>> Is there a reason to continue if SymInitialize failed? It should 
>> return after
> 
> None at all.  Will return immediately.
> 
>> printing the message. Per the error message style guide [2], my 
>> suggestion is
>> "could not initialize the symbol handler: error code %lu". You can 
>> also use
>> GetLastError() directly in the elog call.
>>
>> +        symbol = (PSYMBOL_INFOW) buffer;
>> +        symbol      ->MaxNameLen = MAX_SYM_NAME;
>> +        symbol      ->SizeOfStruct = sizeof(SYMBOL_INFOW);
>>
>> We generally don't add spaces between variable and a member.
>>
>> +        DWORD        i;
>>
>> I'm curious why did you declare this variable as DWORD? Shouldn't int be
>> sufficient? The CaptureStackBackTrace function returns an unsigned short
>> (UShort). You can also declare it in the for loop.
>>
> 
> Out of habit.  I will change it to int.
> 
>> +            DWORD64        address = (DWORD64) (stack[i]);
>>
>> The parenthesis around stack is superfluous. The code usually doesn't 
>> contain
>> additional parenthesis (unless it improves readability).
>>
> 
> I will remove the parenthesis.
> 
>> +        if (frames == 0)
>> +        {
>> +            appendStringInfoString(&errtrace, "\nNo stack frames 
>> captured");
>> +            edata->backtrace = errtrace.data;
>> +            return;
>> +        }
>>
>> It seems CaptureStackBackTrace function may return zero frames under 
>> certain
>> conditions. It is a good point having this additional message. I 
>> noticed that
>> the current code path (HAVE_BACKTRACE_SYMBOLS) doesn't have this 
>> block. IIUC,
>> in certain circumstances (ARM vs unwind-tables flag), the backtrace() 
>> also
>> returns zero frames. Should we add this block for the backtrace() code 
>> path?
>>
> 
> Probably, though that seems like separate cleanup. Want me to include it
> here or handle separately (in another patch)?
> 
>> +            sym_result = SymFromAddrW(win32_backtrace_process,
>> +                                      address,
>> +                                      &displacement,
>> +                                      symbol);
>>
>> You should use SymFromAddr, no? [3] I saw that you used the Unicode 
>> functions
>> instead of the generic functions [4].
>>
>> +                    /* Convert symbol name to UTF-8 */
>> +                    utf8_len = WideCharToMultiByte(CP_UTF8, 0, 
>> symbol->Name, -1,
>> +                                                   NULL, 0, NULL, NULL);
>> +                    if (utf8_len > 0)
>> +                    {
>> +                        char       *filename_utf8;
>> +                        int            filename_len;
>> +
>> +                        utf8_buffer = palloc(utf8_len);
>> +                        WideCharToMultiByte(CP_UTF8, 0, symbol->Name, 
>> -1,
>> +                                            utf8_buffer, utf8_len, 
>> NULL, NULL);
>>
>> +                    /* Convert symbol name to UTF-8 */
>> +                    utf8_len = WideCharToMultiByte(CP_UTF8, 0, 
>> symbol->Name, -1,
>> +                                                   NULL, 0, NULL, NULL);
>>
>> You are calling WideCharToMultiByte twice. The reason is to allocate 
>> the exact
>> memory size. However, you can adopt another logic to avoid the first 
>> call.
>>
>> maxlen = symbol->NameLen * pg_database_encoding_max_length();
>> symbol_name = palloc(maxlen + 1);
>>
>> (I suggest symbol_name instead of ut8_buffer.)
>>
>> You are considering only the UTF-8 case. Shouldn't it use wcstombs or
>> wcstombs_l? Maybe (re)use wchar2char -- see pg_locale_libc.c.
>>
> 
> Hmm, you're probably right. I was thinking these Windows API strings 
> needed special handling, but wchar2char should handle the conversion to 
> database encoding correctly. Let me test that approach.
> 
>> +                        if (filename_len > 0)
>> +                        {
>> +                            filename_utf8 = palloc(filename_len);
>> +                            WideCharToMultiByte(CP_UTF8, 0, 
>> line.FileName, -1,
>> +                                                filename_utf8, 
>> filename_len,
>> +                                                NULL, NULL);
>> +
>> +                            appendStringInfo(&errtrace,
>> +                                             "\n%s+0x%llx [%s:%lu]",
>> +                                             utf8_buffer,
>> +                                             (unsigned long long) 
>> displacement,
>> +                                             filename_utf8,
>> +                                             (unsigned long) 
>> line.LineNumber);
>> +
>> +                            pfree(filename_utf8);
>> +                        }
>> +                        else
>> +                        {
>> +                            appendStringInfo(&errtrace,
>> +                                             "\n%s+0x%llx [0x%llx]",
>> +                                             utf8_buffer,
>> +                                             (unsigned long long)
>> displacement,
>> +                                             (unsigned long long) 
>> address);
>> +                        }
>>
>> Maybe I missed something but is there a reason for not adding the 
>> address in the
>> first condition?
>>
> 
> No particular reason. I was trying to keep it concise when we have file/ 
> line info, but for consistency it probably should be there.
> 
> Will send v5 with these fixes.
> 
>>
>> [1] https://learn.microsoft.com/en-us/windows/win32/api/dbghelp/ns- 
>> dbghelp-symbol_infow
>> [2] https://www.postgresql.org/docs/current/error-style-guide.html
>> [3] https://learn.microsoft.com/en-us/windows/win32/api/dbghelp/nf- 
>> dbghelp-symfromaddrw
>> [4] https://learn.microsoft.com/en-us/windows/win32/intl/conventions- 
>> for-function-prototypes
>>
>>
> 
> Thanks again for the review,
> Bryan
Hi all,

v5 patch attached, incorporating all of Euler's feedback with a caveat 
around unicode.

The most interesting aspect turned out to be the encoding conversion for
symbol names and file paths. Initially I tried using the generic 
SYMBOL_INFO and SymFromAddr functions as Euler suggested, but ran into a 
subtle issue: on PostgreSQL's Windows builds, these become SYMBOL_INFOA 
and SymFromAddrA (the ANSI versions), which return strings in whatever 
Windows ANSI codepage happens to be active (CP1252, etc). This doesn't 
necessarily match the database encoding.

When I tried converting these with wchar2char(), it failed because the
input wasn't actually wide characters - leading to backtraces showing 
only raw addresses even though symbols were present.

The solution was to use the explicit Unicode versions (SYMBOL_INFOW and
SymFromAddrW), which reliably return UTF-16 strings that wchar2char() 
can properly convert to the database encoding. This handles both UTF-8 
and non-UTF-8 databases correctly, and wchar2char() gracefully returns 
-1 on conversion failure rather than throwing errors during error 
handling. Of course this also necessitated using IMAGEHLP_LINEW64 and 
SymGetLineFromAddrW64.

Tested with both UTF-8 and WIN1252 databases - backtraces now show 
proper symbol names in both cases.

This patch also adds a check for zero frames returned by backtrace() on
Unix/Linux platforms, which can occur in certain circumstances such as 
ARM builds without unwind tables.

Bryan