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

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.

+#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
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?

+ elog(WARNING, "SymInitialize failed with error %lu", error);

Is there a reason to continue if SymInitialize failed? It should return after
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.

+ DWORD64 address = (DWORD64) (stack[i]);

The parenthesis around stack is superfluous. The code usually doesn't contain
additional parenthesis (unless it improves readability).

+ 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?

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

+ 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?

[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

--
Euler Taveira
EDB https://www.enterprisedb.com/