Skip to content

[lldb] Don't recursively load types of static member variables in the… #2971

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

Conversation

Teemperor
Copy link

… DWARF AST parser

When LLDB's DWARF parser is parsing the member DIEs of a struct/class it
currently fully resolves the types of static member variables in a class before
adding the respective VarDecl to the record.

For record types fully resolving the type will also parse the member DIEs of the
respective class. The other way of resolving is just 'forward' resolving the type
which will try to load only the minimum amount of information about the type
(for records that would only be the name/kind of the type). Usually we always
resolve types on-demand so it's rarely useful to speculatively fully resolve
them on the first use.

This patch changes makes that we only 'forward' resolve the types of static
members. This solves the fact that LLDB unnecessarily loads debug information
to parse the type if it's maybe not needed later and it also avoids a crash where
the parsed type might in turn reference the surrounding class that is currently
being parsed.

The new test case demonstrates the crash that might happen. The crash happens
with the following steps:

  1. We parse class ToLayout and it's members.

  2. We parse the static class member and fully resolve its type
    (DependsOnParam2<ToLayout>).

  3. That type has a non-static class member DependsOnParam1<ToLayout> for which
    LLDB will try to calculate the size.

  4. The layout (and size)DependsOnParam1<ToLayout> turns depends on the
    ToLayout size/layout.

  5. Clang will calculate the record layout/size for ToLayout even though we are
    currently parsing it and it's missing it's non-static member.

The created is missing the offset for the yet unparsed non-static member. If we
later try to get the offset we end up hitting different asserts. Most common is
the one in TypeSystemClang::DumpValue where it checks that the record layout
has offsets for the current FieldDecl.

        assert(field_idx < record_layout.getFieldCount());

Fixed rdar://67910011

Reviewed By: shafik

Differential Revision: https://reviews.llvm.org/D100180

(cherry picked from commit 34c697c)

… DWARF AST parser

When LLDB's DWARF parser is parsing the member DIEs of a struct/class it
currently fully resolves the types of static member variables in a class before
adding the respective `VarDecl` to the record.

For record types fully resolving the type will also parse the member DIEs of the
respective class. The other way of resolving is just 'forward' resolving the type
which will try to load only the minimum amount of information about the type
(for records that would only be the name/kind of the type). Usually we always
resolve types on-demand so it's rarely useful to speculatively fully resolve
them on the first use.

This patch changes makes that we only 'forward' resolve the types of static
members. This solves the fact that LLDB unnecessarily loads debug information
to parse the type if it's maybe not needed later and it also avoids a crash where
the parsed type might in turn reference the surrounding class that is currently
being parsed.

The new test case demonstrates the crash that might happen. The crash happens
with the following steps:

1. We parse class `ToLayout` and it's members.

2. We parse the static class member and fully resolve its type
(`DependsOnParam2<ToLayout>`).

3. That type has a non-static class member `DependsOnParam1<ToLayout>` for which
LLDB will try to calculate the size.

4. The layout (and size)`DependsOnParam1<ToLayout>` turns depends on the
`ToLayout` size/layout.

5. Clang will calculate the record layout/size for `ToLayout` even though we are
currently parsing it and it's missing it's non-static member.

The created is missing the offset for the yet unparsed non-static member. If we
later try to get the offset we end up hitting different asserts. Most common is
the one in `TypeSystemClang::DumpValue` where it checks that the record layout
has offsets for the current FieldDecl.

```
        assert(field_idx < record_layout.getFieldCount());
```

Fixed rdar://67910011

Reviewed By: shafik

Differential Revision: https://reviews.llvm.org/D100180

(cherry picked from commit 34c697c)
@Teemperor
Copy link
Author

@swift-ci test

Copy link

@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.

Fewer LayoutCompilerTypes looks good to me!

@Teemperor Teemperor merged commit ab6d133 into swiftlang:apple/stable/20210107 May 18, 2021
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.

2 participants