Implement SymbolInfo::getFilename() on Win32 #62577

grynspan · 2022-12-14T15:03:32Z

Here we are again! This change implements SymbolInfo::getFilename() on Win32. It lazily loads the string the first time somebody requests it (since most callers don't ask for it.) It is necessary to change call sites because they currently assume the string is immortal, which can lead to a use-after-free scenario on Windows. So callers must now copy the string if they keep it longer than the calling scope.

As discussed in my last PR, I reviewed my use of auto with SymbolInfo. I changed uses of it when getting fields so the type is explicit, but I left it in for the return type of SymbolInfo::lookup() because, IMHO, it's clear what the type will be in that context, and typing out llvm::Optional<SymbolInfo> consumes a significant portion of our 80-column limit.

Because the filename may include Unicode characters, I need to convert from UTF-16 (wchar_t on Windows) to UTF-8 (char.) So I moved the copyUTF8FromWide() function out from CommandLine.cpp into its own file in the runtime and reused it in SymbolInfo.cpp.

This change also stores the input address to SymbolInfo::lookup() and makes it accessible via getAddress()—I have grand plans for it later!

grynspan · 2022-12-14T15:03:42Z

@swift-ci please test

mikeash

Two tiny nits, looks good overall.

include/swift/Runtime/Win32Strings.h

stdlib/private/SwiftReflectionTest/SwiftReflectionTest.swift

include/swift/Runtime/Win32Strings.h

stdlib/public/runtime/Errors.cpp

stdlib/private/SwiftReflectionTest/SwiftReflectionTest.swift

stdlib/public/runtime/SymbolInfo.cpp

include/swift/Runtime/Win32Strings.h

stdlib/public/runtime/Win32Strings.cpp

grynspan · 2022-12-14T18:48:14Z

@swift-ci please test

grynspan · 2022-12-14T20:19:43Z

@swift-ci please test

grynspan · 2022-12-14T21:36:06Z

@swift-ci please test windows

grynspan · 2022-12-14T23:36:15Z

@swift-ci please test windows

grynspan · 2022-12-15T02:30:37Z

@swift-ci please clean build windows

grynspan · 2022-12-15T02:31:06Z

Blocked by swiftlang/llvm-project#5792.

al45tair · 2022-12-15T09:18:52Z

So I moved the copyUTF8FromWide() function out from CommandLine.cpp into its own file in the runtime and reused it in SymbolInfo.cpp.

Hmmm. See Win32.cpp/Win32.h in #62462. I need the same code there too (I didn't remove the copy from CommandLine.cpp however, but I probably should have.

grynspan · 2022-12-15T12:22:01Z

So I moved the copyUTF8FromWide() function out from CommandLine.cpp into its own file in the runtime and reused it in SymbolInfo.cpp.

Hmmm. See Win32.cpp/Win32.h in #62462. I need the same code there too (I didn't remove the copy from CommandLine.cpp however, but I probably should have.

This change is just about to land—shall we coordinate here?

grynspan · 2022-12-15T12:31:13Z

Waiting for Alastair to land some overlapping changes.

grynspan · 2022-12-15T12:33:01Z

@swift-ci please test

stdlib/public/runtime/SymbolInfo.cpp

CodaFi

I'm not much of a Windows expert, so I can only offer my 👍 on the structure and defensiveness of the calls into and out of Win32

grynspan · 2023-01-06T02:19:31Z

@al45tair Am I good to get back to this? :)

al45tair · 2023-01-06T15:37:06Z

Think so, yes.

grynspan · 2023-01-06T17:29:51Z

@swift-ci please test

grynspan · 2023-01-07T05:03:20Z

@swift-ci please test

compnerd · 2023-01-07T16:22:49Z

test/Runtime/loaded_image_uniqueness.swift

+    defer {
+      _freeMetadataSectionName(cName)
+    }
+    let name = cName.flatMap { String(validatingUTF8: $0) } ?? "<unavailable>"


Nit:<unknown> might be better to match what we do for symbols which we cannot symbolize?

compnerd · 2023-01-07T16:24:08Z

stdlib/public/runtime/Win32.cpp

+#pragma mark - DbgHelp library thread-safety
+
+static LazyMutex dbgHelpMutex;
+static HANDLE dbgHelpHandle = nullptr;


Can we use INVALID_HANDLE_VALUE instead of nullptr? It seems more appropriate even though it may make the checked condition more verbose.

stdlib/public/runtime/Win32.cpp

compnerd · 2023-01-07T16:28:55Z

stdlib/public/runtime/SymbolInfo.cpp

+    return nullptr;
+  }
+#if defined(_WIN32) && !defined(__CYGWIN__)
+  return PathFindFileNameA(imagePath);


I worry about this as technically pszImagePath is limited to MAX_PATH, which may or may not hold, particularly in the Swift world where we go through efforts to use NT style paths to avoid the length limitations. A far less elegant solution which may be safer (I haven't thought hard enough about it) is to use PathCchRemoveFileSpec and then use that to compute the offset into the buffer to compute the "file spec" (why Microsoft does not provide a PathCchGetFileSpec is unclear to me).

Indeed, when it says that's the limit, it's probably not kidding; most likely PathFindFileNameA has a buffer of that size that it uses to convert from ANSI to UTF-16.

Which also brings up the related point that this function will assume ANSI encoding and might produce unusual results if passed UTF-8, which is what we currently have.

Easiest would be to just find the last slash and truncate, same as what we do on Darwin/Linux.

Remembering that both slashes and backslashes are valid path separators on Windows in most places (aside from \\?\ wide-string prefixed paths and NT-style paths, the latter of which you can't use anyway).

compnerd · 2023-01-07T16:32:47Z

stdlib/public/runtime/Errors.cpp

-  libraryName = libraryName.substr(libraryName.rfind('/')).substr(1);
+  const char *libraryName = syminfo->getImageFilename();
+  if (!libraryName) {
+    libraryName = "<unavailable>";


Nit: <unknown> might be better to match what we do with functions which we cannot symbolize.

grynspan · 2023-04-27T13:38:24Z

@al45tair Does this still make sense to complete given your backtrace work?

al45tair · 2023-04-27T21:14:06Z

It'll be a little while before I get it all working on Windows, so if it's useful to fix this in the meantime I say go for it.

grynspan · 2025-01-07T18:35:01Z

This branch is stale at this point. Closing.

grynspan requested review from compnerd, mikeash, CodaFi, al45tair and Azoy December 14, 2022 15:03

mikeash approved these changes Dec 14, 2022

View reviewed changes

include/swift/Runtime/Win32Strings.h Outdated Show resolved Hide resolved

stdlib/private/SwiftReflectionTest/SwiftReflectionTest.swift Outdated Show resolved Hide resolved

compnerd reviewed Dec 14, 2022

View reviewed changes

grynspan requested review from compnerd and mikeash December 14, 2022 16:46

mikeash reviewed Dec 14, 2022

View reviewed changes

include/swift/Runtime/Win32Strings.h Outdated Show resolved Hide resolved

compnerd approved these changes Dec 14, 2022

View reviewed changes

stdlib/public/runtime/Win32Strings.cpp Outdated Show resolved Hide resolved

grynspan force-pushed the jgrynspan/add-symbolinfo-filename-to-win32 branch from 30b7be5 to 29dc84a Compare December 14, 2022 18:47

grynspan force-pushed the jgrynspan/add-symbolinfo-filename-to-win32 branch from 9a7fd5c to 9ef116a Compare December 15, 2022 12:32

al45tair mentioned this pull request Dec 15, 2022

Centralise code to map between UTF-8 and UTF-16 on Windows. #62605

Merged

al45tair reviewed Dec 15, 2022

View reviewed changes

stdlib/public/runtime/SymbolInfo.cpp Outdated Show resolved Hide resolved

CodaFi approved these changes Jan 4, 2023

View reviewed changes

Implement SymbolInfo::getFilename() on Win32

84ab54a

grynspan force-pushed the jgrynspan/add-symbolinfo-filename-to-win32 branch from 9ef116a to 84ab54a Compare January 6, 2023 17:29

grynspan requested review from al45tair and compnerd January 6, 2023 17:29

GetMappedFileNameW() wants a non-const input pointer

d0dd958

compnerd reviewed Jan 7, 2023

View reviewed changes

grynspan closed this Jan 7, 2025

Implement SymbolInfo::getFilename() on Win32 #62577

Implement SymbolInfo::getFilename() on Win32 #62577

Uh oh!

Conversation

grynspan commented Dec 14, 2022

Uh oh!

grynspan commented Dec 14, 2022

Uh oh!

mikeash left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grynspan commented Dec 14, 2022

Uh oh!

grynspan commented Dec 14, 2022

Uh oh!

grynspan commented Dec 14, 2022

Uh oh!

grynspan commented Dec 14, 2022

Uh oh!

grynspan commented Dec 15, 2022

Uh oh!

grynspan commented Dec 15, 2022

Uh oh!

al45tair commented Dec 15, 2022

Uh oh!

grynspan commented Dec 15, 2022

Uh oh!

grynspan commented Dec 15, 2022

Uh oh!

grynspan commented Dec 15, 2022

Uh oh!

Uh oh!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

grynspan commented Jan 6, 2023

Uh oh!

al45tair commented Jan 6, 2023

Uh oh!

grynspan commented Jan 6, 2023

Uh oh!

grynspan commented Jan 7, 2023

Uh oh!

compnerd Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

compnerd Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

al45tair Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

grynspan Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

al45tair Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

grynspan commented Apr 27, 2023

Uh oh!

al45tair commented Apr 27, 2023

Uh oh!

grynspan commented Jan 7, 2025

Uh oh!