[PATCH] D39111: Extensible LLVM RTTI

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: Support/ExtensibleRTTITest.cpp:21
+public:
+  static char ID;
+};
----------------
lhames wrote:
> 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?
I don't know the exact details, but DLLs generally require explicit visibility rules and when a symbol is imported vs. exported. Getting weak functions uniqued across DLLs is solvable, but I've run into lots of problems trying to get variables/static data members uniqued. This is why I opted for a static variable inside an inline function, as that is guaranteed to be safe if the visibility is set properly. 

https://devblogs.microsoft.com/oldnewthing/20140109-00/?p=2123


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D39111