Thank you for the update Jonas. I haven't looked at the patch in detail,
but (for better or worse) it seems more-or-less like what I would expect
at a first glance.
One of the things that occurred to me while looking at this is that it
would be great to be able to test this code in greater isolation instead
of just the full "run lldb.exe" test. (I've already said this on the
file provider patch, but I think that is doubly true here.) There
shouldn't be anything (except the prolific use of the string SB) tying
this code to the SB api, and so we should be able to use it to
record/replay a dummy api for testing purposes.
That would make it easier to test some of the trickier corner cases. For
instance you could use placement new to make sure you get two objects at
the same address instead of hoping that this will eventually happen
during the full lldb run. It would also enable you to make incremental
patches while you work out the remaining issues instead of having to
have a fully working system from the get-go.
I think the only big change that you would need to make for this to work
is to move the core of this code out of the API folder (maybe all the
way down to Utility?), as in the way the build is set up now, it won't
get exported in a way that can be used by anyone else.
I think the patch tool you've made strikes a good compromise between
ease of use and ease of developing it. I expect it will be particularly
useful during the initial stages, when you need to annotate a large
number of functions in batches.
On 17/01/2019 03:29, Jonas Devlieghere wrote:
I've put up a (WIP) patch for the tool (https://reviews.llvm.org/D56822)
in case anybody is curious about that.
On Tue, Jan 15, 2019 at 1:41 PM Jonas Devlieghere <jo...@devlieghere.com
<mailto:jo...@devlieghere.com>> wrote:
I've updated the patch with a new version of the prototype:
https://reviews.llvm.org/D56322
It uses Pavel's suggestion to use the function address as a runtime
ID. All the deserialization code is generated using templates, with
automatic mapping on indices during serialization and deserialization.
I (again) manually added the macros for the same set of functions I
had in the original prototype. Unsurprisingly this is very
error-prone. It's easy to forget to add the right macros for the
registry, the function, and the return type. Some of these things
can be detected at compile time, other only blow up at run-time. I
strongly believe that a tool to add the macros is the way forward.
It would be more of a developer tool rather than something that
hooks up in the build process.
Note that it's still a prototype, there are outstanding issues like
void pointers, callbacks and other types of argument that require
some kind of additional information to serialize. I also didn't get
around yet to the lifetime issue yet that was discussed on IRC last
week.
Please let me know what you think.
Thanks,
Jonas
On Wed, Jan 9, 2019 at 8:58 AM Jonas Devlieghere
<jo...@devlieghere.com <mailto:jo...@devlieghere.com>> wrote:
On Wed, Jan 9, 2019 at 8:42 AM Pavel Labath <pa...@labath.sk
<mailto:pa...@labath.sk>> wrote:
On 09/01/2019 17:15, Jonas Devlieghere wrote:
>
>
> On Wed, Jan 9, 2019 at 5:05 AM Pavel Labath
<pa...@labath.sk <mailto:pa...@labath.sk>
> <mailto:pa...@labath.sk <mailto:pa...@labath.sk>>> wrote:
>
> On 08/01/2019 21:57, Jonas Devlieghere wrote:
> > Before I got around to coding this up I realized
you can't take the
> > address of constructors in C++, so the function
address won't
> work as an
> > identifier.
> >
>
> You gave up way too easily. :P
>
>
> I counted on you having something in mind, it sounded too
obvious for
> you to have missed. ;-)
>
> I realized that constructors are going to be tricky,
but I didn't want
> to dive into those details until I knew if you liked
the general idea.
> The most important thing to realize here is that for
the identifier
> thingy to work, you don't actually need to use the
address of that
> method/constructor as the identifier. It is
sufficient to have
> something
> that can be deterministically computed from the
function. Then you can
> use the address of *that* as the identifier.
>
>
> I was thinking about that yesterday. I still feel like it
would be
> better to have this mapping all done at compile time. I
was considering
> some kind of constexpr hashing but that sounded overkill.
>
Well.. most of this is done through template
meta-programming, which
_is_ compile-time. And the fact that I have a use for the new
construct/invoke functions I create this way means that even
the space
used by those isn't completely wasted (although I'm sure
this could be
made smaller with hard-coded IDs). The biggest impact of
this I can
think of is the increased number of dynamic relocations that
need to be
performed by the loader, as it introduces a bunch of
function pointers
floating around. But even that shouldn't too bad as we have
plenty of
other sources of dynamic relocs (currently about 4% of the
size of
liblldb and 10% of lldb-server).
Yeah of course, it wasn't my intention to critique your
approach. I was talking specifically about the mapping (the
std::map) in the prototype, something I asked about earlier in
the thread. FWIW I think this would be an excellent trade-off is
we don't need a tool to generate code for us. I'm hopeful that
we can have the gross of the deserialization code generated this
way, most of the "special" cases are still pretty similar and
dealing with basic types. Anyway, that should become clear later
today as I integrate this into the lldb prototype.
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
