[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd3492ed01667: [LLDB][NativePDB] Fix struct layout when it
has anonymous unions. (authored by zequanwu).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
Files:
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
lldb/test/Shell/SymbolFile/NativePDB/Inputs/class_layout.lldbinit
lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/global-classes.cpp
lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
lldb/unittests/SymbolFile/NativePDB/CMakeLists.txt
lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
Index: lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
===
--- /dev/null
+++ lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
@@ -0,0 +1,244 @@
+//===-- UdtRecordCompleterTests.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private::npdb;
+using namespace llvm;
+
+namespace {
+using Member = UdtRecordCompleter::Member;
+using MemberUP = std::unique_ptr;
+using Record = UdtRecordCompleter::Record;
+
+class WrappedMember {
+public:
+ WrappedMember(const Member ) : m_obj(obj) {}
+
+private:
+ const Member _obj;
+
+ friend bool operator==(const WrappedMember , const WrappedMember ) {
+return lhs.m_obj.kind == rhs.m_obj.kind &&
+ lhs.m_obj.name == rhs.m_obj.name &&
+ lhs.m_obj.bit_offset == rhs.m_obj.bit_offset &&
+ lhs.m_obj.bit_size == rhs.m_obj.bit_size &&
+ lhs.m_obj.base_offset == rhs.m_obj.base_offset &&
+ std::equal(lhs.m_obj.fields.begin(), lhs.m_obj.fields.end(),
+ rhs.m_obj.fields.begin(), rhs.m_obj.fields.end(),
+ [](const MemberUP , const MemberUP ) {
+return WrappedMember(*lhs) == WrappedMember(*rhs);
+ });
+ }
+
+ friend llvm::raw_ostream <<(llvm::raw_ostream ,
+ const WrappedMember ) {
+os << llvm::formatv("Member{.kind={0}, .name=\"{1}\", .bit_offset={2}, "
+".bit_size={3}, .base_offset={4}, .fields=[",
+w.m_obj.kind, w.m_obj.name, w.m_obj.bit_offset,
+w.m_obj.bit_size, w.m_obj.base_offset);
+llvm::ListSeparator sep;
+for (auto : w.m_obj.fields)
+ os << sep << WrappedMember(*f);
+return os << "]}";
+ }
+};
+
+class WrappedRecord {
+public:
+ WrappedRecord(const Record ) : m_obj(obj) {}
+
+private:
+ const Record _obj;
+
+ friend bool operator==(const WrappedRecord , const WrappedRecord ) {
+return lhs.m_obj.start_offset == rhs.m_obj.start_offset &&
+ std::equal(
+ lhs.m_obj.record.fields.begin(), lhs.m_obj.record.fields.end(),
+ rhs.m_obj.record.fields.begin(), rhs.m_obj.record.fields.end(),
+ [](const MemberUP , const MemberUP ) {
+ return WrappedMember(*lhs) == WrappedMember(*rhs);
+ });
+ }
+
+ friend llvm::raw_ostream <<(llvm::raw_ostream ,
+ const WrappedRecord ) {
+os << llvm::formatv("Record{.start_offset={0}, .record.fields=[",
+w.m_obj.start_offset);
+llvm::ListSeparator sep;
+for (const MemberUP : w.m_obj.record.fields)
+ os << sep << WrappedMember(*f);
+return os << "]}";
+ }
+};
+
+class UdtRecordCompleterRecordTests : public testing::Test {
+protected:
+ Record record;
+
+public:
+ void SetKind(Member::Kind kind) { record.record.kind = kind; }
+ void CollectMember(StringRef name, uint64_t byte_offset, uint64_t byte_size) {
+record.CollectMember(name, byte_offset * 8, byte_size * 8,
+ clang::QualType(), lldb::eAccessPublic, 0);
+ }
+ void ConstructRecord() { record.ConstructRecord(); }
+};
+Member *AddField(Member *member, StringRef name, uint64_t byte_offset,
+ uint64_t byte_size, Member::Kind kind,
+ uint64_t base_offset = 0) {
+ auto field
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
zequanwu updated this revision to Diff 467573.
zequanwu marked 2 inline comments as done.
zequanwu added a comment.
Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
Files:
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
lldb/test/Shell/SymbolFile/NativePDB/Inputs/class_layout.lldbinit
lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/global-classes.cpp
lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
lldb/unittests/SymbolFile/NativePDB/CMakeLists.txt
lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
Index: lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
===
--- /dev/null
+++ lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
@@ -0,0 +1,244 @@
+//===-- UdtRecordCompleterTests.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private::npdb;
+using namespace llvm;
+
+namespace {
+using Member = UdtRecordCompleter::Member;
+using MemberUP = std::unique_ptr;
+using Record = UdtRecordCompleter::Record;
+
+class WrappedMember {
+public:
+ WrappedMember(const Member ) : m_obj(obj) {}
+
+private:
+ const Member _obj;
+
+ friend bool operator==(const WrappedMember , const WrappedMember ) {
+return lhs.m_obj.kind == rhs.m_obj.kind &&
+ lhs.m_obj.name == rhs.m_obj.name &&
+ lhs.m_obj.bit_offset == rhs.m_obj.bit_offset &&
+ lhs.m_obj.bit_size == rhs.m_obj.bit_size &&
+ lhs.m_obj.base_offset == rhs.m_obj.base_offset &&
+ std::equal(lhs.m_obj.fields.begin(), lhs.m_obj.fields.end(),
+ rhs.m_obj.fields.begin(), rhs.m_obj.fields.end(),
+ [](const MemberUP , const MemberUP ) {
+return WrappedMember(*lhs) == WrappedMember(*rhs);
+ });
+ }
+
+ friend llvm::raw_ostream <<(llvm::raw_ostream ,
+ const WrappedMember ) {
+os << llvm::formatv("Member{.kind={0}, .name=\"{1}\", .bit_offset={2}, "
+".bit_size={3}, .base_offset={4}, .fields=[",
+w.m_obj.kind, w.m_obj.name, w.m_obj.bit_offset,
+w.m_obj.bit_size, w.m_obj.base_offset);
+llvm::ListSeparator sep;
+for (auto : w.m_obj.fields)
+ os << sep << WrappedMember(*f);
+return os << "]}";
+ }
+};
+
+class WrappedRecord {
+public:
+ WrappedRecord(const Record ) : m_obj(obj) {}
+
+private:
+ const Record _obj;
+
+ friend bool operator==(const WrappedRecord , const WrappedRecord ) {
+return lhs.m_obj.start_offset == rhs.m_obj.start_offset &&
+ std::equal(
+ lhs.m_obj.record.fields.begin(), lhs.m_obj.record.fields.end(),
+ rhs.m_obj.record.fields.begin(), rhs.m_obj.record.fields.end(),
+ [](const MemberUP , const MemberUP ) {
+ return WrappedMember(*lhs) == WrappedMember(*rhs);
+ });
+ }
+
+ friend llvm::raw_ostream <<(llvm::raw_ostream ,
+ const WrappedRecord ) {
+os << llvm::formatv("Record{.start_offset={0}, .record.fields=[",
+w.m_obj.start_offset);
+llvm::ListSeparator sep;
+for (const MemberUP : w.m_obj.record.fields)
+ os << sep << WrappedMember(*f);
+return os << "]}";
+ }
+};
+
+class UdtRecordCompleterRecordTests : public testing::Test {
+protected:
+ Record record;
+
+public:
+ void SetKind(Member::Kind kind) { record.record.kind = kind; }
+ void CollectMember(StringRef name, uint64_t byte_offset, uint64_t byte_size) {
+record.CollectMember(name, byte_offset * 8, byte_size * 8,
+ clang::QualType(), lldb::eAccessPublic, 0);
+ }
+ void ConstructRecord() { record.ConstructRecord(); }
+};
+Member *AddField(Member *member, StringRef name, uint64_t byte_offset,
+ uint64_t byte_size, Member::Kind kind,
+ uint64_t base_offset = 0) {
+ auto field =
+ std::make_unique(name, byte_offset * 8, byte_size * 8,
+ clang::QualType(),
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
Looks good. I suggested some changes, which I hope will reduce duplication and
better empasize the differences between the various branches in the code. I
think I understand the algorithm now, and I believe it is ok, though I can't
escape the feeling that this could have been done in a simpler way.
Comment at:
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:459-469
+ if (parent->kind == Member::Struct) {
+parent->fields.push_back(std::move(fields.back()));
+end_offset_map[end_offset].push_back(parent);
+ } else {
+lldbassert(parent == &&
+ "If parent is union, it must be the top level record.");
+MemberUP anonymous_struct = std::make_unique(Member::Struct);
The current version will create a needless nested struct in case of a union
with a single member. I think it should be possible to just insert a single
field first, and let it be converted to a struct later -- if it is necessary.
Comment at:
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:471-491
+ if (parent->kind == Member::Struct) {
+MemberUP anonymous_union = std::make_unique(Member::Union);
+for (auto : fields) {
+ int64_t bit_size = field->bit_size;
+ anonymous_union->fields.push_back(std::move(field));
+ end_offset_map[offset + bit_size].push_back(
+ anonymous_union->fields.back().get());
This should be equivalent but shorter, and -- I hope -- more obvious.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
zequanwu updated this revision to Diff 467266.
zequanwu added a comment.
- Handle the case when anonymous struct inside an union.
- Append labath's changes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
Files:
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
lldb/test/Shell/SymbolFile/NativePDB/Inputs/class_layout.lldbinit
lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/global-classes.cpp
lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
lldb/unittests/SymbolFile/NativePDB/CMakeLists.txt
lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
Index: lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
===
--- /dev/null
+++ lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
@@ -0,0 +1,244 @@
+//===-- UdtRecordCompleterTests.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private::npdb;
+using namespace llvm;
+
+namespace {
+using Member = UdtRecordCompleter::Member;
+using MemberUP = std::unique_ptr;
+using Record = UdtRecordCompleter::Record;
+
+class WrappedMember {
+public:
+ WrappedMember(const Member ) : m_obj(obj) {}
+
+private:
+ const Member _obj;
+
+ friend bool operator==(const WrappedMember , const WrappedMember ) {
+return lhs.m_obj.kind == rhs.m_obj.kind &&
+ lhs.m_obj.name == rhs.m_obj.name &&
+ lhs.m_obj.bit_offset == rhs.m_obj.bit_offset &&
+ lhs.m_obj.bit_size == rhs.m_obj.bit_size &&
+ lhs.m_obj.base_offset == rhs.m_obj.base_offset &&
+ std::equal(lhs.m_obj.fields.begin(), lhs.m_obj.fields.end(),
+ rhs.m_obj.fields.begin(), rhs.m_obj.fields.end(),
+ [](const MemberUP , const MemberUP ) {
+return WrappedMember(*lhs) == WrappedMember(*rhs);
+ });
+ }
+
+ friend llvm::raw_ostream <<(llvm::raw_ostream ,
+ const WrappedMember ) {
+os << llvm::formatv("Member{.kind={0}, .name=\"{1}\", .bit_offset={2}, "
+".bit_size={3}, .base_offset={4}, .fields=[",
+w.m_obj.kind, w.m_obj.name, w.m_obj.bit_offset,
+w.m_obj.bit_size, w.m_obj.base_offset);
+llvm::ListSeparator sep;
+for (auto : w.m_obj.fields)
+ os << sep << WrappedMember(*f);
+return os << "]}";
+ }
+};
+
+class WrappedRecord {
+public:
+ WrappedRecord(const Record ) : m_obj(obj) {}
+
+private:
+ const Record _obj;
+
+ friend bool operator==(const WrappedRecord , const WrappedRecord ) {
+return lhs.m_obj.start_offset == rhs.m_obj.start_offset &&
+ std::equal(
+ lhs.m_obj.record.fields.begin(), lhs.m_obj.record.fields.end(),
+ rhs.m_obj.record.fields.begin(), rhs.m_obj.record.fields.end(),
+ [](const MemberUP , const MemberUP ) {
+ return WrappedMember(*lhs) == WrappedMember(*rhs);
+ });
+ }
+
+ friend llvm::raw_ostream <<(llvm::raw_ostream ,
+ const WrappedRecord ) {
+os << llvm::formatv("Record{.start_offset={0}, .record.fields=[",
+w.m_obj.start_offset);
+llvm::ListSeparator sep;
+for (const MemberUP : w.m_obj.record.fields)
+ os << sep << WrappedMember(*f);
+return os << "]}";
+ }
+};
+
+class UdtRecordCompleterRecordTests : public testing::Test {
+protected:
+ Record record;
+
+public:
+ void SetKind(Member::Kind kind) { record.record.kind = kind; }
+ void CollectMember(StringRef name, uint64_t byte_offset, uint64_t byte_size) {
+record.CollectMember(name, byte_offset * 8, byte_size * 8,
+ clang::QualType(), lldb::eAccessPublic, 0);
+ }
+ void ConstructRecord() { record.ConstructRecord(); }
+};
+Member *AddField(Member *member, StringRef name, uint64_t byte_offset,
+ uint64_t byte_size, Member::Kind kind,
+ uint64_t base_offset = 0) {
+ auto field =
+ std::make_unique(name, byte_offset * 8, byte_size * 8,
+
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
zequanwu added inline comments.
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:459
+ uint64_t end_offset = offset + fields.back()->bit_size;
+ parent->fields.push_back(fields.back());
+ end_offset_map[end_offset].push_back(parent);
labath wrote:
> zequanwu wrote:
> > labath wrote:
> > > Can `parent` be a union here? Would the algorithm still be correct?
> > `parent` could only be union when the top level record is a union (at this
> > line `Member *parent = `). That's the only case when we add an
> > union into `end_offset_map`. In that case, all the fields would start at
> > the same offset and we only iterate the loop `for (auto : fields) {`
> > once and then we are done.
> > Otherwise, we only insert {end offset, struct/field} into `end_offset_map`
> > where field must be within an union.
> Are you sure about that? I've created what I think is a [[
> https://reviews.llvm.org/D135768 | counterexample ]] to these statements.
> Here a top-level union contains a bunch of sequential fields, which is
> perfectly possible if the only member of that union is an anonymous struct
> which contains those fields.
>
> I don't think that's what supposed to happen in this case, but I'm open to
> being convinced otherwise.
>
> (I've also rewritten the test logic so it produces better error messages than
> "false is not true".)
Thanks for writing the counter example and making better error message. I
appended your changes in this file.
I updated to handle the case when parent is an union here by creating an
anonymous struct to hold it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
labath added inline comments.
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:459
+ uint64_t end_offset = offset + fields.back()->bit_size;
+ parent->fields.push_back(fields.back());
+ end_offset_map[end_offset].push_back(parent);
zequanwu wrote:
> labath wrote:
> > Can `parent` be a union here? Would the algorithm still be correct?
> `parent` could only be union when the top level record is a union (at this
> line `Member *parent = `). That's the only case when we add an union
> into `end_offset_map`. In that case, all the fields would start at the same
> offset and we only iterate the loop `for (auto : fields) {` once and
> then we are done.
> Otherwise, we only insert {end offset, struct/field} into `end_offset_map`
> where field must be within an union.
Are you sure about that? I've created what I think is a [[
https://reviews.llvm.org/D135768 | counterexample ]] to these statements. Here
a top-level union contains a bunch of sequential fields, which is perfectly
possible if the only member of that union is an anonymous struct which contains
those fields.
I don't think that's what supposed to happen in this case, but I'm open to
being convinced otherwise.
(I've also rewritten the test logic so it produces better error messages than
"false is not true".)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
zequanwu added inline comments.
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:459
+ uint64_t end_offset = offset + fields.back()->bit_size;
+ parent->fields.push_back(fields.back());
+ end_offset_map[end_offset].push_back(parent);
labath wrote:
> Can `parent` be a union here? Would the algorithm still be correct?
`parent` could only be union when the top level record is a union (at this line
`Member *parent = `). That's the only case when we add an union into
`end_offset_map`. In that case, all the fields would start at the same offset
and we only iterate the loop `for (auto : fields) {` once and then we are
done.
Otherwise, we only insert {end offset, struct/field} into `end_offset_map`
where field must be within an union.
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:471
+ } else {
+for (auto : fields) {
+ parent->fields.push_back(field);
labath wrote:
> IIUC, this code is reached when the `parent` object is a union. However, the
> parent is chosen such that it ends before the start of these new fields?
> Therefore its start address will be before the start of these fields as well.
> Is it correct to add the fields to the union under these circumstances, or am
> I misunderstanding something?
This is a special case to handle the case when top level record is an union.
That's the only case we would reach here. This is to avoid adding unnecessary
nested union. For example, the unit test `TestAnonymousUnionInUnion` would
become the following if we always add an anonymous union:
```
union {
union {
m1;
m2;
m3;
m4;
}
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
zequanwu updated this revision to Diff 466575.
zequanwu marked 2 inline comments as done.
zequanwu added a comment.
Address comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
Files:
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
lldb/test/Shell/SymbolFile/NativePDB/Inputs/class_layout.lldbinit
lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/global-classes.cpp
lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
lldb/unittests/SymbolFile/NativePDB/CMakeLists.txt
lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
Index: lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
===
--- /dev/null
+++ lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
@@ -0,0 +1,178 @@
+//===-- UdtRecordCompleterTests.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private::npdb;
+using namespace llvm;
+
+namespace {
+using Member = UdtRecordCompleter::Member;
+using Record = UdtRecordCompleter::Record;
+using MemberUP = std::unique_ptr;
+
+class UdtRecordCompleterRecordTests : public testing::Test {
+ Record record;
+
+public:
+ void SetKind(Member::Kind kind) { record.record.kind = kind; }
+ void CollectMember(StringRef name, uint64_t byte_offset, uint64_t byte_size) {
+record.CollectMember(name, byte_offset * 8, byte_size * 8,
+ clang::QualType(), lldb::eAccessPublic, 0);
+ }
+ void ConstructRecord() { record.ConstructRecord(); }
+
+ static bool VerifyMembers(const llvm::SmallVector ,
+const llvm::SmallVector ) {
+if (lhs.size() != rhs.size())
+ return false;
+for (size_t i = 0; i < lhs.size(); ++i) {
+ if (lhs[i]->kind != rhs[i]->kind || lhs[i]->name != rhs[i]->name ||
+ lhs[i]->bit_offset != rhs[i]->bit_offset ||
+ lhs[i]->bit_size != rhs[i]->bit_size ||
+ lhs[i]->base_offset != rhs[i]->base_offset ||
+ !VerifyMembers(lhs[i]->fields, rhs[i]->fields))
+return false;
+}
+return true;
+ }
+ bool VerifyRecord(Record ) {
+return record.start_offset == testRecord.start_offset &&
+ VerifyMembers(record.record.fields, testRecord.record.fields);
+ }
+};
+Member *AddField(Member *member, StringRef name, uint64_t byte_offset,
+ uint64_t byte_size, Member::Kind kind,
+ uint64_t base_offset = 0) {
+ auto field =
+ std::make_unique(name, byte_offset * 8, byte_size * 8,
+ clang::QualType(), lldb::eAccessPublic, 0);
+ field->kind = kind;
+ field->base_offset = base_offset;
+ member->fields.push_back(std::move(field));
+ return member->fields.back().get();
+}
+} // namespace
+
+TEST_F(UdtRecordCompleterRecordTests, TestAnonymousUnionInStruct) {
+ SetKind(Member::Kind::Struct);
+ CollectMember("m1", 0, 4);
+ CollectMember("m2", 0, 4);
+ CollectMember("m3", 0, 1);
+ CollectMember("m4", 0, 8);
+ ConstructRecord();
+
+ // struct {
+ // union {
+ // m1;
+ // m2;
+ // m3;
+ // m4;
+ // };
+ // };
+ Record record;
+ record.start_offset = 0;
+ Member *u = AddField(, "", 0, 0, Member::Union);
+ AddField(u, "m1", 0, 4, Member::Field);
+ AddField(u, "m2", 0, 4, Member::Field);
+ AddField(u, "m3", 0, 1, Member::Field);
+ AddField(u, "m4", 0, 8, Member::Field);
+ EXPECT_TRUE(VerifyRecord(record));
+}
+
+TEST_F(UdtRecordCompleterRecordTests, TestAnonymousUnionInUnion) {
+ SetKind(Member::Kind::Union);
+ CollectMember("m1", 0, 4);
+ CollectMember("m2", 0, 4);
+ CollectMember("m3", 0, 1);
+ CollectMember("m4", 0, 8);
+ ConstructRecord();
+
+ // union {
+ // m1;
+ // m2;
+ // m3;
+ // m4;
+ // };
+ Record record;
+ record.start_offset = 0;
+ AddField(, "m1", 0, 4, Member::Field);
+ AddField(, "m2", 0, 4, Member::Field);
+ AddField(, "m3", 0, 1, Member::Field);
+ AddField(, "m4", 0, 8, Member::Field);
+ EXPECT_TRUE(VerifyRecord(record));
+}
+
+TEST_F(UdtRecordCompleterRecordTests, TestNestedUnionStructInStruct) {
+ SetKind(Member::Kind::Struct);
+ CollectMember("m1",
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
labath added a comment.
The test looks great, and the comments have helped. I still have a couple of
questions about the algorithm though.
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:431
+uint64_t offset = pair.first;
+auto = pair.second;
+lldbassert(offset >= start_offset);
This shadowing the fields member confused me for quite some time. Could you
choose a different name for one of them?
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:459
+ uint64_t end_offset = offset + fields.back()->bit_size;
+ parent->fields.push_back(fields.back());
+ end_offset_map[end_offset].push_back(parent);
Can `parent` be a union here? Would the algorithm still be correct?
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:471
+ } else {
+for (auto : fields) {
+ parent->fields.push_back(field);
IIUC, this code is reached when the `parent` object is a union. However, the
parent is chosen such that it ends before the start of these new fields?
Therefore its start address will be before the start of these fields as well.
Is it correct to add the fields to the union under these circumstances, or am I
misunderstanding something?
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:64
+uint64_t base_offset;
+llvm::SmallVector fields;
+
zequanwu wrote:
> labath wrote:
> > Can we drop the `SP` part? I think that the owning references (I guess
> > that's this field) could be just plain Field instances (unique_ptr
> > at worst), and the rest could just be plain pointers and references.
> The Field object is shared between fields and m_fields. And we can't have
> Field member instance inside Field class.
You can't have a `Field` member, but you can have a Field*, unique_ptr
and std::vector members. SmallVector is also not possible, for
reasons that are mostly obvious, but then again, storing a pointer inside a
SmallVector negates most of the benefits that one could hope to gain by using
it.
My point is that that using a shared pointer makes it harder to understand the
relationships between the objects because it obfuscates the ownership aspect.
Sometimes that is unavoidable, like when there just isn't a single object that
can own some other object (although llvm mostly avoids that by putting the
ownership inside some "context" object), but it's not clear to me why that
would be the case here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
zequanwu added inline comments. Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:392-443 + // For anonymous unions in a struct, msvc generated pdb doesn't have the + // entity for that union. So, we need to construct anonymous union and struct + // based on field offsets. The final AST is likely not matching the exact + // original AST, but the memory layout is preseved. + + // The end offset to a field/struct/union that ends at the offset. + std::map end_offset_map; labath wrote: > This could use a high-level explanation of the algorithm. Like, I know it > tries to stuff the fields into structs and unions, but I wasn't able to > figure out how it does that, and what are the operating invariants. > > The thing I like about this algorithm, is that the most complicated part (the > thing I highlighted) is basically just playing with numbers and it is > independent of any complex objects and state. If this part is separated out > into a separate function, then it would be perfectly suitable for unit > testing, and the unit tests could also serve as documentation/examples of the > kinds of transformations that the algorithm does. Moved into a separate function and added unit tests. Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:64 +uint64_t base_offset; +llvm::SmallVector fields; + labath wrote: > Can we drop the `SP` part? I think that the owning references (I guess that's > this field) could be just plain Field instances (unique_ptr at worst), > and the rest could just be plain pointers and references. The Field object is shared between fields and m_fields. And we can't have Field member instance inside Field class. Comment at: lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp:53-59 +// CHECK-NEXT: | |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal +// CHECK-NEXT: | | |-DefaultConstructor exists trivial needs_implicit +// CHECK-NEXT: | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param +// CHECK-NEXT: | | |-MoveConstructor exists simple trivial needs_implicit +// CHECK-NEXT: | | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param +// CHECK-NEXT: | | |-MoveAssignment exists simple trivial needs_implicit +// CHECK-NEXT: | | `-Destructor simple irrelevant trivial needs_implicit labath wrote: > I think we should exclude all of these DefinitionData fields from the test > expectation, as they are largely irrelevant to the test (and they also > obfuscate it very well). Otherwise, people will have to update these whenever > a new field gets added. > > I don't know whether it contains the thing you wanted to test (as I don't > know what that is), but the `type lookup C` output will contain the general > structure of the type, and it will be a lot more readable. Thanks, I didn't know that before. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134849/new/ https://reviews.llvm.org/D134849 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
zequanwu updated this revision to Diff 465933.
zequanwu marked 5 inline comments as done.
zequanwu added a comment.
Add unittests and address comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
Files:
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
lldb/test/Shell/SymbolFile/NativePDB/Inputs/class_layout.lldbinit
lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/global-classes.cpp
lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
lldb/unittests/SymbolFile/NativePDB/CMakeLists.txt
lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
Index: lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
===
--- /dev/null
+++ lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
@@ -0,0 +1,178 @@
+//===-- UdtRecordCompleterTests.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private::npdb;
+using namespace llvm;
+
+namespace {
+using Member = UdtRecordCompleter::Member;
+using Record = UdtRecordCompleter::Record;
+using MemberSP = std::shared_ptr;
+
+class UdtRecordCompleterRecordTests : public testing::Test {
+ Record record;
+
+public:
+ void SetKind(Member::Kind kind) { record.record.kind = kind; }
+ void CollectMember(StringRef name, uint64_t byte_offset, uint64_t byte_size) {
+record.CollectMember(name, byte_offset * 8, byte_size * 8,
+ clang::QualType(), lldb::eAccessPublic, 0);
+ }
+ void ConstructRecord() { record.ConstructRecord(); }
+
+ static bool VerifyMembers(const llvm::SmallVector ,
+const llvm::SmallVector ) {
+if (lhs.size() != rhs.size())
+ return false;
+for (size_t i = 0; i < lhs.size(); ++i) {
+ if (lhs[i]->kind != rhs[i]->kind || lhs[i]->name != rhs[i]->name ||
+ lhs[i]->bit_offset != rhs[i]->bit_offset ||
+ lhs[i]->bit_size != rhs[i]->bit_size ||
+ lhs[i]->base_offset != rhs[i]->base_offset ||
+ !VerifyMembers(lhs[i]->fields, rhs[i]->fields))
+return false;
+}
+return true;
+ }
+ bool VerifyRecord(Record ) {
+return record.start_offset == testRecord.start_offset &&
+ VerifyMembers(record.record.fields, testRecord.record.fields);
+ }
+};
+MemberSP AddField(Member *member, StringRef name, uint64_t byte_offset,
+ uint64_t byte_size, Member::Kind kind,
+ uint64_t base_offset = 0) {
+ auto field =
+ std::make_shared(name, byte_offset * 8, byte_size * 8,
+ clang::QualType(), lldb::eAccessPublic, 0);
+ field->kind = kind;
+ field->base_offset = base_offset;
+ member->fields.push_back(field);
+ return field;
+}
+} // namespace
+
+TEST_F(UdtRecordCompleterRecordTests, TestAnonymousUnionInStruct) {
+ SetKind(Member::Kind::Struct);
+ CollectMember("m1", 0, 4);
+ CollectMember("m2", 0, 4);
+ CollectMember("m3", 0, 1);
+ CollectMember("m4", 0, 8);
+ ConstructRecord();
+
+ // struct {
+ // union {
+ // m1;
+ // m2;
+ // m3;
+ // m4;
+ // };
+ // };
+ Record record;
+ record.start_offset = 0;
+ MemberSP u = AddField(, "", 0, 0, Member::Union);
+ AddField(u.get(), "m1", 0, 4, Member::Field);
+ AddField(u.get(), "m2", 0, 4, Member::Field);
+ AddField(u.get(), "m3", 0, 1, Member::Field);
+ AddField(u.get(), "m4", 0, 8, Member::Field);
+ EXPECT_TRUE(VerifyRecord(record));
+}
+
+TEST_F(UdtRecordCompleterRecordTests, TestAnonymousUnionInUnion) {
+ SetKind(Member::Kind::Union);
+ CollectMember("m1", 0, 4);
+ CollectMember("m2", 0, 4);
+ CollectMember("m3", 0, 1);
+ CollectMember("m4", 0, 8);
+ ConstructRecord();
+
+ // union {
+ // m1;
+ // m2;
+ // m3;
+ // m4;
+ // };
+ Record record;
+ record.start_offset = 0;
+ AddField(, "m1", 0, 4, Member::Field);
+ AddField(, "m2", 0, 4, Member::Field);
+ AddField(, "m3", 0, 1, Member::Field);
+ AddField(, "m4", 0, 8, Member::Field);
+ EXPECT_TRUE(VerifyRecord(record));
+}
+
+TEST_F(UdtRecordCompleterRecordTests, TestNestedUnionStructInStruct) {
+ SetKind(Member::Kind::Struct);
+
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
labath added inline comments.
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:331
+ clang::DeclContext *parent_decl_ctx) {
+ static lldb::user_id_t anonymous_id = LLDB_INVALID_UID - 1;
+ clang::FieldDecl *field_decl = nullptr;
Now multiple symbol files can race when accessing this variable (and this also
introduces a strange interaction between two supposedly-independent symbol
files). Better make this member variable of the parent symbol file, or
something like that.
Comment at:
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:392-443
+ // For anonymous unions in a struct, msvc generated pdb doesn't have the
+ // entity for that union. So, we need to construct anonymous union and struct
+ // based on field offsets. The final AST is likely not matching the exact
+ // original AST, but the memory layout is preseved.
+
+ // The end offset to a field/struct/union that ends at the offset.
+ std::map end_offset_map;
This could use a high-level explanation of the algorithm. Like, I know it tries
to stuff the fields into structs and unions, but I wasn't able to figure out
how it does that, and what are the operating invariants.
The thing I like about this algorithm, is that the most complicated part (the
thing I highlighted) is basically just playing with numbers and it is
independent of any complex objects and state. If this part is separated out
into a separate function, then it would be perfectly suitable for unit testing,
and the unit tests could also serve as documentation/examples of the kinds of
transformations that the algorithm does.
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:54
+ struct Field {
+enum Kind { FIELD, STRUCT, UNION } kind;
+// Following are only used for field.
according to
,
these should be called `Field, Struct, Union`
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:64
+uint64_t base_offset;
+llvm::SmallVector fields;
+
Can we drop the `SP` part? I think that the owning references (I guess that's
this field) could be just plain Field instances (unique_ptr at worst),
and the rest could just be plain pointers and references.
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:92
+ // llvm::SmallVector m_fields;
+ uint64_t start_offset = UINT64_MAX;
+ bool m_is_struct;
m_start_offset ?
Comment at: lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp:53-59
+// CHECK-NEXT: | |-DefinitionData pass_in_registers aggregate standard_layout
trivially_copyable pod trivial literal
+// CHECK-NEXT: | | |-DefaultConstructor exists trivial needs_implicit
+// CHECK-NEXT: | | |-CopyConstructor simple trivial has_const_param
needs_implicit implicit_has_const_param
+// CHECK-NEXT: | | |-MoveConstructor exists simple trivial needs_implicit
+// CHECK-NEXT: | | |-CopyAssignment simple trivial has_const_param
needs_implicit implicit_has_const_param
+// CHECK-NEXT: | | |-MoveAssignment exists simple trivial needs_implicit
+// CHECK-NEXT: | | `-Destructor simple irrelevant trivial needs_implicit
I think we should exclude all of these DefinitionData fields from the test
expectation, as they are largely irrelevant to the test (and they also
obfuscate it very well). Otherwise, people will have to update these whenever a
new field gets added.
I don't know whether it contains the thing you wanted to test (as I don't know
what that is), but the `type lookup C` output will contain the general
structure of the type, and it will be a lot more readable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
zequanwu updated this revision to Diff 465565.
zequanwu marked an inline comment as done.
zequanwu added a comment.
Updated to handle the nested anonymous union and struct situation.
It's done by:
1. Create the record layout with `Field` which could be a field member or an
anonymous struct or an anonymous union. If it's struct/union, it has `fields`
to refer to its members.
2. Create the AST from the record layout above.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
Files:
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
lldb/test/Shell/SymbolFile/NativePDB/Inputs/class_layout.lldbinit
lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/global-classes.cpp
lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
Index: lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
===
--- lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
+++ lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
@@ -242,6 +242,7 @@
// CHECK-NEXT: (lldb) type lookup -- Derived
// CHECK-NEXT: class Derived : public Class {
// CHECK-NEXT: public:
+// CHECK-NEXT: Derived();
// CHECK-NEXT: Derived
// CHECK-NEXT: OneMember Member;
// CHECK-NEXT: const OneMember ConstMember;
Index: lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp
===
--- lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp
+++ /dev/null
@@ -1,39 +0,0 @@
-// clang-format off
-// REQUIRES: lld, x86
-
-// Make sure class layout is correct.
-// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -c /Fo%t.obj -- %s
-// RUN: lld-link -debug:full -nodefaultlib -entry:main %t.obj -out:%t.exe -pdb:%t.pdb
-// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe \
-// RUN: -o "expr a" -o "expr b.c" -o "expr b.u.c" -o "expr b.u.i" -o "exit" | FileCheck %s
-
-// CHECK: (lldb) expr a
-// CHECK-NEXT: (A) $0 = (d1 = 'a', d2 = 1, d3 = 2, d4 = 'b')
-// CHECK-NEXT: (lldb) expr b.c
-// CHECK-NEXT: (char) $1 = 'a'
-// CHECK-NEXT: (lldb) expr b.u.c
-// CHECK-NEXT: (char[2]) $2 = "b"
-// CHECK-NEXT: (lldb) expr b.u.i
-// CHECK-NEXT: (int) $3 = 98
-
-struct __attribute__((packed, aligned(1))) A {
- char d1;
- int d2;
- int d3;
- char d4;
-};
-
-struct __attribute__((packed, aligned(1))) B {
- char c;
- union {
-char c[2];
-int i;
- } u;
-};
-
-A a = {'a', 1, 2, 'b'};
-B b = {'a', {"b"}};
-
-int main() {
- return 0;
-}
Index: lldb/test/Shell/SymbolFile/NativePDB/global-classes.cpp
===
--- lldb/test/Shell/SymbolFile/NativePDB/global-classes.cpp
+++ lldb/test/Shell/SymbolFile/NativePDB/global-classes.cpp
@@ -311,41 +311,41 @@
// CHECK: | `-EnumConstantDecl {{.*}} B 'EnumType'
// CHECK: |-CXXRecordDecl {{.*}} struct DerivedClass definition
// CHECK: | |-public 'BaseClass'
-// CHECK: | |-FieldDecl {{.*}} DerivedMember 'int'
-// CHECK: | `-CXXConstructorDecl {{.*}} DerivedClass 'void (int, int)'
-// CHECK: | |-ParmVarDecl {{.*}} 'int'
-// CHECK: | `-ParmVarDecl {{.*}} 'int'
+// CHECK: | |-CXXConstructorDecl {{.*}} DerivedClass 'void (int, int)'
+// CHECK: | | |-ParmVarDecl {{.*}} 'int'
+// CHECK: | | `-ParmVarDecl {{.*}} 'int'
+// CHECK: | `-FieldDecl {{.*}} DerivedMember 'int'
// CHECK: |-VarDecl {{.*}} DC 'const DerivedClass'
// CHECK: |-CXXRecordDecl {{.*}} struct BaseClass definition
-// CHECK: | |-FieldDecl {{.*}} BaseMember 'int'
-// CHECK: | `-CXXMethodDecl {{.*}} BaseClass 'void (int)'
-// CHECK: | `-ParmVarDecl {{.*}} 'int'
+// CHECK: | |-CXXMethodDecl {{.*}} BaseClass 'void (int)'
+// CHECK: | | `-ParmVarDecl {{.*}} 'int'
+// CHECK: | `-FieldDecl {{.*}} BaseMember 'int'
// CHECK: |-CXXRecordDecl {{.*}} struct EBO definition
// CHECK: | |-public 'EmptyBase'
-// CHECK: | |-FieldDecl {{.*}} Member 'int'
-// CHECK: | `-CXXConstructorDecl {{.*}} EBO 'void (int)'
-// CHECK: | `-ParmVarDecl {{.*}} 'int'
+// CHECK: | |-CXXConstructorDecl {{.*}} EBO 'void (int)'
+// CHECK: | | `-ParmVarDecl {{.*}} 'int'
+// CHECK: | `-FieldDecl {{.*}} Member 'int'
// CHECK: |-VarDecl {{.*}} EBOC 'const EBO'
// CHECK: |-CXXRecordDecl {{.*}} struct EmptyBase definition
// CHECK: |-CXXRecordDecl {{.*}} struct PaddedBases definition
// CHECK: | |-public 'BaseClass'
// CHECK: | |-public 'BaseClass'
// CHECK: | |-public 'BaseClass'
-// CHECK: | |-FieldDecl {{.*}} DerivedMember 'long long'
-// CHECK: | `-CXXConstructorDecl {{.*}} PaddedBases 'void (char, short, int, long long)'
-// CHECK: | |-ParmVarDecl {{.*}} 'char'
-// CHECK: | |-ParmVarDecl {{.*}}
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
labath added a comment.
The fact that MSVC does not store all of the anonymous union data is
unfortunate, though not entirely surprising, given that the goal of debug info
never was to offer an exact reconstruction of the source AST.
That, I'm not sure if checking for matching initial offsets is sufficient to
create a semblance of a consistent structure definition. What will this code do
with structures like this:
struct S3 {
char x[3];
};
struct A {
union {
struct {
char c1;
S3 s1;
};
struct {
S3 s2;
char c2;
};
};
} a;
In this case, `a.s1` and `a.c2` overlap, but they don't share the same starting
offset. If the compiler does not provide data for anonymous structs as well,
then I think this algorithm will need to be more complicated. I'm not even sure
they can be reconstructed correctly, as the anonymous structs and unions can
nest indefinitely. Maybe it would be sufficient to represent this as a "union
of structs", where each struct gets (greedily) packed with as many members as
it can hold, and we create as many structs as we need (we can replace the
struct with a simple member if it would hold just one member)?
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:321
+ // the artificial anonymous union.
+ lldb::user_id_t au_id = LLDB_INVALID_UID - toOpaqueUid(m_id);
+ uint64_t au_field_bitoffset = 0;
How are these opaque ids computed? Will they produce unique IDs if you have two
structs like this close together? Might it be better to just make a global
(local to a symbol file or something) counter and always increment/decrement
that when creating another type ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.
zequanwu created this revision.
zequanwu added reviewers: labath, rnk.
Herald added a project: All.
zequanwu requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
Previously, lldb mistook fields in anonymous union in a struct as the direct
field of the struct, which causes lldb crashes due to multiple fields sharing
the same offset in a struct. This patch fixes it.
MSVC generated pdb doesn't have the debug info entity representing a anonymous
union in a struct. It looks like the following:
struct S {
union {
char c;
int i;
};
};
0x1004 | LF_FIELDLIST [size = 40]
- LF_MEMBER [name = `c`, Type = 0x0070 (char), offset = 0, attrs =
public]
- LF_MEMBER [name = `i`, Type = 0x0074 (int), offset = 0, attrs =
public]
0x1005 | LF_STRUCTURE [size = 32] `S`
unique name: `.?AUS@@`
vtable: , base list: , field list: 0x1004
Clang generated pdb is similar, though due to the bug
,
it's not more useful than the debug info above. But that's not very relavent,
lldb should still be able to understand MSVC geneerated pdb.
0x1003 | LF_UNION [size = 60] `S::`
unique name: `.?AT@S@@`
field list:
options: forward ref (= 0x1003) | has unique name | is nested,
sizeof 0
0x1004 | LF_FIELDLIST [size = 40]
- LF_MEMBER [name = `c`, Type = 0x0070 (char), offset = 0, attrs =
public]
- LF_MEMBER [name = `i`, Type = 0x0074 (int), offset = 0, attrs =
public]
- LF_NESTTYPE [name = ``, parent = 0x1003]
0x1005 | LF_STRUCTURE [size = 32] `S`
unique name: `.?AUS@@`
vtable: , base list: , field list: 0x1004
options: contains nested class | has unique name, sizeof 4
0x1006 | LF_FIELDLIST [size = 28]
- LF_MEMBER [name = `c`, Type = 0x0070 (char), offset = 0, attrs =
public]
- LF_MEMBER [name = `i`, Type = 0x0074 (int), offset = 0, attrs =
public]
0x1007 | LF_UNION [size = 60] `S::`
unique name: `.?AT@S@@`
field list: 0x1006
options: has unique name | is nested | sealed, sizeof
This patch delays the FieldDecl creation when travesing LF_FIELDLIST so we know
if there are multiple fields are in the same offsets and are able to group them
into different anonymous unions based on offsets. Nested anonymous union will
be flatten into one anonymous union, because we simply don't have that info, but
they are equivalent in terms of union layout.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D134849
Files:
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
lldb/test/Shell/SymbolFile/NativePDB/Inputs/class_layout.lldbinit
lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/global-classes.cpp
lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp
lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
Index: lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
===
--- lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
+++ lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp
@@ -242,6 +242,7 @@
// CHECK-NEXT: (lldb) type lookup -- Derived
// CHECK-NEXT: class Derived : public Class {
// CHECK-NEXT: public:
+// CHECK-NEXT: Derived();
// CHECK-NEXT: Derived
// CHECK-NEXT: OneMember Member;
// CHECK-NEXT: const OneMember ConstMember;
Index: lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp
===
--- lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp
+++ /dev/null
@@ -1,39 +0,0 @@
-// clang-format off
-// REQUIRES: lld, x86
-
-// Make sure class layout is correct.
-// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -c /Fo%t.obj -- %s
-// RUN: lld-link -debug:full -nodefaultlib -entry:main %t.obj -out:%t.exe -pdb:%t.pdb
-// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe \
-// RUN: -o "expr a" -o "expr b.c" -o "expr b.u.c" -o "expr b.u.i" -o "exit" | FileCheck %s
-
-// CHECK: (lldb) expr a
-// CHECK-NEXT: (A) $0 = (d1 = 'a', d2 = 1, d3 = 2, d4 = 'b')
-// CHECK-NEXT: (lldb) expr b.c
-// CHECK-NEXT: (char) $1 = 'a'
-// CHECK-NEXT: (lldb) expr b.u.c
-// CHECK-NEXT: (char[2]) $2 = "b"
-// CHECK-NEXT: (lldb) expr b.u.i
-// CHECK-NEXT: (int) $3 = 98
-
-struct __attribute__((packed, aligned(1))) A {
- char d1;
- int d2;
- int d3;
- char d4;
-};
-
-struct __attribute__((packed, aligned(1))) B {
- char c;
- union {
-char c[2];
-int i;
- } u;
-};
-
-A a = {'a', 1, 2, 'b'};
-B b = {'a', {"b"}};
-
-int main() {
- return 0;
-}
Index:
