[PATCH] D48807: Add llvm::Any

[Zachary Turner via Phabricator via llvm-commits]

zturner added a comment.

Repeating my previous reply since it was through email and didn't make it onto Phab.

In https://reviews.llvm.org/D48807#1150029, @dblaikie wrote:

> In https://reviews.llvm.org/D48807#1149775, @zturner wrote:
>
> > I've wanted this for quite a long time.  My original motivation for this was back when i was working primarily in LLDB.  There's a lot of places where `void*` is passed around as a generic parameter to a user callback.  So, for example, you say `DoSomethingAsynchronously(Callback, Param)` and when the thing is finished, it invokes `Callback(Param)`.  There are obviously better ways to do this if you're designing this from the beginning, but it's not always easy to retrofit that into an existing design.  So I think several places in LLDB could be cleaned up by replacing some `void*` with `Any`.  Note those places still exist today, so `Any` could be useful there right now.
>
>
> Would it be expensive to reimplement those APIs using stateful functors while providing the backwards compatibility with something like:
>
>   void register(void (*f)(void*), void *v) { register([f, v] { f(v); }); }
>   
>
> Or is there something in the implementation (rather than the existing usage) that makes migrating difficult?


It’s been a while since I was close to any of this code so it’s hard to say

> 
> 
>> More recently while designing a basic API for a tracing library, I've gravitated towards a design where the library generates events (represented by some structure) depending on what happens.  But since tracing is heavily platform dependent, there's only so much of this that you can abstract away into a common structure and there will be some additional pieces that only make sense on the particular platform.  So you call:
>> 
>>   unique_ptr<EventBase> Event = llvm::trace::WaitForEvent();
>>   // Do something depending on what kind of event it is.
>>   llvm::trace::ContinueFromEvent(std::move(Event));
>> 
>> 
>> In order for every possible platform to implement this correctly, there may need to be some platform-specific state maintained between the call to Wait and the call to Continue.  Linux might need to pass the exact signal value and parameters.  Windows might need to pass the exact `EXCEPTION_RECORD` structure <https://docs.microsoft.com/en-us/windows/desktop/api/winnt/ns-winnt-_exception_record> that the OS filled out for us right before we return from `WaitForEvent()`.  It's useful to be able to put that into an `Any`, so that we have something like:
>> 
>>   class EventBase {
>>     TraceEventType Type;
>>     uint64_t ThreadId;
>>     uint64_t ProcessId;
>>     Any PlatformPiece;
>>   };
>>   
>>   class ThreadCreatedEvent : public EventBase {
>>     void *EntryPoint;
>>     void *TlsArea;
>>     // Etc.
>>   };
>>   
>>   class BreakpointEvent : public EventBase {
>>     uint64_t Address;
>>   };
>> 
>> 
>> and so forth and so on.  When creating this struct before returning from `WaitForEvent()`, the library can set `PlatformPiece` to some structure that would contain any platform-specific state needed to properly continue.  I prefer not to use inheritance and `llvm::cast` here (e.g. `WindowsBreakpointEvent`, `PosixBreakpointEvent`, etc) because it just leads to inheritance hierarchy explosion and ultimately it's no more type-safe than `any` (someone still has to make an assumption about exactly what type it is).
> 
> If this is host-platform specific, why would this need to be runtime variable at all? Wouldn't the PlatformPiece be hardcoded to some specific layout based on the host platform the compiler was built for/running on?

Not necessarily.  For example you may have an x64 build of llvm tracing a 32 bit process.  This can arise on Windows (wow64) as well as MachO (universal binaries).  Maybe other platforms as well.  You could probably narrow it to a variant, but we don’t have variant in llvm either and it’s much harder to implement and im not sure it buys much.

Also, in a system where events are passed around and potentially held onto by user code for a long time, it’s useful to have a generic “Tag” field that the user can attach arbitrary data to. This is another potential use case which I didn’t mention before.

these 2-3 use cases aside, given that it’s in c++17, seems like filling as many of the missing gaps as possible between our support libraries and c++17 can only be a good thing


https://reviews.llvm.org/D48807