[lldb][Expression] Printing union with self-referencing member field crashes lldb #68135
Comments
|
@llvm/issue-subscribers-lldb
```
//struct DependentDylibAttributes // << this works
union DependentDylibAttributes // << this crashes
{
static const DependentDylibAttributes regular;
};
const DependentDylibAttributes DependentDylibAttributes::regular{}; int main() { clang++ dyld.cpp -g -O0 -std=c++2a |
|
Oh looks like for unions we're mistakenly adding an |
This is what we do for structures that have static members. But this doesn't happen for unions. Because of this: |
|
That logic was added in: aaae5f8 |
|
So it looks like the change in aaae5f8 directly conflicts with the change in #55040. The latter makes it so we always check the presence of The effect of this is that LLDB never correctly detects that a union has static data members. From both commit descriptions it sounds like both changes were done to support GCC (it neither emits a AFAICT, the change in aaae5f8 is only necessary so that we stop claiming non-static data members are static for GCC-built binaries. I see that @clayborg @jingham Any preference on what to do here? Looks like GCC still isn't attaching CC @dwblaikie @adrian-prantl @labath because DWARF |
|
As an aside, do we need to emit the |
|
Hmmm looks like DWARFv5 says we should emit static data members as
That would make it much easier to differentiate static vs non-static members. Don't think clang does this yet (gcc does) |
|
So to summarise:
|
If the spec says so, I think that's an easy yes. You might need to make the behavior adjustable for some debugger tunings (though I'd argue the default should emit standards-conformant DWARF). |
|
CC @dwblaikie |
|
Summary is, if I'm reading this right:
So we're discussing changing teh way clang emits static members to use And you've tagged me as a heads up that we might be/are going to go in this direction/check I'm cool with it - yeah, sounds good to me. Doing things more clearly, and if that happens to be "more like GCC" I'm generally for it - they're still the bigger fish in the DWARF sea, and it's usually a safe direction to head unless there's extenuating circumstances. At least our internal use ships pretty close to LLDB, so we don't have a particular need for some clang mode that's deeply backwards compatible (the inverse is somewhat true - be good if LLDB didn't just totally give up on doing some of the old heuristics - still able to debug old binaries as best it can, which seems like it wouldn't be too expensive/problematic to maintain for a while at least) & this shouldn't present a GDB compatibility issue, since it'll be closer to what GCC produces anyway. |
Yup! those are exactly the issues Alright sounds like we'll resolve this by simply implementing the DWARFv5 " To avoid crashing on older DWARF i think we could add a best effort attempt to detect the cases where we might have a static data member in a union and do the right thing. E.g., by adding back the check for |
…ic data members **Background** Prior to DWARFv4, there was no clear normative text on how to handle static data members. Non-normative text suggested we compilers should use `DW_AT_external` to mark static data members of structrues/unions. Clang does this consistently. However, GCC doesn't, e.g., when the structure/union is in an anonymous namespace (which is C++ standard conformant). Additionally, GCC never emits `DW_AT_data_member_location`s for union members (regardless of storage linkage and storage duration). Since DWARFv5 (issue 161118.1), static data members get emitted as `DW_TAG_variable`. LLDB used to differentiate between static and non-static members by checking the `DW_AT_external` flag and the absence of `DW_AT_data_member_location`. With D18008 LLDB started to pretend that union members always have a `0` `DW_AT_data_member_location` by default (because GCC never emits these locations). In D124409 LLDB stopped checking the `DW_AT_external` flag to account for the case where GCC doesn't emit the flag for types in anonymous namespaces; instead we only check for presence of `DW_AT_data_member_location`s. The combination of these changes then meant that LLDB would never correctly detect that a union has static data members. **Solution** Instead of unconditionally initializing the `member_byte_offset` to `0` specifically for union members, this patch proposes to check for both the absence of `DW_AT_data_member_location` and `DW_AT_declaration`, which consistently gets emitted for static data members on GCC and Clang. We initialize the `member_byte_offset` to `0` anyway if we determine it wasn't a static. So removing the special case for unions makes this code simpler to reason about. Long-term, we should just use DWARFv5's new representation for static data members. Fixes llvm#68135
|
Proposed solution to handle pre-DWARFv5 here: #68300 |
…ic data members (#68300) **Background** Prior to DWARFv4, there was no clear normative text on how to handle static data members. Non-normative text suggested that compilers should use `DW_AT_external` to mark static data members of structrues/unions. Clang does this consistently. However, GCC doesn't, e.g., when the structure/union is in an anonymous namespace (which is C++ standard conformant). Additionally, GCC never emits `DW_AT_data_member_location`s for union members (regardless of storage linkage and storage duration). Since DWARFv5 (issue 161118.1), static data members get emitted as `DW_TAG_variable`. LLDB used to differentiate between static and non-static members by checking the `DW_AT_external` flag and the absence of `DW_AT_data_member_location`. With [D18008](https://reviews.llvm.org/D18008) LLDB started to pretend that union members always have a `0` `DW_AT_data_member_location` by default (because GCC never emits these locations). In [D124409](https://reviews.llvm.org/D124409) LLDB stopped checking the `DW_AT_external` flag to account for the case where GCC doesn't emit the flag for types in anonymous namespaces; instead we only check for presence of `DW_AT_data_member_location`s. The combination of these changes then meant that LLDB would never correctly detect that a union has static data members. **Solution** Instead of unconditionally initializing the `member_byte_offset` to `0` specifically for union members, this patch proposes to check for both the absence of `DW_AT_data_member_location` and `DW_AT_declaration`, which consistently gets emitted for static data members on GCC and Clang. We initialize the `member_byte_offset` to `0` anyway if we determine it wasn't a static. So removing the special case for unions makes this code simpler to reason about. Long-term, we should just use DWARFv5's new representation for static data members. Fixes #68135
…ic data members (llvm#68300) **Background** Prior to DWARFv4, there was no clear normative text on how to handle static data members. Non-normative text suggested that compilers should use `DW_AT_external` to mark static data members of structrues/unions. Clang does this consistently. However, GCC doesn't, e.g., when the structure/union is in an anonymous namespace (which is C++ standard conformant). Additionally, GCC never emits `DW_AT_data_member_location`s for union members (regardless of storage linkage and storage duration). Since DWARFv5 (issue 161118.1), static data members get emitted as `DW_TAG_variable`. LLDB used to differentiate between static and non-static members by checking the `DW_AT_external` flag and the absence of `DW_AT_data_member_location`. With [D18008](https://reviews.llvm.org/D18008) LLDB started to pretend that union members always have a `0` `DW_AT_data_member_location` by default (because GCC never emits these locations). In [D124409](https://reviews.llvm.org/D124409) LLDB stopped checking the `DW_AT_external` flag to account for the case where GCC doesn't emit the flag for types in anonymous namespaces; instead we only check for presence of `DW_AT_data_member_location`s. The combination of these changes then meant that LLDB would never correctly detect that a union has static data members. **Solution** Instead of unconditionally initializing the `member_byte_offset` to `0` specifically for union members, this patch proposes to check for both the absence of `DW_AT_data_member_location` and `DW_AT_declaration`, which consistently gets emitted for static data members on GCC and Clang. We initialize the `member_byte_offset` to `0` anyway if we determine it wasn't a static. So removing the special case for unions makes this code simpler to reason about. Long-term, we should just use DWARFv5's new representation for static data members. Fixes llvm#68135 (cherry picked from commit f74aaca)
…ic data members (llvm#68300) **Background** Prior to DWARFv4, there was no clear normative text on how to handle static data members. Non-normative text suggested that compilers should use `DW_AT_external` to mark static data members of structrues/unions. Clang does this consistently. However, GCC doesn't, e.g., when the structure/union is in an anonymous namespace (which is C++ standard conformant). Additionally, GCC never emits `DW_AT_data_member_location`s for union members (regardless of storage linkage and storage duration). Since DWARFv5 (issue 161118.1), static data members get emitted as `DW_TAG_variable`. LLDB used to differentiate between static and non-static members by checking the `DW_AT_external` flag and the absence of `DW_AT_data_member_location`. With [D18008](https://reviews.llvm.org/D18008) LLDB started to pretend that union members always have a `0` `DW_AT_data_member_location` by default (because GCC never emits these locations). In [D124409](https://reviews.llvm.org/D124409) LLDB stopped checking the `DW_AT_external` flag to account for the case where GCC doesn't emit the flag for types in anonymous namespaces; instead we only check for presence of `DW_AT_data_member_location`s. The combination of these changes then meant that LLDB would never correctly detect that a union has static data members. **Solution** Instead of unconditionally initializing the `member_byte_offset` to `0` specifically for union members, this patch proposes to check for both the absence of `DW_AT_data_member_location` and `DW_AT_declaration`, which consistently gets emitted for static data members on GCC and Clang. We initialize the `member_byte_offset` to `0` anyway if we determine it wasn't a static. So removing the special case for unions makes this code simpler to reason about. Long-term, we should just use DWARFv5's new representation for static data members. Fixes llvm#68135 (cherry picked from commit f74aaca)
(on llvm.org lldb build at commit
0bfaed8c612705cfa8c5382d26d8089a0a26386b)This will infinitely recurse in
ASTContext::getASTRecordLayout. Possibly the same crash as:expr &C.pImpl->DoubleTy#64628clang::TemplateDeclobject #66335The text was updated successfully, but these errors were encountered: