Skip to content

IRGen: fix swifterror attribute mismatch for WebAssembly #39359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion include/swift/Runtime/RuntimeFnWrappersGen.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ namespace swift {
class AvailabilityContext;
class ASTContext;

namespace irgen {
class IRGenModule;
}

enum class RuntimeAvailability {
AlwaysAvailable,
AvailableByCompatibilityLibrary,
Expand All @@ -39,7 +43,8 @@ llvm::Constant *getRuntimeFn(llvm::Module &Module, llvm::Constant *&cache,
RuntimeAvailability availability,
llvm::ArrayRef<llvm::Type *> retTypes,
llvm::ArrayRef<llvm::Type *> argTypes,
llvm::ArrayRef<llvm::Attribute::AttrKind> attrs);
llvm::ArrayRef<llvm::Attribute::AttrKind> attrs,
irgen::IRGenModule *IGM = nullptr);

} // namespace swift
#endif
4 changes: 2 additions & 2 deletions lib/IRGen/GenCall.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ void IRGenModule::addSwiftErrorAttributes(llvm::AttributeList &attrs,
// We create a shadow stack location of the swifterror parameter for the
// debugger on such platforms and so we can't mark the parameter with a
// swifterror attribute.
if (IsSwiftErrorInRegister)
if (ShouldUseSwiftError)
b.addAttribute(llvm::Attribute::SwiftError);

// The error result should not be aliased, captured, or pointed at invalid
Expand Down Expand Up @@ -4245,7 +4245,7 @@ Address IRGenFunction::createErrorResultSlot(SILType errorType, bool isAsync) {
// The slot for async callees cannot be annotated swifterror because those
// errors are never passed in registers but rather are always passed
// indirectly in the async context.
if (IGM.IsSwiftErrorInRegister && !isAsync)
if (IGM.ShouldUseSwiftError && !isAsync)
cast<llvm::AllocaInst>(addr.getAddress())->setSwiftError(true);

// Initialize at the alloca point.
Expand Down
32 changes: 27 additions & 5 deletions lib/IRGen/IRGenModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -577,10 +577,15 @@ IRGenModule::IRGenModule(IRGenerator &irgen,
AtomicBoolSize = Size(ClangASTContext->getTypeSize(atomicBoolTy));
AtomicBoolAlign = Alignment(ClangASTContext->getTypeSize(atomicBoolTy));
}

IsSwiftErrorInRegister =
// On WebAssembly, tail optional arguments are not allowed because Wasm requires
// callee and caller signature to be the same. So LLVM adds dummy arguments for
// `swiftself` and `swifterror`. If there is `swiftself` but is no `swifterror` in
// a swiftcc function or invocation, then LLVM adds dummy `swifterror` parameter or
// argument. To count up how many dummy arguments should be added, we need to mark
// it as `swifterror` even though it's not in register.
ShouldUseSwiftError =
clang::CodeGen::swiftcall::isSwiftErrorLoweredInRegister(
ClangCodeGen->CGM());
ClangCodeGen->CGM()) || TargetInfo.OutputObjectFormat == llvm::Triple::Wasm;

#ifndef NDEBUG
sanityCheckStdlib(*this);
Expand Down Expand Up @@ -881,7 +886,8 @@ llvm::Constant *swift::getRuntimeFn(llvm::Module &Module,
RuntimeAvailability availability,
llvm::ArrayRef<llvm::Type*> retTypes,
llvm::ArrayRef<llvm::Type*> argTypes,
ArrayRef<Attribute::AttrKind> attrs) {
ArrayRef<Attribute::AttrKind> attrs,
IRGenModule *IGM) {

if (cache)
return cache;
Expand Down Expand Up @@ -954,6 +960,22 @@ llvm::Constant *swift::getRuntimeFn(llvm::Module &Module,
fn->addFnAttrs(buildFnAttr);
fn->addRetAttrs(buildRetAttr);
fn->addParamAttrs(0, buildFirstParamAttr);

// Add swiftself and swifterror attributes only when swift_willThrow
// swift_willThrow is defined in RuntimeFunctions.def, but due to the
// DSL limitation, arguments attributes are not set.
// On the other hand, caller of `swift_willThrow` assumes that it's attributed
// with `swiftself` and `swifterror`.
// This mismatch of attributes would be issue when lowering to WebAssembly.
// While lowering, LLVM counts how many dummy params are necessary to match
// callee and caller signature. So we need to add them correctly.
if (functionName == "swift_willThrow") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we model this with a flag in the RuntimeFunctions.def DSL? The specific function name check feels brittle.

Copy link
Contributor Author

@MaxDesiatov MaxDesiatov Dec 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would require some refinement of such DSL, right? Currently it doesn't allow setting attributes on parameters. What would be the best way to specify these attributes in RuntimeFunctions.def then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed your reply in my flood of GitHub notifications. I think we can do this refactoring, but it shouldn't hold up this PR. Re-running testing and switching to "accept", sorry.

assert(IGM && "IGM is required for swift_willThrow.");
fn->addParamAttr(0, Attribute::AttrKind::SwiftSelf);
if (IGM->ShouldUseSwiftError) {
fn->addParamAttr(1, Attribute::AttrKind::SwiftError);
}
}
}

return cache;
Expand Down Expand Up @@ -997,7 +1019,7 @@ void IRGenModule::registerRuntimeEffect(ArrayRef<RuntimeEffect> effect,
registerRuntimeEffect(EFFECT, #NAME); \
return getRuntimeFn(Module, ID##Fn, #NAME, CC, \
AVAILABILITY(this->Context), \
RETURNS, ARGS, ATTRS); \
RETURNS, ARGS, ATTRS, this); \
}

#include "swift/Runtime/RuntimeFunctions.def"
Expand Down
4 changes: 2 additions & 2 deletions lib/IRGen/IRGenModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -628,8 +628,8 @@ class IRGenModule {
/// Should we add value names to local IR values?
bool EnableValueNames = false;

// Is swifterror returned in a register by the target ABI.
bool IsSwiftErrorInRegister;
// Should `swifterror` attribute be explicitly added for the target ABI.
bool ShouldUseSwiftError;

llvm::Type *VoidTy; /// void (usually {})
llvm::IntegerType *Int1Ty; /// i1
Expand Down
2 changes: 1 addition & 1 deletion lib/IRGen/IRGenSIL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4812,7 +4812,7 @@ void IRGenSILFunction::emitErrorResultVar(CanSILFunctionType FnTy,
DebugValueInst *DbgValue) {
// We don't need a shadow error variable for debugging on ABI's that return
// swifterror in a register.
if (IGM.IsSwiftErrorInRegister)
if (IGM.ShouldUseSwiftError)
return;
auto ErrorResultSlot = getCalleeErrorResultSlot(IGM.silConv.getSILType(
ErrorInfo, FnTy, IGM.getMaximalTypeExpansionContext()));
Expand Down