Skip to content

[lldb][dwarf] Compute fully qualified names on simplified template names with DWARFTypePrinter #112811

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.

Sign up for GitHub

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 4 commits into from
Nov 18, 2024
Merged

[lldb][dwarf] Compute fully qualified names on simplified template names with DWARFTypePrinter #112811

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3fc0675
[lldb][dwarf] Compute fully qualified names on simplified template na…
ZequanWu Oct 10, 2024
38a459b
Address comments.
ZequanWu Nov 7, 2024
769c4fb
revert removal of DWARFBaseDIE::IsValid() api
ZequanWu Nov 7, 2024
58605dc
Add a unittest for DWARFTypePrinter and update shell test.
ZequanWu Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Type.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/DebugInfo/DWARF/DWARFTypePrinter.h"
#include "llvm/Demangle/Demangle.h"

#include <map>
Expand Down Expand Up @@ -825,11 +826,11 @@ std::string DWARFASTParserClang::GetDIEClassTemplateParams(DWARFDIE die) {
if (llvm::StringRef(die.GetName()).contains("<"))
return {};

TypeSystemClang::TemplateParameterInfos template_param_infos;
if (ParseTemplateParameterInfos(die, template_param_infos))
return m_ast.PrintTemplateParams(template_param_infos);

return {};
std::string name;
llvm::raw_string_ostream os(name);
llvm::DWARFTypePrinter<DWARFDIE> type_printer(os);
type_printer.appendAndTerminateTemplateParameters(die);
return name;
}

void DWARFASTParserClang::MapDeclDIEToDefDIE(
Expand Down Expand Up @@ -1617,9 +1618,9 @@ void DWARFASTParserClang::GetUniqueTypeNameAndDeclaration(
case DW_TAG_structure_type:
case DW_TAG_union_type: {
if (const char *class_union_struct_name = parent_decl_ctx_die.GetName()) {
qualified_name.insert(
0, GetDIEClassTemplateParams(parent_decl_ctx_die));
qualified_name.insert(0, "::");
qualified_name.insert(0,
GetDIEClassTemplateParams(parent_decl_ctx_die));
qualified_name.insert(0, class_union_struct_name);
}
parent_decl_ctx_die = parent_decl_ctx_die.GetParentDeclContextDIE();
Expand Down Expand Up @@ -1672,6 +1673,12 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
if (attrs.name) {
GetUniqueTypeNameAndDeclaration(die, cu_language, unique_typename,
unique_decl);
if (log) {
dwarf->GetObjectFile()->GetModule()->LogMessage(
log, "SymbolFileDWARF({0:p}) - {1:x16}: {2} has unique name: {3} ",
static_cast<void *>(this), die.GetID(), DW_TAG_value_to_name(tag),
unique_typename.AsCString());
}
if (UniqueDWARFASTType *unique_ast_entry_type =
dwarf->GetUniqueDWARFASTTypeMap().Find(
unique_typename, die, unique_decl, byte_size,
Expand Down
7 changes: 7 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ class DWARFUnit;
class DWARFDebugInfoEntry;
class DWARFDeclContext;
class SymbolFileDWARF;
class DWARFFormValue;

class DWARFBaseDIE {
public:
using DWARFFormValue = dwarf::DWARFFormValue;
DWARFBaseDIE() = default;

DWARFBaseDIE(DWARFUnit *cu, DWARFDebugInfoEntry *die)
Expand All @@ -46,6 +48,7 @@ class DWARFBaseDIE {
explicit operator bool() const { return IsValid(); }

bool IsValid() const { return m_cu && m_die; }
bool isValid() const { return IsValid(); }
Copy link
Collaborator

@labath labath Nov 5, 2024

Choose a reason for hiding this comment

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

How about using operator bool as the common api ?

Copy link
Contributor Author

@ZequanWu ZequanWu Nov 7, 2024

Choose a reason for hiding this comment

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

Removed both IsValid() and IsValid() and switched to use operator bool ().

Copy link
Collaborator

@labath labath Nov 7, 2024

Choose a reason for hiding this comment

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

I'm sorry, I guess I wasn't clear about this. What I was suggesting is to just revert the addition isValid -- as there already exists a comon API. I didn't want to remove things that are already there. (I mean, I'd be fine with that doing that under the flag of moving the LLDB API closer to LLVM, but I don't think we ought to do that as a part of this PR)


bool HasChildren() const;

Expand Down Expand Up @@ -85,6 +88,8 @@ class DWARFBaseDIE {
// Accessing information about a DIE
dw_tag_t Tag() const;

dw_tag_t getTag() const { return Tag(); }

dw_offset_t GetOffset() const;

// Get the LLDB user ID for this DIE. This is often just the DIE offset,
Expand All @@ -95,6 +100,8 @@ class DWARFBaseDIE {

const char *GetName() const;

const char *getShortName() const { return GetName(); }
Copy link
Member

@Michael137 Michael137 Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: How do people feel about grouping all the new APIs that are required for LLVM compatbility together and adding a comment stating why we have them?

Copy link
Contributor Author

@ZequanWu ZequanWu Nov 7, 2024

Choose a reason for hiding this comment

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

Grouped those new APIs for each lldb class and added a comment for it.

Copy link
Collaborator

@labath labath Nov 7, 2024

Choose a reason for hiding this comment

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

(I was considering the same thing, but I don't particularly care either way)


lldb::ModuleSP GetModule() const;

// Getting attribute values from the DIE.
Expand Down
35 changes: 35 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ elaborating_dies(const DWARFDIE &die) {
}
} // namespace

std::optional<uint64_t> DWARFDIE::getLanguage() const {
if (IsValid())
return m_cu->GetDWARFLanguageType();
return std::nullopt;
}

DWARFDIE
DWARFDIE::GetParent() const {
if (IsValid())
Expand Down Expand Up @@ -118,6 +124,22 @@ DWARFDIE::GetReferencedDIE(const dw_attr_t attr) const {
return {};
}

DWARFDIE DWARFDIE::resolveReferencedType(dw_attr_t attr) const {
return GetReferencedDIE(attr);
}

DWARFDIE DWARFDIE::resolveReferencedType(DWARFFormValue v) const {
if (IsValid())
return v.Reference();
return {};
}

DWARFDIE DWARFDIE::resolveTypeUnitReference() const {
if (DWARFDIE reference = GetReferencedDIE(DW_AT_signature))
return reference;
return *this;
}

DWARFDIE
DWARFDIE::GetDIE(dw_offset_t die_offset) const {
if (IsValid())
Expand Down Expand Up @@ -575,3 +597,16 @@ bool DWARFDIE::GetDIENamesAndRanges(
llvm::iterator_range<DWARFDIE::child_iterator> DWARFDIE::children() const {
return llvm::make_range(child_iterator(*this), child_iterator());
}

DWARFDIE::child_iterator DWARFDIE::begin() const {
return child_iterator(*this);
}

DWARFDIE::child_iterator DWARFDIE::end() const { return child_iterator(); }

std::optional<DWARFFormValue> DWARFDIE::find(const dw_attr_t attr) const {
DWARFFormValue form_value;
if (m_die->GetAttributeValue(m_cu, attr, form_value, nullptr, false))
return form_value;
return std::nullopt;
}
15 changes: 15 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,13 @@ class DWARFDIE : public DWARFBaseDIE {

// Functions for obtaining DIE relations and references

std::optional<uint64_t> getLanguage() const;

DWARFDIE
GetParent() const;

DWARFDIE getParent() const { return GetParent(); }

DWARFDIE
GetFirstChild() const;

Expand All @@ -56,6 +60,12 @@ class DWARFDIE : public DWARFBaseDIE {
DWARFDIE
GetReferencedDIE(const dw_attr_t attr) const;

DWARFDIE resolveReferencedType(dw_attr_t attr) const;

DWARFDIE resolveReferencedType(DWARFFormValue v) const;

DWARFDIE resolveTypeUnitReference() const;

// Get a another DIE from the same DWARF file as this DIE. This will
// check the current DIE's compile unit first to see if "die_offset" is
// in the same compile unit, and fall back to checking the DWARF file.
Expand Down Expand Up @@ -96,6 +106,8 @@ class DWARFDIE : public DWARFBaseDIE {
DWARFDIE
GetAttributeValueAsReferenceDIE(const dw_attr_t attr) const;

std::optional<DWARFFormValue> find(const dw_attr_t attr) const;

bool GetDIENamesAndRanges(
const char *&name, const char *&mangled, DWARFRangeList &ranges,
std::optional<int> &decl_file, std::optional<int> &decl_line,
Expand All @@ -105,6 +117,9 @@ class DWARFDIE : public DWARFBaseDIE {

/// The range of all the children of this DIE.
llvm::iterator_range<child_iterator> children() const;

child_iterator begin() const;
child_iterator end() const;
};

class DWARFDIE::child_iterator
Expand Down
25 changes: 25 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,31 @@ uint64_t DWARFFormValue::Reference(dw_offset_t base_offset) const {
}
}

std::optional<uint64_t> DWARFFormValue::getAsUnsignedConstant() const {
if ((!IsDataForm(m_form)) || m_form == lldb_private::dwarf::DW_FORM_sdata)
return std::nullopt;
return m_value.uval;
}

std::optional<int64_t> DWARFFormValue::getAsSignedConstant() const {
if ((!IsDataForm(m_form)) ||
(m_form == lldb_private::dwarf::DW_FORM_udata &&
uint64_t(std::numeric_limits<int64_t>::max()) < m_value.uval))
return std::nullopt;
switch (m_form) {
case lldb_private::dwarf::DW_FORM_data4:
return int32_t(m_value.uval);
case lldb_private::dwarf::DW_FORM_data2:
return int16_t(m_value.uval);
case lldb_private::dwarf::DW_FORM_data1:
return int8_t(m_value.uval);
case lldb_private::dwarf::DW_FORM_sdata:
case lldb_private::dwarf::DW_FORM_data8:
default:
return m_value.sval;
}
}

const uint8_t *DWARFFormValue::BlockData() const { return m_value.data; }

bool DWARFFormValue::IsBlockForm(const dw_form_t form) {
Expand Down
3 changes: 3 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,14 @@ class DWARFFormValue {

uint64_t Reference(dw_offset_t offset) const;
bool Boolean() const { return m_value.uval != 0; }
std::optional<uint64_t> getAsUnsignedConstant() const;
std::optional<int64_t> getAsSignedConstant() const;
uint64_t Unsigned() const { return m_value.uval; }
void SetUnsigned(uint64_t uval) { m_value.uval = uval; }
int64_t Signed() const { return m_value.sval; }
void SetSigned(int64_t sval) { m_value.sval = sval; }
const char *AsCString() const;
const char *getAsCString() const { return AsCString(); }
dw_addr_t Address() const;
bool IsValid() const { return m_form != 0; }
bool SkipValue(const DWARFDataExtractor &debug_info_data,
Expand Down
36 changes: 9 additions & 27 deletions lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "SymbolFileDWARF.h"

#include "llvm/DebugInfo/DWARF/DWARFDebugLoc.h"
#include "llvm/DebugInfo/DWARF/DWARFTypePrinter.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/FileUtilities.h"
#include "llvm/Support/Format.h"
Expand Down Expand Up @@ -2821,33 +2822,14 @@ void SymbolFileDWARF::FindTypes(const TypeQuery &query, TypeResults &results) {
return true; // Keep iterating over index types, language mismatch.
}

// Check the context matches
std::vector<lldb_private::CompilerContext> die_context;
if (query.GetModuleSearch())
die_context = die.GetDeclContext();
else
die_context = die.GetTypeLookupContext();
assert(!die_context.empty());
if (!query_simple.ContextMatches(die_context))
return true; // Keep iterating over index types, context mismatch.

// Try to resolve the type.
if (Type *matching_type = ResolveType(die, true, true)) {
ConstString name = matching_type->GetQualifiedName();
// We have found a type that still might not match due to template
// parameters. If we create a new TypeQuery that uses the new type's
// fully qualified name, we can find out if this type matches at all
// context levels. We can't use just the "match_simple" context
// because all template parameters were stripped off. The fully
// qualified name of the type will have the template parameters and
// will allow us to make sure it matches correctly.
TypeQuery die_query(name.GetStringRef(),
TypeQueryOptions::e_exact_match);
if (!query.ContextMatches(die_query.GetContextRef()))
return true; // Keep iterating over index types, context mismatch.

results.InsertUnique(matching_type->shared_from_this());
}
std::string qualified_name;
llvm::raw_string_ostream os(qualified_name);
llvm::DWARFTypePrinter<DWARFDIE> type_printer(os);
type_printer.appendQualifiedName(die);
TypeQuery die_query(qualified_name, e_exact_match);
if (query.ContextMatches(die_query.GetContextRef()))
if (Type *matching_type = ResolveType(die, true, true))
results.InsertUnique(matching_type->shared_from_this());
return !results.Done(query); // Keep iterating if we aren't done.
});
if (results.Done(query)) {
Expand Down
20 changes: 0 additions & 20 deletions lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1403,26 +1403,6 @@ static TemplateParameterList *CreateTemplateParameterList(
return template_param_list;
}

std::string TypeSystemClang::PrintTemplateParams(
const TemplateParameterInfos &template_param_infos) {
llvm::SmallVector<NamedDecl *, 8> ignore;
clang::TemplateParameterList *template_param_list =
CreateTemplateParameterList(getASTContext(), template_param_infos,
ignore);
llvm::SmallVector<clang::TemplateArgument, 2> args(
template_param_infos.GetArgs());
if (template_param_infos.hasParameterPack()) {
llvm::ArrayRef<TemplateArgument> pack_args =
template_param_infos.GetParameterPackArgs();
args.append(pack_args.begin(), pack_args.end());
}
std::string str;
llvm::raw_string_ostream os(str);
clang::printTemplateArgumentList(os, args, GetTypePrintingPolicy(),
template_param_list);
return str;
}
Copy link
Member

@Michael137 Michael137 Nov 6, 2024

Choose a reason for hiding this comment

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

Nice


clang::FunctionTemplateDecl *TypeSystemClang::CreateFunctionTemplateDecl(
clang::DeclContext *decl_ctx, OptionalClangModuleID owning_module,
clang::FunctionDecl *func_decl,
Expand Down
4 changes: 0 additions & 4 deletions lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,10 +1148,6 @@ class TypeSystemClang : public TypeSystem {

bool SetDeclIsForcefullyCompleted(const clang::TagDecl *td);

/// Return the template parameters (including surrounding <>) in string form.
std::string
PrintTemplateParams(const TemplateParameterInfos &template_param_infos);

private:
/// Returns the PrintingPolicy used when generating the internal type names.
/// These type names are mostly used for the formatter selection.
Expand Down
31 changes: 31 additions & 0 deletions lldb/test/Shell/SymbolFile/DWARF/x86/simplified-template-names.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Test lldb is able to compute the fully qualified names on templates with
// -gsimple-template-names and -fdebug-types-section.

// REQUIRES: lld

// Test against logging to see if we print the fully qualified names correctly.
// RUN: %clangxx --target=x86_64-pc-linux -g -gsimple-template-names %s -o %t
// RUN: %lldb %t -o "log enable dwarf comp" -o "target variable v1 v2" -o exit | FileCheck %s --check-prefix=LOG

// Test that we following DW_AT_signature correctly. If not, lldb might confuse the types of v1 and v2.
// RUN: %clangxx --target=x86_64-pc-linux -g -gsimple-template-names -fdebug-types-section %s -o %t
// RUN: %lldb %t -o "target variable v1 v2" -o exit | FileCheck %s --check-prefix=TYPE

// LOG: unique name: ::t2<outer_struct1::t1<int> >
// LOG: unique name: ::t2<outer_struct2::t1<int> >
Copy link
Member

@Michael137 Michael137 Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we strictly need this test? Technically the existing -gstimple-template-names tests should already exercise this codepath right?

A more interesting test could be a unit-test that uses the DWARFTypePrinter with LLDB DWARFDIEs. Not sure how difficult that is to set up though.

Copy link
Contributor Author

@ZequanWu ZequanWu Nov 7, 2024

Choose a reason for hiding this comment

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

I added extra logging in https://github.com/llvm/llvm-project/pull/112811/files/3fc0675398547617731d0501e3d4b98e2ffc480e#diff-5dd6736e4d6c38623630df16c4494c1a8b099716ee0f05c9af54b4bafb1d864eR1676-R1681 so we can test the names are generated correctly here. Maybe it's not necessary.

Copy link
Collaborator

@labath labath Nov 7, 2024

Choose a reason for hiding this comment

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

I'm not exactly thrilled by the idea of using logs for testing, and even less about adding log statements with the sole purpose of testing a piece of code.

A unit test for the DWARFTypePrinter would definitely be better, and I think it should be possible to reuse what we have in unittests/SymbolFile/DWARF/DWARFDIETest.cpp. For testing its integration into the rest of lldb, an end-to-end might be better (maybe the existing ones are fine, but we can definitely add new ones if you think something is missing).

Copy link
Contributor Author

@ZequanWu ZequanWu Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I recalled why I added this extra logging. It's to test this change in DWARFASTParserClang::GetUniqueTypeNameAndDeclaration, which is outside the DWARFTypePrinter. Though the change shouldn't change the lldb's behaviour at all (as long as the names generated by this function is consistent and unambiguous, the unique dwarf ast type map will work properly), it will be good the internally used names are correctly generated.

For example, it was generating names like the following before:

unique name: t3::<t2<int> >t4

After that change:

unique name: t3<t2<int> >::t4

I updated this shell test to reflect this and added an unit test for DWARFTypePrinter.


// TYPE: (t2<outer_struct1::t1<int> >) v1 = {}
// TYPE-NEXT: (t2<outer_struct2::t1<int> >) v2 = {}
Copy link
Member

@Michael137 Michael137 Nov 6, 2024

Choose a reason for hiding this comment

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

Do we need to check the typename that gets displayed here? Is that actually affected by this patch?

Copy link
Contributor Author

@ZequanWu ZequanWu Nov 7, 2024

Choose a reason for hiding this comment

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

No, they are not affected by this patch. When I working on this change, I had an oversight on not calling DWARFDIE::resolveTypeUnitReference in https://github.com/llvm/llvm-project/pull/112811/files/3fc0675398547617731d0501e3d4b98e2ffc480e#diff-5e94a4eb2c54c062b27821639b3c2732886de49229c99b11b6b2602f35b0ccdcR788 and existing lldb tests didn't catch it. So, I added this for test coverage.


struct outer_struct1 {
template <typename> struct t1 {};
};

struct outer_struct2 {
template <typename> struct t1 {};
};

template <typename> struct t2 {};
t2<outer_struct1::t1<int>> v1;
t2<outer_struct2::t1<int>> v2;
int main() {}
2 changes: 2 additions & 0 deletions llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,8 @@ class DWARFDie {

bool addressRangeContainsAddress(const uint64_t Address) const;

std::optional<uint64_t> getLanguage() const;

Expected<DWARFLocationExpressionsVector>
getLocations(dwarf::Attribute Attr) const;

Expand Down
Loading
Loading