This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines] Avoid assert splitting hidden coros
ClosedPublic

Authored by modocache on Mar 6 2018, 7:02 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

modocache created this revision.Mar 6 2018, 7:02 PM
modocache updated this revision to Diff 137317.Mar 6 2018, 7:10 PM

Update formatting with clang-format.

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

rnk requested changes to this revision.Mar 30 2018, 3:36 PM

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.

This revision now requires changes to proceed.Mar 30 2018, 3:36 PM
modocache added inline comments.Apr 1 2018, 5:39 PM
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 added inline comments.Apr 2 2018, 9:45 AM
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 added inline comments.Apr 2 2018, 9:47 AM
lib/Transforms/Coroutines/CoroSplit.cpp
266 ↗(On Diff #137317)

Good thinking! Thanks, I'll do this.

modocache updated this revision to Diff 140691.Apr 2 2018, 2:20 PM
modocache edited the summary of this revision. (Show Details)
modocache removed reviewers: majnemer, rnk.

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.

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

rnk accepted this revision.Apr 2 2018, 3:50 PM

lgtm, thanks!

This revision is now accepted and ready to land.Apr 2 2018, 3:50 PM
This revision was automatically updated to reflect the committed changes.