Address feedback

Author: grynspan

include/swift/Runtime/Win32Strings.h

@@ -25,7 +25,7 @@ namespace swift {
 /// @param str The string to convert.
 ///
 /// @returns The string, converted to UTF-8. The caller is responsible for
-///   freeing this string when done with it.
+///   freeing this string with @c free() when done with it.
 ///
 /// If @a str cannot be converted to UTF-8, @c nullptr is returned.
 SWIFT_RUNTIME_STDLIB_INTERNAL

stdlib/private/SwiftReflectionTest/SwiftReflectionTest.swift

@@ -142,6 +142,11 @@ internal func _getMetadataSectionCount() -> UInt
 internal func _copyMetadataSectionName(
   _ metadata_section: UnsafeRawPointer
 ) -> UnsafeMutablePointer<CChar>?
+
+@_silgen_name("swift_freeMetadataSectionName")
+internal func _freeMetadataSectionName(
+  _ name: UnsafeMutablePointer<CChar>
+) -> Void
 #endif
 
 extension Section {
@@ -156,7 +161,7 @@ internal func getReflectionInfoForImage(atIndex i: UInt32) -> ReflectionInfo? {
   return _getMetadataSection(UInt(i)).map { rawPointer in
     let cName = _copyMetadataSectionName(rawPointer)
     defer {
-      cName.deallocate()
+      _freeMetadataSectionName(cName)
     }
     let name = cName.flatMap { String(validatingUTF8: $0) } ?? "<unavailable>"
     let metadataSection = rawPointer.bindMemory(to: MetadataSections.self, capacity: 1).pointee

stdlib/public/runtime/Errors.cpp

@@ -78,8 +78,7 @@ enum: uint32_t {
 using namespace swift;
 
 #if SWIFT_STDLIB_SUPPORTS_BACKTRACE_REPORTING && SWIFT_STDLIB_HAS_DLADDR
-static bool getSymbolNameAddr(llvm::StringRef libraryName,
-                              const SymbolInfo &syminfo,
+static bool getSymbolNameAddr(const SymbolInfo &syminfo,
                               std::string &symbolName, uintptr_t &addrOut) {
   // If we failed to find a symbol and thus dlinfo->dli_sname is nullptr, we
   // need to use the hex address.
@@ -153,10 +152,9 @@ void swift::dumpStackTraceEntry(unsigned index, void *framePC,
   // library name here. Avoid using StringRef::rsplit because its definition
   // is not provided in the header so that it requires linking with
   // libSupport.a.
-  llvm::StringRef libraryName{"<unavailable>"};
-  if (const char *filename = syminfo->getFilename()) {
-    libraryName = filename;
-    libraryName = libraryName.substr(libraryName.rfind('/')).substr(1);
+  const char *libraryName = syminfo->getImageFilename();
+  if (!libraryName) {
+    libraryName = "<unavailable>";
   }
 
   // Next we get the symbol name that we are going to use in our backtrace.
@@ -166,12 +164,12 @@ void swift::dumpStackTraceEntry(unsigned index, void *framePC,
   // we just get HexAddr + 0.
   uintptr_t symbolAddr = uintptr_t(framePC);
   bool foundSymbol =
-      getSymbolNameAddr(libraryName, syminfo.value(), symbolName, symbolAddr);
+      getSymbolNameAddr(syminfo.value(), symbolName, symbolAddr);
   ptrdiff_t offset = 0;
   if (foundSymbol) {
     offset = ptrdiff_t(uintptr_t(framePC) - symbolAddr);
   } else {
-    const void *baseAddress = syminfo->getBaseAddress();
+    const void *baseAddress = syminfo->getImageBaseAddress();
     offset = ptrdiff_t(uintptr_t(framePC) - uintptr_t(baseAddress));
     symbolAddr = uintptr_t(framePC);
     symbolName = "<unavailable>";
@@ -185,12 +183,11 @@ void swift::dumpStackTraceEntry(unsigned index, void *framePC,
   // This gives enough info to reconstruct identical debugging target after
   // this process terminates.
   if (shortOutput) {
-    fprintf(stderr, "%s`%s + %td", libraryName.data(), symbolName.c_str(),
-            offset);
+    fprintf(stderr, "%s`%s + %td", libraryName, symbolName.c_str(), offset);
   } else {
     constexpr const char *format = "%-4u %-34s 0x%0.16" PRIxPTR " %s + %td\n";
-    fprintf(stderr, format, index, libraryName.data(), symbolAddr,
-            symbolName.c_str(), offset);
+    fprintf(stderr, format, index, libraryName, symbolAddr, symbolName.c_str(),
+            offset);
   }
 #else
   if (shortOutput) {

stdlib/public/runtime/ImageInspectionCommon.cpp

@@ -64,10 +64,10 @@ static void fixupMetadataSectionBaseAddress(swift::MetadataSections *sections) {
   if (fixupNeeded) {
     // We need to fix up the base address. We'll need a known-good address in
     // the same image: `sections` itself will work nicely.
-    auto symbolInfo = SymbolInfo::lookup(sections);
-    if (symbolInfo.has_value() && symbolInfo->getBaseAddress()) {
-        sections->baseAddress.store(symbolInfo->getBaseAddress(),
-                                    std::memory_order_relaxed);
+    if (auto symbolInfo = SymbolInfo::lookup(sections)) {
+      if (const void *baseAddress = symbolInfo->getImageBaseAddress()) {
+        sections->baseAddress.store(baseAddress, std::memory_order_relaxed);
+      }
     }
   }
 }
@@ -190,19 +190,24 @@ const swift::MetadataSections *swift_getMetadataSection(size_t index) {
 SWIFT_RUNTIME_EXPORT
 char *swift_copyMetadataSectionName(const swift::MetadataSections *section) {
   if (auto info = SymbolInfo::lookup(section)) {
-    if (info->getFilename()) {
-      return strdup(info->getFilename());
+    if (const char *imagePath = info->getImagePath()) {
+      return strdup(imagePath);
     }
   }
   return "";
 }
 
+SWIFT_RUNTIME_EXPORT
+void swift_freeMetadataSectionName(char *name) {
+  free(name);
+}
+
 SWIFT_RUNTIME_EXPORT
 void swift_getMetadataSectionBaseAddress(const swift::MetadataSections *section,
                                          void const **out_actual,
                                          void const **out_expected) {
   if (auto info = SymbolInfo::lookup(section)) {
-    *out_actual = info->getBaseAddress();
+    *out_actual = info->getImageBaseAddress();
   } else {
     *out_actual = nullptr;
   }

stdlib/public/runtime/ImageInspectionCommon.h

@@ -89,6 +89,9 @@ SWIFT_RUNTIME_EXPORT
 char *
 swift_copyMetadataSectionName(const struct swift::MetadataSections *section);
 
+SWIFT_RUNTIME_EXPORT
+void swift_freeMetadataSectionName(char *name);
+
 SWIFT_RUNTIME_EXPORT
 void swift_getMetadataSectionBaseAddress(
   const struct swift::MetadataSections *section,

stdlib/public/runtime/ReflectionMirror.cpp

@@ -1116,9 +1116,10 @@ id swift_reflectionMirror_quickLookObject(OpaqueValue *value, const Metadata *T)
 
 SWIFT_CC(swift) SWIFT_RUNTIME_STDLIB_INTERNAL
 const char *swift_keyPath_copySymbolName(void *address) {
-  auto info = SymbolInfo::lookup(address);
-  if (info.has_value() && info->getSymbolName()) {
-    return strdup(info->getSymbolName());
+  if (auto info = SymbolInfo::lookup(address)) {
+    if (const char *symbolName = info->getSymbolName()) {
+      return strdup(symbolName);
+    }
   }
   return nullptr;
 }

stdlib/public/runtime/SymbolInfo.cpp

@@ -18,6 +18,7 @@
 #include <Windows.h>
 #include <DbgHelp.h>
 #include <Psapi.h>
+#include <shlwapi.h>
 #elif SWIFT_STDLIB_HAS_DLADDR
 #include <dlfcn.h>
 #endif
@@ -42,8 +43,8 @@ _address(nullptr)
 
 SymbolInfo::~SymbolInfo() {
 #if defined(_WIN32) && !defined(__CYGWIN__)
-  if (char *filename = _filename.getValueOr(nullptr)) {
-    free(filename);
+  if (char *imagePath = _imagePath.getValueOr(nullptr)) {
+    free(imagePath);
   }
 #endif
 }
@@ -52,14 +53,14 @@ const void *SymbolInfo::getAddress() const {
   return _address;
 }
 
-const char *SymbolInfo::getFilename() const {
+const char *getImagePath() const {
 #if defined(_WIN32) && !defined(__CYGWIN__)
-  if (!_filename.has_value()) {
+  if (!_imagePath.has_value()) {
     // On Win32/Win64, the base address of a loaded image and its HMODULE are
     // the same pointer, so we can simply cast from one to the other.
-    HMODULE hModule = reinterpret_cast<HMODULE>(getBaseAddress());
+    HMODULE hModule = reinterpret_cast<HMODULE>(getImageBaseAddress());
     if (!hModule) {
-      _filename = nullptr;
+      _imagePath = nullptr;
       return;
     }
 
@@ -68,19 +69,36 @@ const char *SymbolInfo::getFilename() const {
     // other information we don't need.
     std::array<wchar_t, MAX_PATH> filename;
     if (GetModuleFileNameW(hModule, filename.data(), filename.size())) {
-      _filename = swift_copyUTF8FromWide(filename.data());
+      _imagePath = swift_copyUTF8FromWide(filename.data());
     }
   }
 
-  return _filename.getValueOr(nullptr);
+  return _imagePath.getValueOr(nullptr);
 #elif SWIFT_STDLIB_HAS_DLADDR
   return _info.dli_fname;
 #else
   return nullptr;
 #endif
 }
 
-const void *SymbolInfo::getBaseAddress() const {
+const char *SymbolInfo::getImageFilename() const {
+  // TODO: if we adopt C++17, consider using std::filesystem::path::filename()
+  const char *imagePath = getImagePath();
+  if (!imagePath) {
+    return nullptr;
+  }
+#if defined(_WIN32) && !defined(__CYGWIN__)
+  return PathFindFileNameA(imagePath);
+#else
+  if (const char *lastSlash = strrchr(imagePath, '/')) {
+    return lastSlash + 1;
+  } else {
+    return imagePath;
+  }
+#endif
+}
+
+const void *SymbolInfo::getImageBaseAddress() const {
 #if defined(_WIN32) && !defined(__CYGWIN__)
   return reinterpret_cast<const void *>(_package.si.ModBase);
 #elif SWIFT_STDLIB_HAS_DLADDR
@@ -127,7 +145,7 @@ llvm::Optional<SymbolInfo> SymbolInfo::lookup(const void *address) {
     // If there are real-world examples of symbol names with Unicode characters
     // that we need to consider, SymbolInfo can hold a SYMBOL_INFO_PACKAGEW and
     // call SymFromAddrW(), then lazily convert to UTF-8 on the first call to
-    // getSymbolName() as is done in getFilename().
+    // getSymbolName() as is done in getImagePath().
     SymbolInfo info;
     info._package.si.SizeOfStruct = sizeof(SYMBOL_INFO);
     info._package.si.MaxNameLen = MAX_SYM_NAME;

stdlib/public/runtime/SymbolInfo.h

@@ -36,7 +36,7 @@ struct SymbolInfo {
 private:
 #if defined(_WIN32) && !defined(__CYGWIN__)
   SYMBOL_INFO_PACKAGE _package;
-  mutable llvm::Optional<char *> _filename;
+  mutable llvm::Optional<char *> _imagePath;
 #elif SWIFT_STDLIB_HAS_DLADDR
   Dl_info _info;
 #endif
@@ -50,25 +50,38 @@ struct SymbolInfo {
   /// Get the address that was originally passed when this instance was created.
   const void *getAddress() const;
 
+  /// Get the path to the image where the symbol was found.
+  ///
+  /// The image path uses the platform path convention. To consistently get just
+  /// the filename of the image, use \c getImageFilename().
+  ///
+  /// The resulting C string is only valid for the lifetime of \c this.
+  const char *getImagePath() const;
+
   /// Get the file name of the image where the symbol was found.
   ///
+  /// The file name is the last path component of the image path. If the
+  /// filename cannot be determined from the image path (for instance, because
+  /// it is a relative path or was invalidly specified), the complete image path
+  /// is returned.
+  ///
   /// The resulting C string is only valid for the lifetime of \c this.
-  const char *getFilename() const;
+  const char *getImageFilename() const;
 
   /// Get the base address of the image where the symbol was found.
-  const void *getBaseAddress() const;
+  const void *getImageBaseAddress() const;
 
   /// Get the name of the symbol.
   ///
-  /// If the input address is valid within a loaded library, but does not
+  /// If the input address is valid within a loaded image, but does not
   /// correspond to any known symbol, the resulting pointer may be \c nullptr.
   ///
   /// The resulting C string is only valid for the lifetime of \c this.
   const char *getSymbolName() const;
 
   /// Get the address of the symbol.
   ///
-  /// If the input address is valid within a loaded library, but does not
+  /// If the input address is valid within a loaded image, but does not
   /// correspond to any known symbol, the resulting pointer may be \c nullptr.
   const void *getSymbolAddress() const;
 

test/Runtime/loaded_image_uniqueness.swift

@@ -17,6 +17,11 @@ internal func _copyMetadataSectionName(
     _ metadata_section: UnsafeRawPointer
 ) -> UnsafeMutablePointer<CChar>?
 
+@_silgen_name("swift_freeMetadataSectionName")
+internal func _freeMetadataSectionName(
+  _ name: UnsafeMutablePointer<CChar>
+) -> Void
+
 @_silgen_name("swift_getMetadataSectionBaseAddress")
 internal func _getMetadataSectionBaseAddress(
   _ metadata_section: UnsafeRawPointer,
@@ -36,7 +41,7 @@ do {
     }
     let cName = _copyMetadataSectionName(rawPointer)
     defer {
-      cName.deallocate()
+      _freeMetadataSectionName(cName)
     }
     let name = cName.flatMap { String(validatingUTF8: $0) } ?? "<unavailable>"