Reland "[modules] Fix miscompilation when using two RecordDecl definitions with the same name."

[vsapsai]

This reverts commit a0423cc and
re-applies commit 9454716.

After adding isThisDeclarationADemotedDefinition API now swift can
query if a declaration was a definition but was demoted. This way it can
keep accepting RecordDecl that were merged across different modules.

rdar://86548228

[vsapsai]

@swift-ci test

[vsapsai]

The plan is to re-land this change on "next" but keep it out of release-specific branches for now. When we fix known projects relying on using definitions from multiple modules, this change is ready for release.

If you see problems with the change or with the plan, I appreciate your feedback.

[etcwilde]

I'll take a look shortly. From a quick glance, it looks like the same patch.
From Swift's perspective, the clang-importer has to trust what the clang AST says. If the clang AST says that there isn't a definition in a given module, then it has to go with that being true, not that there is a definition and clang has just decided not to use it. Like the modules in C++20, a definition can be accessible via different modules.

The example below is why the patch was reverted last time:

              ┌───────────────┐          ┌ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─
              │   pthread.h   │◀─ ─ ─ ─ ─    pthread_mutex_t    │
              └───────────────┘          └ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─
                      │
                      │
        ┌─────────────┴───────────┐
        │                         │
        │                         │
        ▼                         ▼
 ┌────────────┐        ┌────────────────────┐
 │   GlibC    │        │     CDispatch      │
 └────────────┘        └────────────────────┘
        │                         │
        │                         │
        │                         ▼
        │              ┌────────────────────┐
        │              │      Dispatch      │
        │              └────────────────────┘
        └─────────────┐           │
                      │           │
                      │           │
                      ├───────────┘
                      │
                      │
                      │
                      ▼
         ┌────────────────────────┐
         │    MyProgram.swift     │
         └────────────────────────┘

pthread_mutex_t is declared and defined in pthread.h. Both GlibC and CDispatch include pthread.h, and therefore have a definition of pthread_mutex_t. Clang decided that the definition to use was the one "in" CDispatch, so you would have to use CDispatch.pthread_mutex_t to use the type. Minor perturbations of the program would result in a different ordering. Since the mechanism for deciding which definition to use is completely opaque to a programmer, this results in unexpected source-breaking changes. Swift won't export the definition from modules that report that they don't have a definition though. If clang lies about whether a module actually has a definition, then it will not be included in the listing of modules containing the definition. We just need to know whether the module actually has a definition or not. Not whether that's the definition clang will use for compiling Objective-C code.

[vsapsai]

How does CodeGen for CDispatch.pthread_mutex_t work? Does it go through clang's CodeGen (aka IRGen)? This change is important for clang's IRGen because it has expectations about clang's AST and that it maintains its invariants. But if Swift generates it's own SIL (and subsequently IR) for CDispatch.pthread_mutex_t, I understand your argument that Swift doesn't care about clang's AST.

[etcwilde]

Right, Swift will turn the type into a Swift type and generate it like it would any other Swift struct. We really just need to know whether the module has a definition and what that definition is.

[vsapsai]

That's good to know. I'm thinking if ClangImporter should import each module independently. Because when it is one big AST on clang side, it is tricky to maintain duplicate definitions and types. We are planning to perform in clang more decl and type merging because without them we are hitting redefinition errors and ambiguous lookup errors. And I'm afraid that merging isn't particularly useful for Swift but can cause more problems. So it is important to have some mechanism for Swift that deals with modules independently.

And we'll need to check with LLDB folks to make sure any solution we come up with is compatible with expression evaluation.

[vsapsai]

Inviting more people as my proposal gets more ambitious and I want folks to catch unintended consequences.

So, the problem is that we'd like Swift to import Clang modules more independently. Multiple definitions with the same name in different clang modules isn't a problem for swift because those definitions are kept separate and we are relying on swift name lookup to resolve any ambiguities. Ambiguities on clang side aren't a problem because swift doesn't use clang name lookup at this point.

Now we are facing the question of how to achieve the separation between imported clang modules. My idea was for ClangImporter to use separate ASTContext+Sema for each clang module imported directly from swift (and its transitive dependencies, I guess). Is it possible? What downsides are associated with such approach?

An alternative is to have some flag that prevents clang from merging entities with the same name. So far I've discarded this approach because we still need to detect duplicates in Objective-C code and I don't know how the flag can tell us which duplicates are OK to ignore and which deserve diagnostic. And this approach seems to be fragile and error-prone as we need to maintain real AST and implicit AST-for-Swift. Also I'm not sure we'll be able to use this mechanism with C++ modules.

And the question to Adrian (or anybody from LLDB team that Adrian believes can help) - does LLDB use clang's CodeGen for evaluating Swift code? Or does it take imported Swift types and evaluates that relying on Swift's SILGen + subsequent machine code generation?

[vsapsai]

The plan now is not to import clang modules separately but to detect and accept definitions that were demoted to declarations. Swift-side change is swiftlang/swift#41144

[vsapsai]

@swift-ci test

[vsapsai]

@swift-ci test

[vsapsai]

@swift-ci test

[vsapsai]

I've rebased the PR on top of "stable/20211026" instead of "next" as PR testing for "next" is broken.

Also Evan has asked if this revert-of-revert is the same as the original commit and I've added an inline comment for a single line that I've changed in this re-landing.

[vsapsai]

Compared to the original patch I've changed this line from RD->setCompleteDefinition(false); to RD->demoteThisDefinitionToDeclaration();

[etcwilde]

I think this looks fine.
I've put up swiftlang/swift-corelibs-foundation#3149 to check that foundation still builds correctly since that was where I detected the issue in the first place.
LGTM once that passes.

[etcwilde]

Alright, swiftlang/swift-corelibs-foundation#3149 passed. LGTM.

[vsapsai]

Thanks for the review! I'll keep an eye on any potential problems in case we need to revert it again.