Re: [RFC PATCH] Support debuginfo and source file fetching via debuginfo server

[Simon Marchi <simark@simark.ca>]

On 2019-11-05 5:25 a.m., Frank Ch. Eigler wrote:
> Hi -
> 
>> Maybe a user_data void pointer would be useful, to be able to get some
>> context in the callback?
> 
> In your case, with a single-threaded app with C++ lambda captured
> variables, you wouldn't need the library to do that.  In a
> multithreaded app's case, __thread variables and the promise to call
> those functions back from the calling thread should again let one get
> context if required.

Hmm does it actually work to capture some variables in the C++ lambda?

Given that the callback is declared as a simple function pointer:

  64 typedef int (*debuginfod_progressfn_t)(long a, long b);
  65 debuginfod_progressfn_t debuginfod_set_progressfn(debuginfod_progressfn_t fn);

I'm not sure that we can pass a lambda that captures stuff.  Just like this program:

    static void func(void (*callback)(void)) { callback(); }
    int main() { int a; func([a] { }); return 0; }

Generates this error:

    test.cpp: In function ‘int main()’:
    test.cpp:2:33: error: cannot convert ‘main()::<lambda()>’ to ‘void (*)()’
        2 | int main() { int a; func([a] { }); return 0; }
          |                                 ^
          |                                 |
          |                                 main()::<lambda()>
    test.cpp:1:25: note:   initializing argument 1 of ‘void func(void (*)())’
        1 | static void func(void (*callback)(void)) { callback(); }
          |                  ~~~~~~~^~~~~~~~~~~~~~~

>> I am noticing that debuginfod calls don't have any "context" or "handle"
>> parameters, which could suggest that the state of the library (if there
>> is any) is kept as global variables.  [...]

I think that providing a void pointer for context is pretty standard thing
for C libraries that accept callbacks.  I would use __thread and the promise
to call the function back in the same thread to "retro-fit" some thread-safety
in an existing API that doesn't provide a context pointer, and that can't be
changed.  But if the API is new, I think the context pointer solution is more
obvious for the user.

> It turns out it's the opposite: except for this callback function
> pointer, it's practically all local variables therefore multithread-safe.

Ok, well I think it would be a good reason to adopt an API of the style:

    debuginfod_client *client = debuginfod_create_client ();
    debuginfod_set_progressfn (client, my_progress_cb);

and where you pass `client` to all further calls.  That client object would
keep the callback for those calls, but also any other state that you might
want to keep across in the future.  Again, this is easy to add in the
beginning, but hard to retro-fit later on.

Simon