GrammaTech / gtirb

Intermediate Representation for Binary analysis and transformation
https://grammatech.github.io/gtirb/
Other
305 stars 36 forks source link

Call node destructors when freeing the Context. #28

Closed nweston closed 5 years ago

nweston commented 5 years ago

Adds a subclass of BumpPtrAllocatorImpl which only allocates a single type, allowing it to walk the slabs and destroy objects during its own destructor.

Context now has a separate allocator for each node type, with a templated Allocate method which is specialized to use the correct allocator.

The Create methods now call Context::Allocate explicitly, and use the built-in placement new rather than our own version. So this code:

return new (C) Module(C);

becomes this:

return new (C.Allocate<Module>()) Module(C);

This allows us to pass the type information down to the Context, so it can pick the right allocator.

There are two things which I don't really like about this design:

  1. DtorBumpPtrAllocator is very dependent on the details of BumpPtrAllocatorImpl. But the public API is insufficient for our purposes, and it seems better to use an existing allocator which is presumably well-tested than to start from scratch. And in any event we aren't likely to make a lot of changes to this code. So I think this is ultimately the right way to go.

  2. DtorBumpPtrAllocator is responsible for calling the destructors, but the various Create methods determine the type of the allocated object. So essentially, the two sides of the process are handled at different layers. One can imagine a copy-paste error leading to code like:

    return new (C.Allocate<Module>()) MyNode(C);

This won't be detected at compile time and could lead to subtle errors. But this seems to be an unavoidable consequence of the way placement new works.

nweston commented 5 years ago

I've replaced my allocator with SpecificBumpPtrAllocator from LLVM, which resolves all the comments about the specifics of the allocator. And I reworked the Create methods as we discussed yesterday.

nweston commented 5 years ago

All fixed now.