[Coroutines] Avoid assert splitting hidden coros

[modocache]

Summary

When attempting to split a coroutine with 'hidden' visibility (for
example, a C++ coroutine that is inlined when compiled with the option
'-fvisibility-inlines-hidden'), LLVM would hit an assertion in
include/llvm/IR/GlobalValue.h:240: "local linkage requires default
visibility". The issue is that the visibility is copied from the source
of the function split in the CloneFunctionInto function, but the linkage
is not. To fix, create the new function first with external linkage,
then copy the linkage from the original function *after* CloneFunctionInto
is called.

Since GlobalValue::setLinkage in turn calls maybeSetDsoLocal, the
explicit call to setDSOLocal can be removed in CoroSplit.cpp.

Test Plan: check-llvm

[modocache]

Update formatting with clang-format.

[GorNishanov]

Looks good to me, but, adding @rnk and @majnemer, just in case.

[rnk]

I think the right fix is to leave these internal and to avoid copying the visibility. Internal linkage symbols are in some sense always hidden, they never go in the dynamic symbol table. I can't see where the visibility gets copied, though.

[modocache]

lib/Transforms/Coroutines/CoroSplit.cpp

266 ↗

(On Diff #137317)

@rnk This function, CloneFunctionInto, calls Function::copyAttributesFrom, which in turn calls GlobalValue::copyAttributesFrom and finally GlobalValue::setVisibility, which triggers the assert when the original function F has hidden linkage.

I think the right fix is to leave these internal and to avoid copying the visibility.

That sounds like a good idea to me, too. It might be a little tricky because the copying occurs several layers deep.

Perhaps I could add a Function::copyAttributesFrom overload that also takes, as an argument, a list of attributes to not copy? I could then prevent the visibility attribute from being copied here, in lib/Transforms/Utils/CloneFunction.cpp:

void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
                             ValueToValueMapTy &VMap,
                             bool ModuleLevelChanges,
                             SmallVectorImpl<ReturnInst*> &Returns,
                             const char *NameSuffix, ClonedCodeInfo *CodeInfo,
                             ValueMapTypeRemapper *TypeMapper,
                             ValueMaterializer *Materializer) {
  // ...

  // Copy all attributes other than those stored in the AttributeList.  We need
  // to remap the parameter indices of the AttributeList.
  AttributeList NewAttrs = NewFunc->getAttributes();
  NewFunc->copyAttributesFrom(OldFunc);
  NewFunc->setAttributes(NewAttrs);

  // ...
}

It seems like it's the intention of this code to only copy attributes that aren't set on the new function, so an overload may express this intent even better. And preventing the visibility from being copied should prevent this assert from being hit as well. What do you think? Does that sound like a reasonable approach?

[rnk]

lib/Transforms/Coroutines/CoroSplit.cpp

266 ↗

(On Diff #137317)

I would suggest creating the function with external linkage, and then down here after CloneFunctionInfo call setLinkage, which will reset the visibility and DSO local bits.

[modocache]

lib/Transforms/Coroutines/CoroSplit.cpp

266 ↗

(On Diff #137317)

Good thinking! Thanks, I'll do this.

[modocache]

Thanks @rnk! I updated this as per your suggestions. As a result it's a smaller and less invasive change -- thank you! Let me know if this looks good.

[modocache]

Oops! I used arc diff --verbatim and ended up removing reviewers.

[rnk]

lgtm, thanks!

llvm/trunk/lib/Transforms/Coroutines/CoroSplit.cpp

 static Function *createClone(Function &F, Twine Suffix, coro::Shape &Shape,
                              BasicBlock *ResumeEntry, int8_t FnIndex) {
   Module *M = F.getParent();
   auto *FrameTy = Shape.FrameTy;
   auto *FnPtrTy = cast<PointerType>(FrameTy->getElementType(0));
   auto *FnTy = cast<FunctionType>(FnPtrTy->getElementType());
 
   Function *NewF =
-      Function::Create(FnTy, GlobalValue::LinkageTypes::InternalLinkage,
+      Function::Create(FnTy, GlobalValue::LinkageTypes::ExternalLinkage,
                        F.getName() + Suffix, M);
   NewF->addParamAttr(0, Attribute::NonNull);
   NewF->addParamAttr(0, Attribute::NoAlias);
 
   ValueToValueMapTy VMap;
   // Replace all args with undefs. The buildCoroutineFrame algorithm already
   // rewritten access to the args that occurs after suspend points with loads
   // and stores to/from the coroutine frame.
   for (Argument &A : F.args())
     VMap[&A] = UndefValue::get(A.getType());
 
   SmallVector<ReturnInst *, 4> Returns;
 
   CloneFunctionInto(NewF, &F, VMap, /*ModuleLevelChanges=*/true, Returns);
-  NewF->setDSOLocal(true);
+  NewF->setLinkage(GlobalValue::LinkageTypes::InternalLinkage);
 
   // Remove old returns.
   for (ReturnInst *Return : Returns)
     changeToUnreachable(Return, /*UseLLVMTrap=*/false);
 
   // Remove old return attributes.
   NewF->removeAttributes(
       AttributeList::ReturnIndex,

llvm/trunk/test/Transforms/Coroutines/coro-split-hidden.ll

+; Tests that coro-split can convert functions with hidden visibility.
+; These may be generated by a frontend such as Clang, when inlining with
+; '-fvisibility-inlines-hidden'.
+; RUN: opt < %s -coro-split -S | FileCheck %s
+
+define hidden i8* @f() "coroutine.presplit"="1" {
+entry:
+  %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
+  %need.alloc = call i1 @llvm.coro.alloc(token %id)
+  br i1 %need.alloc, label %dyn.alloc, label %begin
+
+dyn.alloc:
+  %size = call i32 @llvm.coro.size.i32()
+  %alloc = call i8* @malloc(i32 %size)
+  br label %begin
+
+begin:
+  %phi = phi i8* [ null, %entry ], [ %alloc, %dyn.alloc ]
+  %hdl = call i8* @llvm.coro.begin(token %id, i8* %phi)
+  call void @print(i32 0)
+  %0 = call i8 @llvm.coro.suspend(token none, i1 false)
+  switch i8 %0, label %suspend [i8 0, label %resume
+                                i8 1, label %cleanup]
+resume:
+  call void @print(i32 1)
+  br label %cleanup
+
+cleanup:
+  %mem = call i8* @llvm.coro.free(token %id, i8* %hdl)
+  call void @free(i8* %mem)
+  br label %suspend
+suspend:
+  call i1 @llvm.coro.end(i8* %hdl, i1 0)
+  ret i8* %hdl
+}
+
+; CHECK-LABEL: hidden{{.*}}@f(
+; CHECK: call i8* @malloc
+; CHECK: @llvm.coro.begin(token %id, i8* %phi)
+; CHECK: store void (%f.Frame*)* @f.resume, void (%f.Frame*)** %resume.addr
+; CHECK: %[[SEL:.+]] = select i1 %need.alloc, void (%f.Frame*)* @f.destroy, void (%f.Frame*)* @f.cleanup
+; CHECK: store void (%f.Frame*)* %[[SEL]], void (%f.Frame*)** %destroy.addr
+; CHECK: call void @print(i32 0)
+; CHECK-NOT: call void @print(i32 1)
+; CHECK-NOT: call void @free(
+; CHECK: ret i8* %hdl
+
+; CHECK-LABEL: internal{{.*}}@f.resume(
+; CHECK-NOT: call i8* @malloc
+; CHECK-NOT: call void @print(i32 0)
+; CHECK: call void @print(i32 1)
+; CHECK-NOT: call void @print(i32 0)
+; CHECK: call void @free(
+; CHECK: ret void
+
+; CHECK-LABEL: internal{{.*}}@f.destroy(
+; CHECK-NOT: call i8* @malloc
+; CHECK-NOT: call void @print(
+; CHECK: call void @free(
+; CHECK: ret void
+
+; CHECK-LABEL: internal{{.*}}@f.cleanup(
+; CHECK-NOT: call i8* @malloc
+; CHECK-NOT: call void @print(
+; CHECK-NOT: call void @free(
+; CHECK: ret void
+
+declare i8* @llvm.coro.free(token, i8*)
+declare i32 @llvm.coro.size.i32()
+declare i8  @llvm.coro.suspend(token, i1)
+declare void @llvm.coro.resume(i8*)
+declare void @llvm.coro.destroy(i8*)
+
+declare token @llvm.coro.id(i32, i8*, i8*, i8*)
+declare i1 @llvm.coro.alloc(token)
+declare i8* @llvm.coro.begin(token, i8*)
+declare i1 @llvm.coro.end(i8*, i1)
+
+declare noalias i8* @malloc(i32)
+declare void @print(i32)
+declare void @free(i8*)