Skip to content

[core] Fix TDataMember::GetOffsetCint interference with GetOffset #23844

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
ravindra-RKB wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[core] Fix TDataMember::GetOffsetCint interference with GetOffset #23844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 13 additions & 4 deletions core/meta/src/TDataMember.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,12 @@ const char *TDataMember::GetTrueTypeName() const

Longptr_t TDataMember::GetOffset() const
{
if (fOffset>=0) return fOffset;
if (Property() & (kIsStatic | kIsEnum)) {
Error("GetOffset", "Cannot get offset of static or enum data member %s", GetName());
return -1;
}

if (fOffset >= 0) return fOffset;

R__LOCKGUARD(gInterpreterMutex);
//case of an interpreted or emulated class
Expand Down Expand Up @@ -495,13 +500,17 @@ Longptr_t TDataMember::GetOffset() const

Longptr_t TDataMember::GetOffsetCint() const
{
if (fOffset>=0) return fOffset;

@pcanal pcanal Oct 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not the best solution. The address of the enums and static should also be cached. The main issue is that GetOffset updates fOffset to a valid looking value instead of leaving it to -1 when called with static and enums.

@ravindra-RKB ravindra-RKB Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, now caching the address in fOffset within GetOffsetCint().

if (fOffset >= 0) return fOffset;

R__LOCKGUARD(gInterpreterMutex);
TDataMember *dm = const_cast<TDataMember*>(this);

if (dm->IsValid()) return gCling->DataMemberInfo_Offset(dm->fInfo);
else return -1;
if (dm->IsValid()) {
Longptr_t offset = gCling->DataMemberInfo_Offset(dm->fInfo);
dm->fOffset = offset;
return offset;
}
return -1;
Comment on lines +508 to +513

@pcanal pcanal Oct 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure this is the right solution. A priori the value can be as well cached for these call as for the previous call. It is not clear whether there is really a need for both functions to be distinct. However this need a deep analysis of the usage pattern in ROOT and in user codes.

@ravindra-RKB ravindra-RKB Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @pcanal. Should we cache this into a dedicated fOffsetCint member instead, or do you prefer consolidating both functions? Happy to adapt based on what fits best.

@pcanal pcanal Oct 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The firsts step is to do an analysis of whether it makes sense or not to merge the 2 or to protect them from being called in the wrong context.

@ravindra-RKB ravindra-RKB Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looking through the codebase usages:

  • GetOffset() is used for instance byte offsets (TStreamerInfo, RFieldMeta, TTree). For non-static data members, both functions yield the same offset.
  • For static variables and enum constants, GetOffset() returns 0 because BuildRealData() does not track them, whereas GetOffsetCint() queries Cling, which returns the address/value.
  • If called on an enum or static member, GetOffset() is technically the "wrong context" (no instance offset exists) and erroneously caches 0 into fOffset.

Would it make sense to protect GetOffset() so it returns -1 (or avoids setting fOffset = 0) when Property() & (kIsStatic | kIsEnum), or should GetOffset() delegate to Cling for static/enum members as well?

@pcanal pcanal Oct 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should indeed protect GetOffset against being called with static and enums. It should both return -1 and issue an error (as most code path do not check the return value).

GetOffsetCint (by which we meant GetOffsetDirectlyFromInterpreter) is an odd beast that returns either an offset compared to the start of the type it belongs to or an address depending on the use case. We should probably rename it to something that better express the semantic of its return value.

One special feature of GetOffset is that it works also for abstract classes (once a concrete derived class has its list of real data member created).

it may make sense to add a new function GetAddress that works only for static and enums (and does what GetOffsetCint does.

Side note: Cppyy::GetDatamemberOffset is the only non-test usage of GetOffsetCint in our code (who knows how it is used elsewhere).

@ravindra-RKB ravindra-RKB Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated as suggested: protected GetOffset() against static and enums to issue an error and return -1, and updated GetOffsetCint() to cache into fOffset. Also updated the regression test to verify both behaviors.

}

////////////////////////////////////////////////////////////////////////////////
Expand Down
18 changes: 18 additions & 0 deletions roottest/root/meta/unit/datamem.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,21 @@ const int ROOT8499<T>::s_info = ROOT8499<T>::initHelper();");
EXPECT_NE(0, dm->GetOffsetCint());
}

TEST(TDataMemberTests, Issue23777)
{
// Test that calling GetOffset() does not corrupt or interfere with GetOffsetCint()
TClass *cl = TClass::GetClass("TAttImage");
ASSERT_NE(nullptr, cl);
TObject *obj = cl->GetListOfDataMembers()->FindObject("kImgBest");
ASSERT_NE(nullptr, obj);
TDataMember *dm = dynamic_cast<TDataMember *>(obj);
ASSERT_NE(nullptr, dm);

EXPECT_STREQ("TAttImage::EImageQuality", dm->GetTypeName());
// Static and enum members do not have an instance offset; GetOffset() must return -1
EXPECT_EQ(-1, dm->GetOffset());
// GetOffsetCint() must return a non-zero address/offset from Cling even after GetOffset() was called
EXPECT_NE(0, dm->GetOffsetCint());
// GetOffset() must still return -1 even after GetOffsetCint() cached the address
EXPECT_EQ(-1, dm->GetOffset());
}