Skip to content

[DebugInfo] Emit witness and objc method declarations in debug info #71965

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
Mar 1, 2024

Conversation

augusto2112
Copy link
Contributor

When emitting a method definition in debug info, the compiler should also emit the method's declaration, because LLVM LTO can't unify type definitions when a child DIE is a full subprogram definition. This is already the behavior for standard methods, this patch implements the same behavior for witness and objc methods as well.

rdar://123334375

@augusto2112
Copy link
Contributor Author

@swift-ci smoke test

Copy link
Contributor

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find!

if (Rep == SILFunctionTypeRepresentation::Method) {
if (Rep == SILFunctionTypeRepresentation::Method ||
Rep == SILFunctionTypeRepresentation::ObjCMethod ||
Rep == SILFunctionTypeRepresentation::WitnessMethod) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity — what other representations are there and do they all not appear as children of a DICompositeType?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the entire list:

enum class SILFunctionTypeRepresentation : uint8_t {
  /// A freestanding thick function.
  Thick = uint8_t(FunctionTypeRepresentation::Swift),

  /// A thick function that is represented as an Objective-C block.
  Block = uint8_t(FunctionTypeRepresentation::Block),

  /// A freestanding thin function that needs no context.
  Thin = uint8_t(FunctionTypeRepresentation::Thin),

  /// A C function pointer, which is thin and also uses the C calling
  /// convention.
  CFunctionPointer = uint8_t(FunctionTypeRepresentation::CFunctionPointer),

  /// The value of the greatest AST function representation.
  LastAST = CFunctionPointer,

  /// The value of the least SIL-only function representation.
  FirstSIL = 8,

  /// A Swift instance method.
  Method = FirstSIL,

  /// An Objective-C method.
  ObjCMethod,

  /// A Swift protocol witness.
  WitnessMethod,

  /// A closure invocation function that has not been bound to a context.
  Closure,

  /// A C++ method that takes a "this" argument (not a static C++ method or
  /// constructor). Except for
  /// handling the "this" argument, has the same behavior as "CFunctionPointer".
  CXXMethod,

  /// A KeyPath accessor function, which is thin and also uses the variadic
  /// length generic components serialization in trailing buffer.
  /// Each representation has a different convention for which parameters
  /// have serialized generic type info.
  KeyPathAccessorGetter,
  KeyPathAccessorSetter,
  KeyPathAccessorEquals,
  KeyPathAccessorHash,
}

Could a CXXMethod conceivably show up here? I added an assertion to check for that and ran the swift test suite, but didn't encounter any such cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the inlining problem could happen we probably need to do this for every function that has a DICompositeType as parent.

@adrian-prantl
Copy link
Contributor

@swift-ci test source compatibility

When emitting a method definition in debug info, the compiler should
also emit the method's declaration, because LLVM LTO can't unify type
definitions when a child DIE is a full subprogram definition. This is
already the behavior for standard methods, this patch implements the
same behavior for witness and objc methods as well.

rdar://123334375
@augusto2112
Copy link
Contributor Author

@swift-ci smoke test

@augusto2112
Copy link
Contributor Author

@swift-ci test source compatibility

Copy link
Contributor

@felipepiovezan felipepiovezan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Slightly surprised this didn't break earlier!

@augusto2112
Copy link
Contributor Author

LGTM! Slightly surprised this didn't break earlier!

I don't think lto is often used when compiling Swift :)

@adrian-prantl
Copy link
Contributor

I'm going to merge this regardless, but I think we need to add more cases.

@adrian-prantl adrian-prantl merged commit 8a25294 into swiftlang:main Mar 1, 2024
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.

3 participants