[SR-6468] inheriting from class with synthesized Codable implementation creates invalid code

[swift-ci]

Previous ID	SR-6468
Radar	rdar://problem/35699277
Original Reporter	derammo (JIRA User)
Type	Bug
Status	Resolved
Resolution	Done

Attachment: Download

Environment

swift-4.1-DEVELOPMENT-SNAPSHOT-2017-11-23-a.xctoolchain

also happens in Xcode 9.1 as released by Apple:

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Bug, Codable
Assignee	@slavapestov
Priority	Medium

md5: 44a3722f047bd288f0a7c1c8573bee7e

is duplicated by:

• SR-6507 Encodable in combination with class inheritance and computed properties generates unexpected behaviour
• SR-7090 Crashing with EXC_BAD_ACCESS after decoding Codable subclass
• SR-7156 Synthesized instance methods result in vtable-related crashes

Issue Description:

Inheriting from a class that has some of its "Codable" protocol implementation synthesized automatically can lead to invalid code. Specifically, the derived class ends up with code that accesses out of the bounds of the table of methods when trying to invoke a method on an instance of the derived class.

I verified this by disassembly. Instead of fetching the address of the first getter, it fetches a small number (which seems to be the table size) that is located 8 bytes to the left of the correct information. Then it calls this address, which obviously faults.

In this example, you see if it fetching the 0x18 (presumably table size) instead of the valid address of the getter 0x0106bdf460, which is stored right after it.

(lldb) dis -l

-> 10      func testInheritedGeneratedInitializerFromCodableSynthesizer() {
-> 11          let instance = TestPartiallySynthesizedDerivedClass(arg1: 1, arg2: 2)
-> 12          let getting = instance.derivedMember

testUnit`testCompiler.testInheritedGeneratedInitializerFromCodableSynthesizer():
->  0x106bb6af9 <+41>: movq   (%rax), %rsi
    0x106bb6afc <+44>: movq   0xb0(%rsi), %rsi
    0x106bb6b03 <+51>: movq   %rax, %r13
    0x106bb6b06 <+54>: movq   %rax, -0x18(%rbp)
    0x106bb6b0a <+58>: callq  *%rsi
(lldb) nexti
(lldb) register read rsi
     rsi = 0x0000000106c46550  type metadata for testUnit.TestPartiallySynthesizedDerivedClass
(lldb) memory read 0x106C46600
0x106c46600: 18 00 00 00 00 00 00 00 60 f4 bd 06 01 00 00 00  ........`.......
0x106c46610: b0 f4 bd 06 01 00 00 00 30 f5 bd 06 01 00 00 00  ........0.......
(lldb) dis -a 0106bdf460
testUnit`_T08testUnit36TestPartiallySynthesizedDerivedClassC13derivedMemberSbvg:
    0x106bdf460 <+0>:  pushq  %rbp
    0x106bdf461 <+1>:  movq   %rsp, %rbp
    0x106bdf464 <+4>:  subq   $0x40, %rsp
    0x106bdf468 <+8>:  movq   %r13, -0x8(%rbp)
    0x106bdf46c <+12>: movq   %r13, %rax
    0x106bdf46f <+15>: addq   $0x20, %rax
    0x106bdf473 <+19>: xorl   %ecx, %ecx
    0x106bdf475 <+21>: movl   %ecx, %edx
    0x106bdf477 <+23>: leaq   -0x20(%rbp), %rsi
    0x106bdf47b <+27>: movq   %rax, %rdi
    0x106bdf47e <+30>: movq   %rsi, -0x28(%rbp)
    0x106bdf482 <+34>: movq   %rdx, -0x30(%rbp)
    0x106bdf486 <+38>: movq   -0x30(%rbp), %rcx
    0x106bdf48a <+42>: movq   %r13, -0x38(%rbp)
    0x106bdf48e <+46>: callq  0x106c278de               ; symbol stub for: swift_beginAccess
    0x106bdf493 <+51>: movq   -0x38(%rbp), %rax
    0x106bdf497 <+55>: movb   0x20(%rax), %al
    0x106bdf49a <+58>: movq   -0x28(%rbp), %rdi
    0x106bdf49e <+62>: movb   %al, -0x39(%rbp)
    0x106bdf4a1 <+65>: callq  0x106c2790e               ; symbol stub for: swift_endAccess
    0x106bdf4a6 <+70>: movb   -0x39(%rbp), %al
    0x106bdf4a9 <+73>: addq   $0x40, %rsp
    0x106bdf4ad <+77>: popq   %rbp
    0x106bdf4ae <+78>: retq   

The problem is reliably reproduced but extremely specific. Adding explicit support for Codable in the base class makes it go away. Adding explicit support for Codable in the derived class (by adding decode(from) also) also makes it go away. The classes must be in a separate file from the caller for this to fail.

The attached files reproduce the problem 100% of the time as a unit test.

[belkadan]

@itaiferber, any idea what's going on here? I know we've had problems here but I didn't expect crashes!

[itaiferber]

@belkadan I'm not sure, to be honest. This is the first I've seen of this, so I'll investigate. It looks like inheriting the superclass's method is somehow invalid? That seems really strange to me.

[itaiferber]

@swift-ci Create

[swift-ci]

Comment by Ammo Goettsch (JIRA)

I saw it again in another place in the code yesterday, where I was inheriting a class. Once I ported the base class code to use Codable (with some synthesized code), it started calling the wrong method in the derived class in a test (i.e. wrong offset again, this time hitting an actual method rather than the end of the table.) So I don't think it as rare as I thought.

[belkadan]

Just to double-check, you are rebuilding the test after changing the codable classes, right?

[swift-ci]

Comment by Ammo Goettsch (JIRA)

Oh yes: clean, exit Xcode, rm -rf ~/Library/Developer/Xcode/DerivedData

I even sometimes reboot to clear out /var/folders where Xcode makes a mess... but I can't swear I have done that for this test. The attached test code should produce the problem though for you. Let me know if it does not repro and I can work with you to find the settings I might be using differently or send you a project file.

[itaiferber]

@belkadan Thanks for consolidating! Who might best be able to help me investigate what's happening with the vtable here (or help point in the right direction)? Encodable synthesis isn't doing anything special and follows the same model as Hashable and Equatable.

[lorentey]

Encodable is special in that it is currently the only protocol for which we synthesize methods in class types. I'm also interested in finding a good solution to this – I ran into the same issue when I tried adding limited support for classes for Hashable synthesis in PR #15122.

[belkadan]

I shoved @slavapestov at one of the dups but I don't know if he's started looking yet. In our brief conversations about it we haven't thought of anything better than "if you use a class, you must validate its Hashable conformance".

[itaiferber]

@belkadan What do you mean by "validate its conformance"? Don't we already have passes that validate the decls? Or is this at a different layer/level?

[belkadan]

I mean it's not good enough to know that the class declares conformance to Hashable; we need to actually go check that it does so correctly, because that's what forces the new overridable declarations to be synthesized.

[lorentey]

FWIW, I went for a little Mr Magoo-style adventure in the type checker, and I was excited to stumble upon TypeChecker::handleExternalDecl. Unfortunately I was distracted by Other Stuff before I had time to break anything. 🙂

[itaiferber]

@belkadan That's what I'm saying — the type checker already has passes that validate conformances; we do that on the second pass:

// lib/Sema/TypeCheckDecl.cpp:4631
void visitClassDecl(ClassDecl *CD) {
  TC.checkDeclAttributesEarly(CD);
  TC.computeAccessLevel(CD);

  if (!IsSecondPass) {
    checkUnsupportedNestedType(CD);

    TC.validateDecl(CD);
    if (!CD->hasValidSignature())
      return;

    TC.requestSuperclassLayout(CD);
    TC.DeclsToFinalize.remove(CD);

    {
      // Check for circular inheritance.
      SmallVector<ClassDecl *, 8> path;
      path.push_back(CD);
      checkCircularity(TC, CD, diag::circular_class_inheritance,
                       diag::class_here, path);
    }
  }

  // If this class needs an implicit constructor, add it.
  if (!IsFirstPass)
    TC.addImplicitConstructors(CD);

  CD->addImplicitDestructor();

  if (!IsFirstPass && !CD->isInvalid())
    TC.checkConformancesInContext(CD, CD);

  for (Decl *Member : CD->getMembers())
    visit(Member);

  // <snip>

  TC.checkDeclAttributes(CD);
}

Or am I still misunderstanding?

(I'd try to step through here but unfortunately my machine can't seem to build Swift at the moment)

[lorentey]

@itaiferber I believe external decls aren't fully type checked.

[itaiferber]

@lorentey Ah; that might do it. If the class is defined in a separate file it's considered "external" and thus might not be fully validated?