[PATCH] D39111: Extensible LLVM RTTI

[Lang Hames via Phabricator via llvm-commits]

lhames marked 2 inline comments as done.
lhames added inline comments.


================
Comment at: Support/ExtensibleRTTITest.cpp:21
+public:
+  static char ID;
+};
----------------
rriddle wrote:
> chandlerc wrote:
> > lhames wrote:
> > > chandlerc wrote:
> > > > dblaikie wrote:
> > > > > These chars could be provided in the CRTP base, perhaps? Less boilerplate. (though this would mean they'd have linkonce linkage, rather than having strong external definitions - but I doubt that duplication in objects would be too painful?)
> > > > > 
> > > > > Could even be local to the RTTIExtends::classID function (& dynamicClassID would just call classID)
> > > > Sadly, this will (I suspect) run into the non-conforming implementation of static data members of class templates on MSVC between DLLs. Specifically, the address of ID will be different between two DLLs. =[
> > > > 
> > > > I'm actually a bit hesitant with this entire thing as a consequence. We need to really clearly document the hard constraints on how this CRTP base is used as a consequence of this (you cannot use this as part of class templates unless you explicitly instantiate all specializations of them).
> > > > Sadly, this will (I suspect) run into the non-conforming implementation of static data members of class templates on MSVC between DLLs. Specifically, the address of ID will be different between two DLLs. =[
> > > 
> > > I'm not familiar with this. Is it only a problem for static data members on class templates, or does it apply to static members of ordinary classes too?
> > > 
> > > In this case MyBaseClass is an ordinary class, so I would have hoped it would be safe.
> > > 
> > > I had originally defined ID in the RTTIExtends class template so the user didn't have to write anything at all, but ran in to an issue (similar to the one you described) with MinGW, so abandoned that idea.
> > > 
> > Only an issue for static data members on class templates.
> > 
> > So, as long as the types providing the IDs are non-templates and you're getting the correct one's address via a virtual function call, this should work out fine IIRC. But moving them into the CRTP base class won't work. =/
> This is a solvable problem so long as we have the ability to properly annotate with dllexport/dllimport. I did an annoying amount of DLL testing when writing mlir::TypeID, but we don't have a way to do that in LLVM right now AFAIK so I've punted on doing that. 
> 
> (https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Support/TypeID.h) 
Interesting. I'd never looked in to *why* weak symbols weren't uniqued across DLLs. Is it just because they weren't visible by default?


================
Comment at: llvm/lib/Support/CMakeLists.txt:94
   ErrorHandling.cpp
+  ExtensibleRTTI.cpp
   FileCheck.cpp
----------------
sammccall wrote:
> This file doesn't exist, so removed this line in dffbeffa39f5625fab98ca203b0922e0ba14d06b
Thanks Sam. File and line have been added back in 255cc202ea.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D39111/new/

https://reviews.llvm.org/D39111