[Lldb-commits] [PATCH] D77759: [lldb/Reproducers] Fix passive replay for functions returning string as (char*, size_t).

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

In D77759#1988419 <https://reviews.llvm.org/D77759#1988419>, @labath wrote:

> Hmm... well, I like how this (active&passive) is unified at the template level. It still feels like there's too much code to implement this thing (coming from the duplication for const, non-const and static variants), but I'm not sure what can be done about that. I'll need to ponder this a bit more


After sleeping on this a couple of times, I've come to believe that the problem here is that we've let the redirection functions inherit the c++ function pointer weirdness, which resulted in a need to write template goo for const, non-const and static methods (and maybe sometimes even constructors). The goo is (more or less) unavoidable if we want to have compiler-generated "identifiers" for all these functions, but there's a difference between writing that once, and once for each redirection we need to make. Particularly as it doesn't seem like there's a compelling reason for that. There no reason that the "redirected" function in the replayer machinery needs be globally unique in the same way as the "original" functions do. In fact I think it doesn't even have to be a template at all -- it could just take the function-to-wrap as a regular argument instead of a templated one. I believe `char_ptr_redirect<goo>::method<&goo>::record` could have been written as:

  // this handles const and non-const methods
  template<typename Result, typename This> // Maybe "Result" not even needed, if this is always the same type.
  Result char_ptr_redirect(Result (*f)(This *, char *, size_t), This *this_, char *ptr, size_t len) {
    char *buffer = reinterpret_cast<char *>(calloc(len, sizeof(char)));
    return f(this_, buffer, len)
  }
  // and another overload without This for static functions

Similar thing could be applied to the "replay" functions introduced here. Reduction from three overloads to two may not sound like much, but given that it would also remove the need for a lot of template goo (and reduce code size), I still find it interesting. It might be possible to also merge the two remaining overloads if we try hard enough.

Nonetheless, as long as we have only one kind of redirects (char_ptr thingy), it's benefit is not very clear, so I'm not going to insist on that. However, if more of these crop up, I think we should definitely revisit this.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D77759