From a801568c8176f66f5a8acec022b0678e5a721092 Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Thu, 12 Sep 2024 06:57:46 +0000 Subject: [PATCH 1/8] Fix: resolve nested attributes of anonymous records in GetDatamembers --- lib/Interpreter/CppInterOp.cpp | 57 +++++++++++++-- .../CppInterOp/VariableReflectionTest.cpp | 71 +++++++++++++++++++ 2 files changed, 122 insertions(+), 6 deletions(-) diff --git a/lib/Interpreter/CppInterOp.cpp b/lib/Interpreter/CppInterOp.cpp index 96aab8020..c696e0bf8 100644 --- a/lib/Interpreter/CppInterOp.cpp +++ b/lib/Interpreter/CppInterOp.cpp @@ -1128,9 +1128,33 @@ namespace Cpp { if (auto *CXXRD = llvm::dyn_cast_or_null(D)) { std::vector datamembers; - for (auto it = CXXRD->field_begin(), end = CXXRD->field_end(); it != end; - it++) { - datamembers.push_back((TCppScope_t)*it); + llvm::SmallVector stack_begin; + llvm::SmallVector stack_end; + stack_begin.push_back(CXXRD->field_begin()); + stack_end.push_back(CXXRD->field_end()); + while (stack_begin.size()) { + if (stack_begin.back() == stack_end.back()) { + stack_begin.pop_back(); + stack_end.pop_back(); + continue; + } + Decl* D = *(stack_begin.back()); + if (D->getKind() == clang::Decl::Field && + dyn_cast(D)->isAnonymousStructOrUnion()) { + if (auto* CXXRD = llvm::dyn_cast_or_null( + dyn_cast(D) + ->getType() + ->getAs() + ->getDecl())) { + stack_begin.back()++; + stack_begin.push_back(CXXRD->field_begin()); + stack_end.push_back(CXXRD->field_end()); + continue; + } + assert(false); // should be unreachable else internal error + } + datamembers.push_back((TCppScope_t)D); + stack_begin.back()++; } return datamembers; @@ -1175,9 +1199,30 @@ namespace Cpp { auto *D = (Decl *) var; auto &C = getASTContext(); - if (auto *FD = llvm::dyn_cast(D)) - return (intptr_t) C.toCharUnitsFromBits(C.getASTRecordLayout(FD->getParent()) - .getFieldOffset(FD->getFieldIndex())).getQuantity(); + if (auto* FD = llvm::dyn_cast(D)) { + const clang::RecordDecl* RD = FD->getParent(); + intptr_t offset = + C.toCharUnitsFromBits( + C.getASTRecordLayout(RD).getFieldOffset(FD->getFieldIndex())) + .getQuantity(); + while (RD->isAnonymousStructOrUnion()) { + const clang::RecordDecl* anon = RD; + RD = llvm::dyn_cast(anon->getParent()); + for (auto f = RD->field_begin(); f != RD->field_end(); ++f) { + auto* rt = f->getType()->getAs(); + if (!rt) + continue; + if (anon == rt->getDecl()) { + FD = *f; + break; + } + } + offset += C.toCharUnitsFromBits(C.getASTRecordLayout(RD).getFieldOffset( + FD->getFieldIndex())) + .getQuantity(); + } + return offset; + } if (auto *VD = llvm::dyn_cast(D)) { auto GD = GlobalDecl(VD); diff --git a/unittests/CppInterOp/VariableReflectionTest.cpp b/unittests/CppInterOp/VariableReflectionTest.cpp index 2c42897dd..61dd98aa4 100644 --- a/unittests/CppInterOp/VariableReflectionTest.cpp +++ b/unittests/CppInterOp/VariableReflectionTest.cpp @@ -39,6 +39,77 @@ TEST(VariableReflectionTest, GetDatamembers) { EXPECT_EQ(datamembers1.size(), 0); } +#define CODE \ + struct Klass1 { \ + Klass1(int i) : num(1), b(i) {} \ + int num; \ + union { \ + double a; \ + int b; \ + }; \ + } k1(5); \ + struct Klass2 { \ + Klass2(double d) : num(2), a(d) {} \ + int num; \ + struct { \ + double a; \ + int b; \ + }; \ + } k2(2.5); \ + struct Klass3 { \ + Klass3(int i) : num(i) {} \ + int num; \ + struct { \ + double a; \ + union { \ + float b; \ + int c; \ + }; \ + }; \ + int num2; \ + } k3(5); + +CODE + +TEST(VariableReflectionTest, DatamembersWithAnonymousStructOrUnion) { + std::vector Decls; +#define Stringify(s) Stringifyx(s) +#define Stringifyx(...) #__VA_ARGS__ + GetAllTopLevelDecls(Stringify(CODE), Decls); +#undef Stringifyx +#undef Stringify +#undef CODE + + auto datamembers_klass1 = Cpp::GetDatamembers(Decls[0]); + auto datamembers_klass2 = Cpp::GetDatamembers(Decls[2]); + auto datamembers_klass3 = Cpp::GetDatamembers(Decls[4]); + + EXPECT_EQ(datamembers_klass1.size(), 3); + EXPECT_EQ(datamembers_klass2.size(), 3); + + EXPECT_EQ(Cpp::GetVariableOffset(datamembers_klass1[0]), 0); + EXPECT_EQ(Cpp::GetVariableOffset(datamembers_klass1[1]), + ((intptr_t) & (k1.a)) - ((intptr_t) & (k1.num))); + EXPECT_EQ(Cpp::GetVariableOffset(datamembers_klass1[2]), + ((intptr_t) & (k1.b)) - ((intptr_t) & (k1.num))); + + EXPECT_EQ(Cpp::GetVariableOffset(datamembers_klass2[0]), 0); + EXPECT_EQ(Cpp::GetVariableOffset(datamembers_klass2[1]), + ((intptr_t) & (k2.a)) - ((intptr_t) & (k2.num))); + EXPECT_EQ(Cpp::GetVariableOffset(datamembers_klass2[2]), + ((intptr_t) & (k2.b)) - ((intptr_t) & (k2.num))); + + EXPECT_EQ(Cpp::GetVariableOffset(datamembers_klass3[0]), 0); + EXPECT_EQ(Cpp::GetVariableOffset(datamembers_klass3[1]), + ((intptr_t) & (k3.a)) - ((intptr_t) & (k3.num))); + EXPECT_EQ(Cpp::GetVariableOffset(datamembers_klass3[2]), + ((intptr_t) & (k3.b)) - ((intptr_t) & (k3.num))); + EXPECT_EQ(Cpp::GetVariableOffset(datamembers_klass3[3]), + ((intptr_t) & (k3.c)) - ((intptr_t) & (k3.num))); + EXPECT_EQ(Cpp::GetVariableOffset(datamembers_klass3[4]), + ((intptr_t) & (k3.num2)) - ((intptr_t) & (k3.num))); +} + TEST(VariableReflectionTest, LookupDatamember) { std::vector Decls; std::string code = R"( From 846796b704b9ddb43f555a6305b3a8c8f5f06167 Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Thu, 12 Sep 2024 12:45:13 +0530 Subject: [PATCH 2/8] Apply suggestions from code review (github-actions bot) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- lib/Interpreter/CppInterOp.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Interpreter/CppInterOp.cpp b/lib/Interpreter/CppInterOp.cpp index c696e0bf8..9abd80afb 100644 --- a/lib/Interpreter/CppInterOp.cpp +++ b/lib/Interpreter/CppInterOp.cpp @@ -1132,7 +1132,7 @@ namespace Cpp { llvm::SmallVector stack_end; stack_begin.push_back(CXXRD->field_begin()); stack_end.push_back(CXXRD->field_end()); - while (stack_begin.size()) { + while (!stack_begin.empty()) { if (stack_begin.back() == stack_end.back()) { stack_begin.pop_back(); stack_end.pop_back(); @@ -1209,7 +1209,7 @@ namespace Cpp { const clang::RecordDecl* anon = RD; RD = llvm::dyn_cast(anon->getParent()); for (auto f = RD->field_begin(); f != RD->field_end(); ++f) { - auto* rt = f->getType()->getAs(); + const auto* rt = f->getType()->getAs(); if (!rt) continue; if (anon == rt->getDecl()) { From a2fd87e69cd344010a289bc4be956f75c1babe03 Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Thu, 12 Sep 2024 13:47:05 +0000 Subject: [PATCH 3/8] suggestions from code review & disable anonymous struct warning for test --- lib/Interpreter/CppInterOp.cpp | 23 +++++++++---------- unittests/CppInterOp/CMakeLists.txt | 4 ++++ .../CppInterOp/VariableReflectionTest.cpp | 6 ++--- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/Interpreter/CppInterOp.cpp b/lib/Interpreter/CppInterOp.cpp index 9abd80afb..9fc58930b 100644 --- a/lib/Interpreter/CppInterOp.cpp +++ b/lib/Interpreter/CppInterOp.cpp @@ -1139,19 +1139,18 @@ namespace Cpp { continue; } Decl* D = *(stack_begin.back()); - if (D->getKind() == clang::Decl::Field && - dyn_cast(D)->isAnonymousStructOrUnion()) { - if (auto* CXXRD = llvm::dyn_cast_or_null( - dyn_cast(D) - ->getType() - ->getAs() - ->getDecl())) { - stack_begin.back()++; - stack_begin.push_back(CXXRD->field_begin()); - stack_end.push_back(CXXRD->field_end()); - continue; + if (D->getKind() == clang::Decl::Field) { + if (auto* FD = llvm::dyn_cast(D)) { + if (FD->isAnonymousStructOrUnion()) { + if (auto* CXXRD = llvm::dyn_cast_or_null( + FD->getType()->getAs()->getDecl())) { + stack_begin.back()++; + stack_begin.push_back(CXXRD->field_begin()); + stack_end.push_back(CXXRD->field_end()); + continue; + } + } } - assert(false); // should be unreachable else internal error } datamembers.push_back((TCppScope_t)D); stack_begin.back()++; diff --git a/unittests/CppInterOp/CMakeLists.txt b/unittests/CppInterOp/CMakeLists.txt index 00e455605..2d8496d17 100644 --- a/unittests/CppInterOp/CMakeLists.txt +++ b/unittests/CppInterOp/CMakeLists.txt @@ -18,6 +18,10 @@ target_link_libraries(CppInterOpTests clangCppInterOp ) +set_source_files_properties(VariableReflectionTest.cpp PROPERTIES COMPILE_FLAGS + "-Wno-pedantic" +) + set_source_files_properties(InterpreterTest.cpp PROPERTIES COMPILE_DEFINITIONS "LLVM_BINARY_DIR=\"${LLVM_BINARY_DIR}\"" ) diff --git a/unittests/CppInterOp/VariableReflectionTest.cpp b/unittests/CppInterOp/VariableReflectionTest.cpp index 61dd98aa4..ddb07aab9 100644 --- a/unittests/CppInterOp/VariableReflectionTest.cpp +++ b/unittests/CppInterOp/VariableReflectionTest.cpp @@ -47,7 +47,7 @@ TEST(VariableReflectionTest, GetDatamembers) { double a; \ int b; \ }; \ - } k1(5); \ + } const k1(5); \ struct Klass2 { \ Klass2(double d) : num(2), a(d) {} \ int num; \ @@ -55,7 +55,7 @@ TEST(VariableReflectionTest, GetDatamembers) { double a; \ int b; \ }; \ - } k2(2.5); \ + } const k2(2.5); \ struct Klass3 { \ Klass3(int i) : num(i) {} \ int num; \ @@ -67,7 +67,7 @@ TEST(VariableReflectionTest, GetDatamembers) { }; \ }; \ int num2; \ - } k3(5); + } const k3(5); CODE From e837ec6fe67e2b3f77c238ccea58c66c10cdc390 Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Thu, 12 Sep 2024 13:53:17 +0000 Subject: [PATCH 4/8] disabling warning if not windows --- unittests/CppInterOp/CMakeLists.txt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/unittests/CppInterOp/CMakeLists.txt b/unittests/CppInterOp/CMakeLists.txt index 2d8496d17..0e155f709 100644 --- a/unittests/CppInterOp/CMakeLists.txt +++ b/unittests/CppInterOp/CMakeLists.txt @@ -18,9 +18,11 @@ target_link_libraries(CppInterOpTests clangCppInterOp ) -set_source_files_properties(VariableReflectionTest.cpp PROPERTIES COMPILE_FLAGS - "-Wno-pedantic" -) +if(NOT WIN32) + set_source_files_properties(VariableReflectionTest.cpp PROPERTIES COMPILE_FLAGS + "-Wno-pedantic" + ) +endif() set_source_files_properties(InterpreterTest.cpp PROPERTIES COMPILE_DEFINITIONS "LLVM_BINARY_DIR=\"${LLVM_BINARY_DIR}\"" From 34f0c4cfa653acf033ce49a3434db3024672ed28 Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Fri, 13 Sep 2024 03:33:31 +0000 Subject: [PATCH 5/8] Apply suggestions from code review --- lib/Interpreter/CppInterOp.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/lib/Interpreter/CppInterOp.cpp b/lib/Interpreter/CppInterOp.cpp index 9fc58930b..cdad4a71c 100644 --- a/lib/Interpreter/CppInterOp.cpp +++ b/lib/Interpreter/CppInterOp.cpp @@ -1139,16 +1139,14 @@ namespace Cpp { continue; } Decl* D = *(stack_begin.back()); - if (D->getKind() == clang::Decl::Field) { - if (auto* FD = llvm::dyn_cast(D)) { - if (FD->isAnonymousStructOrUnion()) { - if (auto* CXXRD = llvm::dyn_cast_or_null( - FD->getType()->getAs()->getDecl())) { - stack_begin.back()++; - stack_begin.push_back(CXXRD->field_begin()); - stack_end.push_back(CXXRD->field_end()); - continue; - } + if (auto* FD = llvm::dyn_cast(D)) { + if (FD->isAnonymousStructOrUnion()) { + if (auto* CXXRD = llvm::dyn_cast_or_null( + FD->getType()->getAs()->getDecl())) { + stack_begin.back()++; + stack_begin.push_back(CXXRD->field_begin()); + stack_end.push_back(CXXRD->field_end()); + continue; } } } From e8dd9a14a8a467698142e3e17f2ecc5b07c70ed3 Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Fri, 13 Sep 2024 06:13:21 +0000 Subject: [PATCH 6/8] suggestions from code review & skip test on valgrind --- lib/Interpreter/CppInterOp.cpp | 34 +++++++++---------- .../CppInterOp/VariableReflectionTest.cpp | 3 ++ 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/lib/Interpreter/CppInterOp.cpp b/lib/Interpreter/CppInterOp.cpp index cdad4a71c..7d94f93b3 100644 --- a/lib/Interpreter/CppInterOp.cpp +++ b/lib/Interpreter/CppInterOp.cpp @@ -1139,14 +1139,16 @@ namespace Cpp { continue; } Decl* D = *(stack_begin.back()); - if (auto* FD = llvm::dyn_cast(D)) { + if (auto* FD = llvm::dyn_cast_or_null(D)) { if (FD->isAnonymousStructOrUnion()) { - if (auto* CXXRD = llvm::dyn_cast_or_null( - FD->getType()->getAs()->getDecl())) { - stack_begin.back()++; - stack_begin.push_back(CXXRD->field_begin()); - stack_end.push_back(CXXRD->field_end()); - continue; + if (auto* RT = FD->getType()->getAs()) { + if (auto* CXXRD = + llvm::dyn_cast_or_null(RT->getDecl())) { + stack_begin.back()++; + stack_begin.push_back(CXXRD->field_begin()); + stack_end.push_back(CXXRD->field_end()); + continue; + } } } } @@ -1199,24 +1201,20 @@ namespace Cpp { if (auto* FD = llvm::dyn_cast(D)) { const clang::RecordDecl* RD = FD->getParent(); intptr_t offset = - C.toCharUnitsFromBits( - C.getASTRecordLayout(RD).getFieldOffset(FD->getFieldIndex())) - .getQuantity(); + C.toCharUnitsFromBits(C.getFieldOffset(FD)).getQuantity(); while (RD->isAnonymousStructOrUnion()) { const clang::RecordDecl* anon = RD; RD = llvm::dyn_cast(anon->getParent()); - for (auto f = RD->field_begin(); f != RD->field_end(); ++f) { - const auto* rt = f->getType()->getAs(); - if (!rt) + for (auto F = RD->field_begin(); F != RD->field_end(); ++F) { + const auto* RT = F->getType()->getAs(); + if (!RT) continue; - if (anon == rt->getDecl()) { - FD = *f; + if (anon == RT->getDecl()) { + FD = *F; break; } } - offset += C.toCharUnitsFromBits(C.getASTRecordLayout(RD).getFieldOffset( - FD->getFieldIndex())) - .getQuantity(); + offset += C.toCharUnitsFromBits(C.getFieldOffset(FD)).getQuantity(); } return offset; } diff --git a/unittests/CppInterOp/VariableReflectionTest.cpp b/unittests/CppInterOp/VariableReflectionTest.cpp index ddb07aab9..efb604cde 100644 --- a/unittests/CppInterOp/VariableReflectionTest.cpp +++ b/unittests/CppInterOp/VariableReflectionTest.cpp @@ -72,6 +72,9 @@ TEST(VariableReflectionTest, GetDatamembers) { CODE TEST(VariableReflectionTest, DatamembersWithAnonymousStructOrUnion) { + if (llvm::sys::RunningOnValgrind()) + GTEST_SKIP() << "XFAIL due to Valgrind report"; + std::vector Decls; #define Stringify(s) Stringifyx(s) #define Stringifyx(...) #__VA_ARGS__ From 25cf88d0e95292053525bc420f4322a472fbd8b0 Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Fri, 13 Sep 2024 12:07:39 +0530 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: Vassil Vassilev Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- lib/Interpreter/CppInterOp.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/Interpreter/CppInterOp.cpp b/lib/Interpreter/CppInterOp.cpp index 7d94f93b3..e4d6a7537 100644 --- a/lib/Interpreter/CppInterOp.cpp +++ b/lib/Interpreter/CppInterOp.cpp @@ -1139,11 +1139,11 @@ namespace Cpp { continue; } Decl* D = *(stack_begin.back()); - if (auto* FD = llvm::dyn_cast_or_null(D)) { + if (auto* FD = llvm::dyn_cast(D)) { if (FD->isAnonymousStructOrUnion()) { - if (auto* RT = FD->getType()->getAs()) { + if (const auto* RT = FD->getType()->getAs()) { if (auto* CXXRD = - llvm::dyn_cast_or_null(RT->getDecl())) { + llvm::dyn_cast(RT->getDecl())) { stack_begin.back()++; stack_begin.push_back(CXXRD->field_begin()); stack_end.push_back(CXXRD->field_end()); From 36f9aeee72bd4b3c58a46823b9cc7154520f2716 Mon Sep 17 00:00:00 2001 From: Vipul Cariappa Date: Fri, 13 Sep 2024 06:41:36 +0000 Subject: [PATCH 8/8] clang format change --- lib/Interpreter/CppInterOp.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/Interpreter/CppInterOp.cpp b/lib/Interpreter/CppInterOp.cpp index e4d6a7537..c91b26d79 100644 --- a/lib/Interpreter/CppInterOp.cpp +++ b/lib/Interpreter/CppInterOp.cpp @@ -1142,8 +1142,7 @@ namespace Cpp { if (auto* FD = llvm::dyn_cast(D)) { if (FD->isAnonymousStructOrUnion()) { if (const auto* RT = FD->getType()->getAs()) { - if (auto* CXXRD = - llvm::dyn_cast(RT->getDecl())) { + if (auto* CXXRD = llvm::dyn_cast(RT->getDecl())) { stack_begin.back()++; stack_begin.push_back(CXXRD->field_begin()); stack_end.push_back(CXXRD->field_end());