Skip to content

Fix: resolve nested attributes of anonymous records in GetDatamembers #321

New issue

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

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

Already on GitHub? # to your account

Merged
merged 8 commits into from
Sep 13, 2024

Conversation

Vipul-Cariappa
Copy link
Collaborator

No description provided.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 24. Check the log or trigger a new build to see more.

@Vipul-Cariappa
Copy link
Collaborator Author

The test fail with ISO C++ prohibits anonymous structs [-Werror=pedantic], and one of the tests has an anonymous struct.
We have a similar anonymous struct in the cppyy tests.
Should I add -Wno-pedantic to the compiler flags?

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 16. Check the log or trigger a new build to see more.

Comment on lines 1142 to 1143
if (D->getKind() == clang::Decl::Field &&
dyn_cast<FieldDecl>(D)->isAnonymousStructOrUnion()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (D->getKind() == clang::Decl::Field &&
dyn_cast<FieldDecl>(D)->isAnonymousStructOrUnion()) {
if (isa<FieldDecl>(D) &&
cast<FieldDecl>(D)->isAnonymousStructOrUnion()) {

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have a proper dyn_cast and isAnonymousStructOrUnion as a nested if...

@vgvassilev
Copy link
Contributor

The test fail with ISO C++ prohibits anonymous structs [-Werror=pedantic], and one of the tests has an anonymous struct. We have a similar anonymous struct in the cppyy tests. Should I add -Wno-pedantic to the compiler flags?

Yes, but only for this test file.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@Vipul-Cariappa
Copy link
Collaborator Author

Hi @vgvassilev, Valgrind fails. Should I add

if (llvm::sys::RunningOnValgrind())
    GTEST_SKIP() << "XFAIL due to Valgrind report";

?

It is Conditional jump or move depends on uninitialised value(s) at LLVM's code base, according to my eyes.

continue;
}
Decl* D = *(stack_begin.back());
if (D->getKind() == clang::Decl::Field) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (D->getKind() == clang::Decl::Field) {

const clang::RecordDecl* RD = FD->getParent();
intptr_t offset =
C.toCharUnitsFromBits(
C.getASTRecordLayout(RD).getFieldOffset(FD->getFieldIndex()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
C.getASTRecordLayout(RD).getFieldOffset(FD->getFieldIndex()))
C.getFieldOffset(FD)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

if (auto* FD = llvm::dyn_cast<FieldDecl>(D)) {
if (FD->isAnonymousStructOrUnion()) {
if (auto* CXXRD = llvm::dyn_cast_or_null<CXXRecordDecl>(
FD->getType()->getAs<RecordType>()->getDecl())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: Called C++ object pointer is null [clang-analyzer-core.CallAndMessage]

                    FD->getType()->getAs<RecordType>()->getDecl())) {
                    ^
Additional context

lib/Interpreter/CppInterOp.cpp:1128: Assuming 'D' is a 'CastReturnType'

    if (auto *CXXRD = llvm::dyn_cast_or_null<CXXRecordDecl>(D)) {
                      ^

lib/Interpreter/CppInterOp.cpp:1128: 'CXXRD' is non-null

    if (auto *CXXRD = llvm::dyn_cast_or_null<CXXRecordDecl>(D)) {
              ^

lib/Interpreter/CppInterOp.cpp:1128: Taking true branch

    if (auto *CXXRD = llvm::dyn_cast_or_null<CXXRecordDecl>(D)) {
    ^

lib/Interpreter/CppInterOp.cpp:1134: Loop condition is true. Entering loop body

      while (!stack_begin.empty()) {
      ^

lib/Interpreter/CppInterOp.cpp:1135: Taking false branch

        if (stack_begin.back() == stack_end.back()) {
        ^

lib/Interpreter/CppInterOp.cpp:1141: Assuming 'D' is a 'CastReturnType'

        if (auto* FD = llvm::dyn_cast<FieldDecl>(D)) {
                       ^

lib/Interpreter/CppInterOp.cpp:1141: 'FD' is non-null

        if (auto* FD = llvm::dyn_cast<FieldDecl>(D)) {
                  ^

lib/Interpreter/CppInterOp.cpp:1141: Taking true branch

        if (auto* FD = llvm::dyn_cast<FieldDecl>(D)) {
        ^

lib/Interpreter/CppInterOp.cpp:1142: Assuming the condition is true

          if (FD->isAnonymousStructOrUnion()) {
              ^

lib/Interpreter/CppInterOp.cpp:1142: Taking true branch

          if (FD->isAnonymousStructOrUnion()) {
          ^

lib/Interpreter/CppInterOp.cpp:1144: Assuming the object is not a 'const RecordType *'

                    FD->getType()->getAs<RecordType>()->getDecl())) {
                    ^

lib/Interpreter/CppInterOp.cpp:1144: Called C++ object pointer is null

                    FD->getType()->getAs<RecordType>()->getDecl())) {
                    ^

@vgvassilev
Copy link
Contributor

Hi @vgvassilev, Valgrind fails. Should I add

if (llvm::sys::RunningOnValgrind())
    GTEST_SKIP() << "XFAIL due to Valgrind report";

?

It is Conditional jump or move depends on uninitialised value(s) at LLVM's code base, according to my eyes.

Yes, please do.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

if (auto* FD = llvm::dyn_cast<FieldDecl>(D)) {
const clang::RecordDecl* RD = FD->getParent();
intptr_t offset =
C.toCharUnitsFromBits(C.getFieldOffset(FD)).getQuantity();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'uint64_t' (aka 'unsigned long') to signed type 'int64_t' (aka 'long') is implementation-defined [cppcoreguidelines-narrowing-conversions]

          C.toCharUnitsFromBits(C.getFieldOffset(FD)).getQuantity();
                                ^

break;
}
}
offset += C.toCharUnitsFromBits(C.getFieldOffset(FD)).getQuantity();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'uint64_t' (aka 'unsigned long') to signed type 'int64_t' (aka 'long') is implementation-defined [cppcoreguidelines-narrowing-conversions]

        offset += C.toCharUnitsFromBits(C.getFieldOffset(FD)).getQuantity();
                                        ^

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.69%. Comparing base (ea73105) to head (36f9aee).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   73.44%   73.69%   +0.25%     
==========================================
  Files           8        8              
  Lines        2990     3019      +29     
==========================================
+ Hits         2196     2225      +29     
  Misses        794      794              
Files with missing lines Coverage Δ
lib/Interpreter/CppInterOp.cpp 79.54% <100.00%> (+0.34%) ⬆️
Files with missing lines Coverage Δ
lib/Interpreter/CppInterOp.cpp 79.54% <100.00%> (+0.34%) ⬆️

Vipul-Cariappa and others added 2 commits September 13, 2024 12:07
Co-authored-by: Vassil Vassilev <v.g.vassilev@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Lgtm!

@vgvassilev vgvassilev merged commit 33546ba into compiler-research:main Sep 13, 2024
59 checks passed
Vipul-Cariappa added a commit to Vipul-Cariappa/cppyy-compiler-research that referenced this pull request Sep 17, 2024
Vipul-Cariappa added a commit to Vipul-Cariappa/cppyy-compiler-research that referenced this pull request Sep 19, 2024
Vipul-Cariappa added a commit to Vipul-Cariappa/cppyy-compiler-research that referenced this pull request Sep 19, 2024
aaronj0 pushed a commit to compiler-research/cppyy that referenced this pull request Sep 19, 2024
@Vipul-Cariappa Vipul-Cariappa deleted the Anonymous branch September 28, 2024 04:16
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants