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
