Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix GetVariableOffset in cases of multi-level and multiple inheritance #396

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion include/clang/Interpreter/CppInterOp.h
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,8 @@ namespace Cpp {

/// Gets the address of the variable, you can use it to get the
/// value stored in the variable.
CPPINTEROP_API intptr_t GetVariableOffset(TCppScope_t var);
CPPINTEROP_API intptr_t GetVariableOffset(TCppScope_t var,
TCppScope_t parent = nullptr);

/// Checks if the provided variable is a 'Public' variable.
CPPINTEROP_API bool IsPublicVariable(TCppScope_t var);
Expand Down
61 changes: 53 additions & 8 deletions lib/Interpreter/CppInterOp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "llvm/Support/Debug.h"
#include "llvm/Support/raw_os_ostream.h"

#include <map>
#include <set>
#include <sstream>
#include <string>
Expand Down Expand Up @@ -1222,20 +1223,22 @@
return 0;
}

intptr_t GetVariableOffset(compat::Interpreter& I, Decl* D) {
intptr_t GetVariableOffset(compat::Interpreter& I, Decl* D,
CXXRecordDecl* BaseCXXRD) {
if (!D)
return 0;

auto& C = I.getSema().getASTContext();

if (auto* FD = llvm::dyn_cast<FieldDecl>(D)) {
const clang::RecordDecl* RD = FD->getParent();
clang::RecordDecl* FieldParentRecordDecl = FD->getParent();
intptr_t offset =
C.toCharUnitsFromBits(C.getFieldOffset(FD)).getQuantity();
while (RD->isAnonymousStructOrUnion()) {
const clang::RecordDecl* anon = RD;
RD = llvm::dyn_cast<RecordDecl>(anon->getParent());
for (auto F = RD->field_begin(); F != RD->field_end(); ++F) {
while (FieldParentRecordDecl->isAnonymousStructOrUnion()) {
clang::RecordDecl* anon = FieldParentRecordDecl;
FieldParentRecordDecl = llvm::dyn_cast<RecordDecl>(anon->getParent());
for (auto F = FieldParentRecordDecl->field_begin();
F != FieldParentRecordDecl->field_end(); ++F) {
const auto* RT = F->getType()->getAs<RecordType>();
if (!RT)
continue;
Expand All @@ -1246,6 +1249,46 @@
}
offset += C.toCharUnitsFromBits(C.getFieldOffset(FD)).getQuantity();
}
if (BaseCXXRD && BaseCXXRD != FieldParentRecordDecl) {
// FieldDecl FD belongs to some class C, but the base class BaseCXXRD is
// not C. That means BaseCXXRD derives from C. Offset needs to be
// calculated for Derived class

// Depth first Search is performed to the class that declears FD from
// the base class
std::vector<CXXRecordDecl*> stack;
std::map<CXXRecordDecl*, CXXRecordDecl*> direction;
Vipul-Cariappa marked this conversation as resolved.
Show resolved Hide resolved
stack.push_back(BaseCXXRD);
while (!stack.empty()) {
CXXRecordDecl* RD = stack.back();
stack.pop_back();
size_t num_bases = GetNumBases(RD);
bool flag = false;
for (size_t i = 0; i < num_bases; i++) {
auto* CRD = static_cast<CXXRecordDecl*>(GetBaseClass(RD, i));
direction[CRD] = RD;
if (CRD == FieldParentRecordDecl) {
flag = true;
break;
}
stack.push_back(CRD);
}
if (flag)
break;
}
if (auto* RD = llvm::dyn_cast<CXXRecordDecl>(FieldParentRecordDecl)) {
// add in the offsets for the (multi level) base classes
while (BaseCXXRD != RD) {
CXXRecordDecl* Parent = direction.at(RD);
offset += C.getASTRecordLayout(Parent)
.getBaseClassOffset(RD)
.getQuantity();
RD = Parent;
}
} else {
assert(false && "Unreachable");

Check warning on line 1289 in lib/Interpreter/CppInterOp.cpp

View check run for this annotation

Codecov / codecov/patch

lib/Interpreter/CppInterOp.cpp#L1289

Added line #L1289 was not covered by tests
}
}
return offset;
}

Expand Down Expand Up @@ -1321,9 +1364,11 @@
return 0;
}

intptr_t GetVariableOffset(TCppScope_t var) {
intptr_t GetVariableOffset(TCppScope_t var, TCppScope_t parent) {
auto* D = static_cast<Decl*>(var);
return GetVariableOffset(getInterp(), D);
auto* RD =
llvm::dyn_cast_or_null<CXXRecordDecl>(static_cast<Decl*>(parent));
return GetVariableOffset(getInterp(), D, RD);
}

// Check if the Access Specifier of the variable matches the provided value.
Expand Down
84 changes: 84 additions & 0 deletions unittests/CppInterOp/VariableReflectionTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

#include "gtest/gtest.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'gtest/gtest.h' file not found [clang-diagnostic-error]

#include "gtest/gtest.h"
         ^


#include <cstddef>

using namespace TestUtils;
using namespace llvm;
using namespace clang;
Expand Down Expand Up @@ -258,6 +260,88 @@ TEST(VariableReflectionTest, GetVariableOffset) {
EXPECT_TRUE((bool) Cpp::GetVariableOffset(VD_C_s_a));
}

#define CODE \
class BaseA { \
public: \
virtual ~BaseA() {} \
int a; \
BaseA(int a) : a(a) {} \
}; \
\
class BaseB : public BaseA { \
public: \
virtual ~BaseB() {} \
std::string b; \
BaseB(int x, std::string b) : BaseA(x), b(b) {} \
}; \
\
class Base1 { \
public: \
virtual ~Base1() {} \
int i; \
std::string s; \
Base1(int i, std::string s) : i(i), s(s) {} \
}; \
\
class MyKlass : public BaseB, public Base1 { \
public: \
virtual ~MyKlass() {} \
int k; \
MyKlass(int k, int i, int x, std::string b, std::string s) \
: BaseB(x, b), Base1(i, s), k(k) {} \
} my_k(5, 4, 3, "Cpp", "Python");

CODE
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 'b' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:270: expanded from macro 'CODE'

    BaseB(int x, std::string b) : BaseA(x), b(b) {}                            \
                                              ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 's' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:277: expanded from macro 'CODE'

    Base1(int i, std::string s) : i(i), s(s) {}                                \
                                          ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: pass by value and use std::move [modernize-pass-by-value]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:270: expanded from macro 'CODE'

    BaseB(int x, std::string b) : BaseA(x), b(b) {}                            \
                 ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'my_k' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:285: expanded from macro 'CODE'

  } my_k(5, 4, 3, "Cpp", "Python");
    ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'Base1' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:275: expanded from macro 'CODE'

  class Base1 {                                                                \
        ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'BaseA' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:261: expanded from macro 'CODE'

  class BaseA {                                                                \
        ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'BaseB' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:268: expanded from macro 'CODE'

  class BaseB : public BaseA {                                                 \
        ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'MyKlass' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:283: expanded from macro 'CODE'

  class MyKlass : public BaseB, public Base1 {                                 \
        ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 'b' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:272: expanded from macro 'CODE'

    BaseB(int x, std::string b) : BaseA(x), b(b) {}                            \
                                              ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 's' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:280: expanded from macro 'CODE'

    Base1(int i, std::string s) : i(i), s(s) {}                                \
                                          ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: pass by value and use std::move [modernize-pass-by-value]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:272: expanded from macro 'CODE'

    BaseB(int x, std::string b) : BaseA(x), b(b) {}                            \
                 ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'my_k' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:289: expanded from macro 'CODE'

  } my_k(5, 4, 3, "Cpp", "Python");
    ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'Base1' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:277: expanded from macro 'CODE'

  class Base1 {                                                                \
        ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'BaseA' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:263: expanded from macro 'CODE'

  class BaseA {                                                                \
        ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'BaseB' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:270: expanded from macro 'CODE'

  class BaseB : public BaseA {                                                 \
        ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'MyKlass' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:285: expanded from macro 'CODE'

  class MyKlass : public BaseB, public Base1 {                                 \
        ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 'b' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:274: expanded from macro 'CODE'

    BaseB(int x, std::string b) : BaseA(x), b(b) {}                            \
                                              ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 's' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:282: expanded from macro 'CODE'

    Base1(int i, std::string s) : i(i), s(s) {}                                \
                                          ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: pass by value and use std::move [modernize-pass-by-value]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:274: expanded from macro 'CODE'

    BaseB(int x, std::string b) : BaseA(x), b(b) {}                            \
                 ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'my_k' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

CODE
^
Additional context

unittests/CppInterOp/VariableReflectionTest.cpp:291: expanded from macro 'CODE'

  } my_k(5, 4, 3, "Cpp", "Python");
    ^


TEST(VariableReflectionTest, VariableOffsetsWithInheritance) {
if (llvm::sys::RunningOnValgrind())
GTEST_SKIP() << "XFAIL due to Valgrind report";

Cpp::Declare("#include<string>");

#define Stringify(s) Stringifyx(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function-like macro 'Stringify' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]

#define Stringify(s) Stringifyx(s)
        ^

#define Stringifyx(...) #__VA_ARGS__
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variadic macro 'Stringifyx' used; consider using a 'constexpr' variadic template function [cppcoreguidelines-macro-usage]

#define Stringifyx(...) #__VA_ARGS__
        ^

Cpp::Declare(Stringify(CODE));
#undef Stringifyx
#undef Stringify
#undef CODE

Cpp::TCppScope_t myklass = Cpp::GetNamed("MyKlass");
EXPECT_TRUE(myklass);

size_t num_bases = Cpp::GetNumBases(myklass);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "size_t" is directly included [misc-include-cleaner]

unittests/CppInterOp/VariableReflectionTest.cpp:6:

+ #include <cstddef>

EXPECT_EQ(num_bases, 2);

std::vector<Cpp::TCppScope_t> datamembers;
Cpp::GetDatamembers(myklass, datamembers);
for (size_t i = 0; i < num_bases; i++) {
Cpp::TCppScope_t base = Cpp::GetBaseClass(myklass, i);
EXPECT_TRUE(base);
for (size_t i = 0; i < Cpp::GetNumBases(base); i++) {
Cpp::TCppScope_t bbase = Cpp::GetBaseClass(base, i);
EXPECT_TRUE(base);
Cpp::GetDatamembers(bbase, datamembers);
}
Cpp::GetDatamembers(base, datamembers);
}
EXPECT_EQ(datamembers.size(), 5);

EXPECT_EQ(Cpp::GetVariableOffset(datamembers[0], myklass),
((intptr_t)&(my_k.k)) - ((intptr_t)&(my_k)));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

            ((intptr_t)&(my_k.k)) - ((intptr_t)&(my_k)));
                                     ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

            ((intptr_t)&(my_k.k)) - ((intptr_t)&(my_k)));
             ^


EXPECT_EQ(Cpp::GetVariableOffset(datamembers[1], myklass),
((intptr_t)&(my_k.a)) - ((intptr_t)&(my_k)));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

            ((intptr_t)&(my_k.a)) - ((intptr_t)&(my_k)));
                                     ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

            ((intptr_t)&(my_k.a)) - ((intptr_t)&(my_k)));
             ^


EXPECT_EQ(Cpp::GetVariableOffset(datamembers[2], myklass),
((intptr_t)&(my_k.b)) - ((intptr_t)&(my_k)));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

            ((intptr_t)&(my_k.b)) - ((intptr_t)&(my_k)));
                                     ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

            ((intptr_t)&(my_k.b)) - ((intptr_t)&(my_k)));
             ^


EXPECT_EQ(Cpp::GetVariableOffset(datamembers[3], myklass),
((intptr_t)&(my_k.i)) - ((intptr_t)&(my_k)));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

            ((intptr_t)&(my_k.i)) - ((intptr_t)&(my_k)));
                                     ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

            ((intptr_t)&(my_k.i)) - ((intptr_t)&(my_k)));
             ^


EXPECT_EQ(Cpp::GetVariableOffset(datamembers[4], myklass),
((intptr_t)&(my_k.s)) - ((intptr_t)&(my_k)));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

            ((intptr_t)&(my_k.s)) - ((intptr_t)&(my_k)));
                                     ^

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

            ((intptr_t)&(my_k.s)) - ((intptr_t)&(my_k)));
             ^

}

TEST(VariableReflectionTest, IsPublicVariable) {
std::vector<Decl *> Decls, SubDecls;
std::string code = R"(
Expand Down