[Evan Huus <eapache@xxxxxxxxx>]

On Sat, Oct 27, 2012 at 3:09 PM, Evan Huus <eapache@xxxxxxxxx> wrote:
> On Sat, Oct 27, 2012 at 2:17 PM, Dirk Jagdmann <doj@xxxxxxxxx> wrote:
>>> General thoughts from the list on whether or not this would be a good idea?
>>
>> some general comments on the whole wmem idea:
>> memory allocation is done almost everywhere in Wireshark, also in somewhat
>> hidden places. For example all the proto_tree_add_* functions will need to
>> allocate memory. It will be a real pain to pass down the current allocator
>> object into each of those function invocations, you're looking at changing a
>> considerable amount of code.
>
> Yes, it wouldn't be a small job.
>
>> Also when thinking about writing dissector code,
>> it's probably not useful to mix different allocators while dissecting a protocol.
>
> Not sure what you mean here. Mixing ep_ and se_ memory? Or mixing
> g_alloc and mmap memory? The separation of the allocators (glib, mmap,
> etc.) from the front-end isn't driven by a desire to mix and match
> allocators during a specific dissection (I agree that would be odd).
> It was more driven by the fact that the current allocators are all
> jumbled together and thus hard to maintain. It also makes it easy to
> add new scopes where necessary without writing hundreds of new wrapper
> functions.
>
>> An alternative idea (although a little more ugly) is to use a thread local
>> global variable which contains pointers to the current ep and se allocators. The
>> se_* and ep_* functions retrieve the allocator object from that thread local
>> global variable and do their allocations. If a function wants to change the
>> allocators for its own function calls, it can make a copy of the global variable
>> pointers to local variables, set it's desired new allocators and restore the
>> global pointers at the end, like:
>>
>> __thread void* se_allocator;
>> __thread void* ep_allocator;
>>
>> void my_special_func()
>> {
>>   void* old_se = se_allocator;
>>   void* old_ep = ep_allocator;
>>
>>   se_allocator = create_special_se();
>>   ep_allocator = create_special_ep();
>>   dissect_packet();
>>   garbage_collect(se_allocator);
>>   garbage_collect(ep_allocator);
>>
>>   se_allocator = old_se;
>>   pe_allocator = old_ep;
>> }
>
> This would certainly require fewer changes to existing code. It is,
> however, complicated by the lack of nice cross-platform API for
> thread-local storage. I just took a quick look at glib's gestures in
> this direction and they seem rather limited (a max of 128 thread-local
> objects, and they can't be destroyed).
>
> One of the things I was hoping to do by removing the allocators from
> the global scope was make it very obvious what the exact scopes of the
> various allocators are. Right now ep_ and se_ memory is used in a lot
> of places that aren't correct because the code has simply been able to
> grab the global. We really don't want anyone using ep_ memory unless
> there is actually a packet being dissected, and scoping the ep_ pool
> would enforce that nicely.

We might be able to fake the proper scoping using thread-local globals
if we wrap everything in functions that assert the state of a
dissection. Something like:

__thread wmem_allocator_t *packet_scope;
__thread gboolean packet_in_scope = FALSE;

wmem_allocator_t *
wmem_packet_scope(void)
{
  g_assert(packet_in_scope);
  return packet_scope;
}

void
wmem_start_packet_scope(void)
{
  g_assert(!packet_in_scope);
  packet_in_scope = TRUE;
}

void
wmem_stop_packet_scope(void)
{
  g_assert(packet_in_scope);
  packet_in_scope = FALSE;
  wmem_free_all(packet_scope);
}

Invalid accesses would still compile, but at least they would throw an
assertion as soon as the code path was hit. Thoughts on something like
this?