Message-ID: <c04e5ca9-d1e1-48a6-acf3-0fd42f103ac0@gmail.com>
Date: Fri, 31 Oct 2025 14:07:18 -0600
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Add Windows support for backtrace_functions (MSVC only)
To: Euler Taveira <euler@eulerto.com>,
 Jakub Wartak <jakub.wartak@enterprisedb.com>
Cc: =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>,
 Michael Paquier <michael@paquier.xyz>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
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>
Content-Language: en-US
From: Bryan Green <dbryan.green@gmail.com>
In-Reply-To: <2241662b-da7d-428f-beba-a416ef48e9e9@app.fastmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/c04e5ca9-d1e1-48a6-acf3-0fd42f103ac0%40gmail.com>
Precedence: bulk

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