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

[Support] Extend ExtensibleRTTI utility to support basic multiple inheritance. #112643

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

lhames
Copy link
Contributor

@lhames lhames commented Oct 17, 2024

Clients can now pass multiple parent classes to RTTIExtends. Each parent class will be inherited from publicly (and non-virtually). The isa, cast, and dyn_cast methods will now work as expected for types with multiple inheritance.

@llvmbot
Copy link
Member

llvmbot commented Oct 17, 2024

@llvm/pr-subscribers-llvm-support

Author: Lang Hames (lhames)

Changes

Clients can now pass multiple parent classes to RTTIExtends. Each parent class will be inherited from publicly (and non-virtually). The isa, cast, and dyn_cast methods will now work as expected for types with multiple inheritance.


Full diff: https://github.com/llvm/llvm-project/pull/112643.diff

2 Files Affected:

  • (modified) llvm/include/llvm/Support/ExtensibleRTTI.h (+21-9)
  • (modified) llvm/unittests/Support/ExtensibleRTTITest.cpp (+46-10)
diff --git a/llvm/include/llvm/Support/ExtensibleRTTI.h b/llvm/include/llvm/Support/ExtensibleRTTI.h
index d3193be6f529e1..6b8f5efd6bd81a 100644
--- a/llvm/include/llvm/Support/ExtensibleRTTI.h
+++ b/llvm/include/llvm/Support/ExtensibleRTTI.h
@@ -81,10 +81,6 @@ class RTTIRoot {
     return ClassID == classID();
   }
 
-  /// Check whether this instance is a subclass of QueryT.
-  template <typename QueryT>
-  bool isA() const { return isA(QueryT::classID()); }
-
 private:
   virtual void anchor();
 
@@ -110,21 +106,37 @@ class RTTIRoot {
 ///   static char ID;
 /// };
 ///
-template <typename ThisT, typename ParentT>
-class RTTIExtends : public ParentT {
+template <typename ThisT, typename... ParentTs>
+class RTTIExtends : public ParentTs... {
 public:
   // Inherit constructors from ParentT.
-  using ParentT::ParentT;
+  using ParentTs::ParentTs...;
 
   static const void *classID() { return &ThisT::ID; }
 
   const void *dynamicClassID() const override { return &ThisT::ID; }
 
+  /// Check whether this instance is a subclass of QueryT.
+  template <typename QueryT>
+  bool isA() const { return isA(QueryT::classID()); }
+
   bool isA(const void *const ClassID) const override {
-    return ClassID == classID() || ParentT::isA(ClassID);
+    return ClassID == classID() || parentsAreA<ParentTs...>(ClassID);
+  }
+
+  template <typename T>
+  static bool classof(const T *R) { return R->template isA<ThisT>(); }
+private:
+
+  template <typename T>
+  bool parentsAreA(const void *const ClassID) const {
+    return T::isA(ClassID);
   }
 
-  static bool classof(const RTTIRoot *R) { return R->isA<ThisT>(); }
+  template <typename T, typename T2, typename... Ts>
+  bool parentsAreA(const void *const ClassID) const {
+    return T::isA(ClassID) || parentsAreA<T2, Ts...>(ClassID);
+  }
 };
 
 } // end namespace llvm
diff --git a/llvm/unittests/Support/ExtensibleRTTITest.cpp b/llvm/unittests/Support/ExtensibleRTTITest.cpp
index 9715d26a9de297..4d781066903e88 100644
--- a/llvm/unittests/Support/ExtensibleRTTITest.cpp
+++ b/llvm/unittests/Support/ExtensibleRTTITest.cpp
@@ -36,15 +36,24 @@ class MyDeeperDerivedType
   static char ID;
 };
 
+class MyMultipleInheritanceType
+  : public RTTIExtends<MyMultipleInheritanceType,
+                       MyDerivedType, MyOtherDerivedType> {
+public:
+  static char ID;
+};
+
 char MyBaseType::ID = 0;
 char MyDerivedType::ID = 0;
 char MyOtherDerivedType::ID = 0;
 char MyDeeperDerivedType::ID = 0;
+char MyMultipleInheritanceType::ID = 0;
 
 TEST(ExtensibleRTTI, isa) {
   MyBaseType B;
   MyDerivedType D;
   MyDeeperDerivedType DD;
+  MyMultipleInheritanceType MI;
 
   EXPECT_TRUE(isa<MyBaseType>(B));
   EXPECT_FALSE(isa<MyDerivedType>(B));
@@ -60,26 +69,53 @@ TEST(ExtensibleRTTI, isa) {
   EXPECT_TRUE(isa<MyDerivedType>(DD));
   EXPECT_FALSE(isa<MyOtherDerivedType>(DD));
   EXPECT_TRUE(isa<MyDeeperDerivedType>(DD));
+
+  EXPECT_TRUE(isa<MyBaseType>(MI));
+  EXPECT_TRUE(isa<MyDerivedType>(MI));
+  EXPECT_TRUE(isa<MyOtherDerivedType>(MI));
+  EXPECT_FALSE(isa<MyDeeperDerivedType>(MI));
+  EXPECT_TRUE(isa<MyMultipleInheritanceType>(MI));
 }
 
 TEST(ExtensibleRTTI, cast) {
-  MyDerivedType D;
-  MyBaseType &BD = D;
-
-  (void)cast<MyBaseType>(D);
-  (void)cast<MyBaseType>(BD);
-  (void)cast<MyDerivedType>(BD);
+  MyMultipleInheritanceType MI;
+  MyDerivedType &D = MI;
+  MyOtherDerivedType &OD = MI;
+  MyBaseType &B = D;
+
+  EXPECT_EQ(&cast<MyBaseType>(D), &B);
+  EXPECT_EQ(&cast<MyDerivedType>(MI), &D);
+  EXPECT_EQ(&cast<MyOtherDerivedType>(MI), &OD);
+  EXPECT_EQ(&cast<MyMultipleInheritanceType>(MI), &MI);
 }
 
 TEST(ExtensibleRTTI, dyn_cast) {
-  MyBaseType B;
-  MyDerivedType D;
+  MyMultipleInheritanceType MI;
+  MyDerivedType &D = MI;
+  MyOtherDerivedType &OD = MI;
   MyBaseType &BD = D;
+  MyBaseType &BOD = OD;
 
-  EXPECT_EQ(dyn_cast<MyDerivedType>(&B), nullptr);
-  EXPECT_EQ(dyn_cast<MyDerivedType>(&D), &D);
   EXPECT_EQ(dyn_cast<MyBaseType>(&BD), &BD);
   EXPECT_EQ(dyn_cast<MyDerivedType>(&BD), &D);
+
+  EXPECT_EQ(dyn_cast<MyBaseType>(&BOD), &BOD);
+  EXPECT_EQ(dyn_cast<MyOtherDerivedType>(&BOD), &OD);
+
+  EXPECT_EQ(dyn_cast<MyBaseType>(&D), &BD);
+  EXPECT_EQ(dyn_cast<MyDerivedType>(&D), &D);
+  EXPECT_EQ(dyn_cast<MyMultipleInheritanceType>(&D), &MI);
+
+  EXPECT_EQ(dyn_cast<MyBaseType>(&OD), &BOD);
+  EXPECT_EQ(dyn_cast<MyOtherDerivedType>(&OD), &OD);
+  EXPECT_EQ(dyn_cast<MyMultipleInheritanceType>(&OD), &MI);
+
+  EXPECT_EQ(dyn_cast<MyDerivedType>(&MI), &D);
+  EXPECT_EQ(dyn_cast<MyMultipleInheritanceType>(&MI), &MI);
+
+  EXPECT_EQ(dyn_cast<MyDerivedType>(&MI), &D);
+  EXPECT_EQ(dyn_cast<MyOtherDerivedType>(&MI), &OD);
+  EXPECT_EQ(dyn_cast<MyMultipleInheritanceType>(&MI), &MI);
 }
 
 } // namespace

Copy link

github-actions bot commented Oct 17, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@lhames
Copy link
Contributor Author

lhames commented Oct 17, 2024

The motivating use-case is ORC memory management: We want JITLinkMemoryManagers to be able to inherit from additional interfaces (e.g. for allocating contiguous address ranges for each JITDylib), and to be able to check whether a given memory manager supports a given extended interface.

…eritance.

Clients can now pass multiple parent classes to RTTIExtends. Each parent class
will be inherited from publicly (and non-virtually). The isa, cast, and dyn_cast
methods will now work as expected for types with multiple inheritance.
@lhames lhames force-pushed the extensible-rtti-multiple-inheritance branch from 0953277 to be115ab Compare October 17, 2024 01:37
Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

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

Looks good - one optional/commentable thing with the ctor.

public:
// Inherit constructors from ParentT.
using ParentT::ParentT;
using ParentTs::ParentTs...;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this still apply - I guess it only works if you want to call one of the base ctors and all the other bases support default construction?

Probably OK as a first step - but maybe needs a FIXME to add something like std::pair's piecewise construct (though interesting that tuple doesn't have that... )

bool isA(const void *const ClassID) const override {
return ClassID == classID() || ParentT::isA(ClassID);
return ClassID == classID() || parentsAreA<ParentTs...>(ClassID);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be able to skip the parentsAreA helper:

return ClassId == classId() || (ParentTs::isA(ClassID) || ...);

(here's at least a little proof of concept I did: https://godbolt.org/z/vGca7jYbT )

Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

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

Re:single/first base class gets to expose it's non-defsult ctor and all other bases must have default ctors - sounds ok for a first pass.

@lhames lhames merged commit 633a6c9 into llvm:main Oct 24, 2024
8 checks passed
@lhames lhames deleted the extensible-rtti-multiple-inheritance branch October 24, 2024 22:53
@llvm-ci
Copy link
Collaborator

llvm-ci commented Oct 24, 2024

LLVM Buildbot has detected a new failure on builder openmp-s390x-linux running on systemz-1 while building llvm at step 6 "test-openmp".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/3856

Here is the relevant piece of the build log for the reference
Step 6 (test-openmp) failure: test (failure)
******************** TEST 'libomp :: tasking/issue-94260-2.c' FAILED ********************
Exit Code: -11

Command Output (stdout):
--
# RUN: at line 1
/home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp   -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src  -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic && /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# note: command had no output on stdout or stderr
# error: command failed with exit status: -11

--

********************


@frobtech frobtech mentioned this pull request Oct 25, 2024
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…eritance. (llvm#112643)

Clients can now pass multiple parent classes to RTTIExtends. Each parent
class will be inherited from publicly (and non-virtually). The isa,
cast, and dyn_cast methods will now work as expected for types with
multiple inheritance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants