From: Bryan Green <dbryan.green@gmail.com>
To: Euler Taveira <euler@eulerto.com>
To: Jakub Wartak <jakub.wartak@enterprisedb.com>
Cc: Álvaro Herrera <alvherre@kurilemu.de>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Add Windows support for backtrace_functions (MSVC only)
Date: Fri, 31 Oct 2025 14:07:18 -0600
Message-ID: <c04e5ca9-d1e1-48a6-acf3-0fd42f103ac0@gmail.com> (raw)
In-Reply-To: <2241662b-da7d-428f-beba-a416ef48e9e9@app.fastmail.com>
References: <202510300932.isdvgkzqzshk@alvherre.pgsql>
<5b4c6ce0-ed17-4abb-9127-859cc95cbdf3@gmail.com>
<CAKZiRmzuqPmgd_A+M+RiuT-4UtDt_TASqQJoqCdvCUrVY3w7cg@mail.gmail.com>
<30aea073-109c-43cc-979c-081b922d055d@gmail.com>
<2241662b-da7d-428f-beba-a416ef48e9e9@app.fastmail.com>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: dbryan.green@gmail.com, euler@eulerto.com, jakub.wartak@enterprisedb.com, alvherre@kurilemu.de, michael@paquier.xyz
Subject: Re: [PATCH] Add Windows support for backtrace_functions (MSVC only)
In-Reply-To: <c04e5ca9-d1e1-48a6-acf3-0fd42f103ac0@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox