Merge pull request swiftlang#64006 from rintaro/macros-plugin-errhandling

[Macros] Improve error handling for executable macro plugins

Author: rintaro

lib/AST/CASTBridging.cpp

@@ -653,15 +653,26 @@ bool Plugin_sendMessage(PluginHandle handle, const BridgedData data) {
   auto *plugin = static_cast<LoadedExecutablePlugin *>(handle);
   StringRef message(data.baseAddress, data.size);
   auto error = plugin->sendMessage(message);
-  bool hadError = bool(error);
-  llvm::consumeError(std::move(error));
-  return hadError;
+  if (error) {
+    // FIXME: Pass the error message back to the caller.
+    llvm::consumeError(std::move(error));
+//    llvm::handleAllErrors(std::move(error), [](const llvm::ErrorInfoBase &err) {
+//      llvm::errs() << err.message() << "\n";
+//    });
+    return true;
+  }
+  return false;
 }
 
 bool Plugin_waitForNextMessage(PluginHandle handle, BridgedData *out) {
   auto *plugin = static_cast<LoadedExecutablePlugin *>(handle);
   auto result = plugin->waitForNextMessage();
   if (!result) {
+    // FIXME: Pass the error message back to the caller.
+    llvm::consumeError(result.takeError());
+//    llvm::handleAllErrors(result.takeError(), [](const llvm::ErrorInfoBase &err) {
+//      llvm::errs() << err.message() << "\n";
+//    });
     return true;
   }
   auto &message = result.get();

lib/AST/PluginRegistry.cpp

@@ -12,6 +12,7 @@
 
 #include "swift/AST/PluginRegistry.h"
 
+#include "swift/Basic/Defer.h"
 #include "swift/Basic/LLVM.h"
 #include "swift/Basic/Program.h"
 #include "swift/Basic/Sandbox.h"
@@ -21,6 +22,7 @@
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Config/config.h"
 
+#include <signal.h>
 
 #if defined(_WIN32)
 #define WIN32_LEAN_AND_MEAN
@@ -119,7 +121,6 @@ LoadedExecutablePlugin::LoadedExecutablePlugin(
       outputFileDescriptor(outputFileDescriptor) {}
 
 LoadedExecutablePlugin::~LoadedExecutablePlugin() {
-  // Close the pipes.
   close(inputFileDescriptor);
   close(outputFileDescriptor);
 
@@ -131,10 +132,18 @@ ssize_t LoadedExecutablePlugin::read(void *buf, size_t nbyte) const {
   ssize_t bytesToRead = nbyte;
   void *ptr = buf;
 
+#if defined(SIGPIPE)
+  /// Ignore SIGPIPE while reading.
+  auto *old_handler = signal(SIGPIPE, SIG_IGN);
+  SWIFT_DEFER { signal(SIGPIPE, old_handler); };
+#endif
+
   while (bytesToRead > 0) {
     ssize_t readingSize = std::min(ssize_t(INT32_MAX), bytesToRead);
     ssize_t readSize = ::read(inputFileDescriptor, ptr, readingSize);
-    if (readSize == 0) {
+    if (readSize <= 0) {
+      // 0: EOF (the plugin exited?), -1: error (e.g. broken pipe.)
+      // FIXME: Mark the plugin 'stale' and relaunch later.
       break;
     }
     ptr = static_cast<char *>(ptr) + readSize;
@@ -148,10 +157,18 @@ ssize_t LoadedExecutablePlugin::write(const void *buf, size_t nbyte) const {
   ssize_t bytesToWrite = nbyte;
   const void *ptr = buf;
 
+#if defined(SIGPIPE)
+  /// Ignore SIGPIPE while writing.
+  auto *old_handler = signal(SIGPIPE, SIG_IGN);
+  SWIFT_DEFER { signal(SIGPIPE, old_handler); };
+#endif
+
   while (bytesToWrite > 0) {
     ssize_t writingSize = std::min(ssize_t(INT32_MAX), bytesToWrite);
     ssize_t writtenSize = ::write(outputFileDescriptor, ptr, writingSize);
-    if (writtenSize == 0) {
+    if (writtenSize <= 0) {
+      // -1: error (e.g. broken pipe,)
+      // FIXME: Mark the plugin 'stale' and relaunch later.
       break;
     }
     ptr = static_cast<const char *>(ptr) + writtenSize;
@@ -170,12 +187,17 @@ llvm::Error LoadedExecutablePlugin::sendMessage(llvm::StringRef message) const {
   uint64_t header = llvm::support::endian::byte_swap(
       uint64_t(size), llvm::support::endianness::little);
   writtenSize = write(&header, sizeof(header));
-  assert(writtenSize == sizeof(header) &&
-         "failed to write plugin message header");
+  if (writtenSize != sizeof(header)) {
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "failed to write plugin message header");
+  }
 
   // Write message.
   writtenSize = write(data, size);
-  assert(writtenSize == ssize_t(size) && "failed to write plugin message data");
+  if (writtenSize != ssize_t(size)) {
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "failed to write plugin message data");
+  }
 
   return llvm::Error::success();
 }
@@ -187,8 +209,10 @@ llvm::Expected<std::string> LoadedExecutablePlugin::waitForNextMessage() const {
   uint64_t header;
   readSize = read(&header, sizeof(header));
 
-  // FIXME: Error handling. Disconnection, etc.
-  assert(readSize == sizeof(header) && "failed to read plugin message header");
+  if (readSize != sizeof(header)) {
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "failed to read plugin message header");
+  }
 
   size_t size = llvm::support::endian::read<uint64_t>(
       &header, llvm::support::endianness::little);
@@ -200,6 +224,10 @@ llvm::Expected<std::string> LoadedExecutablePlugin::waitForNextMessage() const {
   while (sizeToRead > 0) {
     char buffer[4096];
     readSize = read(buffer, std::min(sizeof(buffer), sizeToRead));
+    if (readSize == 0) {
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "failed to read plugin message data");
+    }
     sizeToRead -= readSize;
     message.append(buffer, readSize);
   }

lib/ASTGen/Sources/ASTGen/Macros.swift

@@ -254,30 +254,40 @@ func expandFreestandingMacroIPC(
 
   let macro = macroPtr.assumingMemoryBound(to: ExportedExecutableMacro.self).pointee
 
+  // Send the message.
   let message = HostToPluginMessage.expandFreestandingMacro(
     macro: .init(moduleName: macro.moduleName, typeName: macro.typeName, name: macroName),
     discriminator: discriminator,
     syntax: PluginMessage.Syntax(syntax: macroSyntax, in: sourceFilePtr)!)
-
   do {
     let result = try macro.plugin.sendMessageAndWait(message)
-    switch result {
-    case .expandFreestandingMacroResult(var expandedSource, let diagnostics):
-      if !diagnostics.isEmpty {
-        let diagEngine = PluginDiagnosticsEngine(cxxDiagnosticEngine: diagEnginePtr)
-        diagEngine.add(exportedSourceFile: sourceFilePtr)
-
-        for diagnostic in diagnostics {
-          diagEngine.emit(diagnostic, messageSuffix: " (from macro '\(macroName)')")
-        }
-      }
-      return expandedSource
+    guard
+      case .expandFreestandingMacroResult(let expandedSource, let diagnostics) = result
+    else {
+      throw PluginError.invalidReponseKind
+    }
 
-    default:
-      fatalError("unexpected result")
+    // Process the result.
+    if !diagnostics.isEmpty {
+      let diagEngine = PluginDiagnosticsEngine(cxxDiagnosticEngine: diagEnginePtr)
+      diagEngine.add(exportedSourceFile: sourceFilePtr)
+      diagEngine.emit(diagnostics, messageSuffix: " (from macro '\(macroName)')")
     }
+    return expandedSource
+
   } catch let error {
-    fatalError("\(error)")
+    let srcMgr = SourceManager(cxxDiagnosticEngine: diagEnginePtr)
+    srcMgr.insert(sourceFilePtr)
+    srcMgr.diagnose(
+      diagnostic: .init(
+        node: macroSyntax,
+        // FIXME: This is probably a plugin communication error.
+        // The error might not be relevant as the diagnostic message.
+        message: ThrownErrorDiagnostic(message: String(describing: error))
+      ),
+      messageSuffix: " (from macro '\(macroName)')"
+    )
+    return nil
   }
 }
 
@@ -580,6 +590,7 @@ func expandAttachedMacroIPC(
     parentDeclSyntax = nil
   }
 
+  // Send the message.
   let message = HostToPluginMessage.expandAttachedMacro(
     macro: .init(moduleName: macro.moduleName, typeName: macro.typeName, name: macroName),
     macroRole: macroRole,
@@ -589,30 +600,42 @@ func expandAttachedMacroIPC(
     parentDeclSyntax: parentDeclSyntax)
   do {
     let result = try macro.plugin.sendMessageAndWait(message)
-    switch result {
-    case .expandAttachedMacroResult(let expandedSources, let diagnostics):
-      // Form the result buffer for our caller.
-
-      if !diagnostics.isEmpty {
-        let diagEngine = PluginDiagnosticsEngine(cxxDiagnosticEngine: diagEnginePtr)
-        diagEngine.add(exportedSourceFile: customAttrSourceFilePtr)
-        diagEngine.add(exportedSourceFile: declarationSourceFilePtr)
-        if let parentDeclSourceFilePtr = parentDeclSourceFilePtr {
-          diagEngine.add(exportedSourceFile: parentDeclSourceFilePtr)
-        }
-
-        for diagnostic in diagnostics {
-          diagEngine.emit(diagnostic, messageSuffix: " (from macro '\(macroName)')")
-        }
+    guard
+      case .expandAttachedMacroResult(let expandedSources, let diagnostics) = result
+    else {
+      throw PluginError.invalidReponseKind
+    }
 
+    // Process the result.
+    if !diagnostics.isEmpty {
+      let diagEngine = PluginDiagnosticsEngine(cxxDiagnosticEngine: diagEnginePtr)
+      diagEngine.add(exportedSourceFile: customAttrSourceFilePtr)
+      diagEngine.add(exportedSourceFile: declarationSourceFilePtr)
+      if let parentDeclSourceFilePtr = parentDeclSourceFilePtr {
+        diagEngine.add(exportedSourceFile: parentDeclSourceFilePtr)
       }
-      return expandedSources
-
-    default:
-      fatalError("unexpected result")
+      diagEngine.emit(diagnostics, messageSuffix: " (from macro '\(macroName)')")
     }
+    return expandedSources
+
   } catch let error {
-    fatalError("\(error)")
+    let srcMgr = SourceManager(cxxDiagnosticEngine: diagEnginePtr)
+    srcMgr.insert(customAttrSourceFilePtr)
+    srcMgr.insert(declarationSourceFilePtr)
+    if let parentDeclSourceFilePtr = parentDeclSourceFilePtr {
+      srcMgr.insert(parentDeclSourceFilePtr)
+    }
+    // FIXME: Need to decide where to diagnose the error:
+    srcMgr.diagnose(
+      diagnostic: .init(
+        node: Syntax(declarationNode),
+        // FIXME: This is probably a plugin communication error.
+        // The error might not be relevant as the diagnostic message.
+        message: ThrownErrorDiagnostic(message: String(describing: error))
+      ),
+      messageSuffix: " (from macro '\(macroName)')"
+    )
+    return nil
   }
 }
 

lib/ASTGen/Sources/ASTGen/PluginHost.swift

@@ -14,7 +14,11 @@ import CASTBridging
 import CBasicBridging
 import SwiftSyntax
 
-struct PluginError: Error {}
+enum PluginError: Error {
+  case failedToSendMessage
+  case failedToReceiveMessage
+  case invalidReponseKind
+}
 
 @_cdecl("swift_ASTGen_initializePlugin")
 public func _initializePlugin(
@@ -50,7 +54,7 @@ struct CompilerPlugin {
       return Plugin_sendMessage(opaqueHandle, BridgedData(baseAddress: data.baseAddress, size: data.count))
     }
     if hadError {
-      throw PluginError()
+      throw PluginError.failedToSendMessage
     }
   }
 
@@ -59,7 +63,7 @@ struct CompilerPlugin {
     let hadError = Plugin_waitForNextMessage(opaqueHandle, &result)
     defer { BridgedData_free(result) }
     guard !hadError else {
-      throw PluginError()
+      throw PluginError.failedToReceiveMessage
     }
     let data = UnsafeBufferPointer(start: result.baseAddress, count: result.size)
 //    // FIXME: Add -dump-plugin-message option?
@@ -209,19 +213,31 @@ class PluginDiagnosticsEngine {
     SwiftDiagnostic_finish(diag)
   }
 
+  /// Emit diagnostics.
+  func emit(
+    _ diagnostics: [PluginMessage.Diagnostic],
+    messageSuffix: String? = nil
+  ) {
+    for diagnostic in diagnostics {
+      self.emit(diagnostic)
+    }
+  }
+
   /// Produce the C++ source location for a given position based on a
   /// syntax node.
   private func cxxSourceLocation(
     at offset: Int, in fileName: String
   ) -> CxxSourceLoc? {
     // Find the corresponding exported source file.
-    guard let exportedSourceFile = exportedSourceFileByName[fileName]
+    guard
+      let exportedSourceFile = exportedSourceFileByName[fileName]
     else {
       return nil
     }
 
     // Compute the resulting address.
-    guard let bufferBaseAddress = exportedSourceFile.pointee.buffer.baseAddress
+    guard
+      let bufferBaseAddress = exportedSourceFile.pointee.buffer.baseAddress
     else {
       return nil
     }

test/Macros/macro_plugin_error.swift

@@ -0,0 +1,56 @@
+// REQUIRES: OS=macosx
+
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+// RUN: %clang \
+// RUN:  -isysroot %sdk \
+// RUN:  -I %swift_src_root/include \
+// RUN:  -L %swift-lib-dir -l_swiftMockPlugin \
+// RUN:  -Wl,-rpath,%swift-lib-dir \
+// RUN:  -o %t/mock-plugin \
+// RUN:  %t/plugin.c
+
+// RUN: %swift-target-frontend \
+// RUN:   -typecheck -verify \
+// RUN:   -swift-version 5 -enable-experimental-feature Macros \
+// RUN:   -load-plugin-executable %t/mock-plugin#TestPlugin \
+// RUN:   -dump-macro-expansions \
+// RUN:   %t/test.swift
+
+//--- test.swift
+@freestanding(expression) macro fooMacro(_: Any) -> String = #externalMacro(module: "TestPlugin", type: "FooMacro")
+
+func test() {
+  // FIXME: Should be more user friendly message.
+
+  let _: String = #fooMacro(1)
+  // expected-error @-1 {{typeMismatch(swiftASTGen.PluginToHostMessage}}
+  let _: String = #fooMacro(2)
+  // expected-error @-1 {{failedToReceiveMessage}}
+  let _: String = #fooMacro(3)
+  // expected-error @-1 {{failedToSendMessage}}
+  //FIXME: ^ This should succeed. Error recovery is not implemented.
+}
+
+//--- plugin.c
+#include "swift-c/MockPlugin/MockPlugin.h"
+
+MOCK_PLUGIN([
+  {
+    "expect": {"getCapability": {}},
+    "response": {"getCapabilityResult": {"capability": {"protocolVersion": 1}}}
+  },
+  {
+    "expect": {"expandFreestandingMacro": {
+                "macro": {"moduleName": "TestPlugin", "typeName": "FooMacro"},
+                "syntax": {"kind": "expression", "source": "#fooMacro(1)"}}},
+    "response": {"invalidResponse": {}}
+  },
+  {
+    "expect": {"expandFreestandingMacro": {
+                "macro": {"moduleName": "TestPlugin", "typeName": "FooMacro"},
+                "syntax": {"kind": "expression", "source": "#fooMacro(3)"}}},
+    "response": {"expandFreestandingMacroResult": {"expandedSource": "3", "diagnostics": []}}
+  }
+])