Skip to content

Fix SILVerifier assert for witness_method with dynamic self type #34738

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
Nov 16, 2020

Conversation

meg-gupta
Copy link
Contributor

@meg-gupta meg-gupta commented Nov 13, 2020

Fixes rdar://71290925

@meg-gupta
Copy link
Contributor Author

@swift-ci test

@slavapestov
Copy link
Contributor

Should we instead try to fix this to work? What goes wrong if such a witness_method instruction is generated?

@aschwaighofer
Copy link
Contributor

We hit the SIL verifier not liking such a witness method instruction. It only wants opened archetype type operands.

void checkWitnessMethodInst(WitnessMethodInst *AMI) {  
    auto lookupType = AMI->getLookupType();                                     
    if (getOpenedArchetypeOf(lookupType)) {                                     
      require(AMI->getTypeDependentOperands().size() == 1,                      
              "Must have a type dependent operand for the opened archetype");   
      verifyOpenedArchetype(AMI, lookupType);                                   
    } else {                                                                    
      require(AMI->getTypeDependentOperands().empty(),                          
              "Should not have an operand for the opened existential");         
    } 

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - de957f86e97716e4304bc84a810972a67003d600

@meg-gupta meg-gupta changed the title Fix SILCombine to avoid generating witness_method for dynamic self type Fix SILVerifier assert for witness_method with dynamic self type Nov 14, 2020
@meg-gupta
Copy link
Contributor Author

I changed what this PR was initially intended for. Turns out we do support witness_method with dynamic_self. The SILVerifier assertions for witness_method needed to be changed

@meg-gupta
Copy link
Contributor Author

@swift-ci smoke test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - de957f86e97716e4304bc84a810972a67003d600

@meg-gupta
Copy link
Contributor Author

@swift-ci smoke test OS X platform

@meg-gupta
Copy link
Contributor Author

@swift-ci smoke test Windows Platform

@slavapestov
Copy link
Contributor

Thanks, changing the verifier here makes sense. We do support witness_method on concrete types just fine (either in IRGen, or the devirtualizer if that gets to run)

@meg-gupta
Copy link
Contributor Author

@swift-ci smoke test

@meg-gupta meg-gupta merged commit 744767b into swiftlang:main Nov 16, 2020
ainu-bot added a commit to google/swift that referenced this pull request Nov 16, 2020
* 'main' of github.com:apple/swift: (89 commits)
  Fix SILVerifier assert for witness_method with dynamic self type (swiftlang#34738)
  Allow a borrowed address to be passed as in_guaranteed arg in partial_apply [on_stack] only (swiftlang#34736)
  Fix test for windows/linux
  IRGen: Fix debug emission for dynamically sized stack vars
  [Testing] Add missing REQUIRES: concurrency
  [ownership] Make SILUndef always have ValueOwnershipKind::None.
  [value-lifetime] Cleanup constructors.
  [ownership] Centralize the concept of isReborrow on BorrowingOperand.
  [sil][value-lifetime] Add ValueLifetimeAnalysis::FrontierImpl = SmallVectorImpl<SILInstruction *>
  [Test] Disable partial_apply_forwarder.sil for arm64e.
  [Async CC] Unroll workaround where @main was async.
  Remove test that depends on Swift.Int metadata, which isn't around.
  [Concurrency] More cleanups for futures
  [Concurrency] Remove an unnecessary 'const' from a non-static data member.
  [Concurrency] Minor cleanups to futures.
  [Concurrency] Have future functions write their results directly.
  [Concurrency] Clarify/fix error object handling in futures.
  [Concurrency] Inline AsyncTask::FutureFragment::fragmentSize.
  [Concurrency] Turn "simple task" into just "task".
  [Concurrency] Use SchedulerPrivate for the "next waiting task" link.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants