[PATCH] D137933: [llvm-debuginfo-analyzer] 08a - Memory Management

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

Hi @CarlosAlbertoEnciso and @dblaikie - I've had a look at this patch too and have left some inline nits / questions / suggestions.

> Is this API purely a "build/growth" without ever modifying? (like Clang's AST which uses bump/arena allocation - because you can't mutate the AST after it's built - you can't remove nodes, change nodes, etc)

This is the impression I get. AFAIK the tree is built and then printed, or compared with another tree, then the work is done and the nodes can be deleted (at once, together). Saying that, there is something interesting going on during the comparison where it looks like nodes are removed from one tree (which will be discarded) to another. Although some nodes are mutated in that case it doesn't involve deleting any nodes. I think the node-mutation works due to the fact that the containers used by nodes to point to their children are heap allocated using the standard allocator (i.e., some content of the nodes are spilled out from the bumpptrallocators into the general heap - this may be a necessary design choice, I'm not familiar enough to be able to say?).

FWIW I think this looks like a generally tidier memory management strategy than was previously used. It seems like a reasonable solution to me - it is now clear that all raw pointers are referencing memory owned by some other object (almost always the reader).



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVLocation.h:175
   // Location descriptors for the active range.
-  LVAutoOperations *Entries = nullptr;
+  using LVOperationSet = SmallVector<std::unique_ptr<LVOperation>, 8>;
+  std::unique_ptr<LVOperationSet> Entries;
----------------
Why is this using a `unique_ptr` - can or should this be a container of raw pointers, allocating the objects using the new bumpptr allocator?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:79
+// the debug information parsing. For its creation it uses a specific
+//  bump allocator for each type of logical element.
+// Define a specific bump allocator for the given KIND.
----------------
I think this comment should be copied or moved to the `class LVReader` level (and should be a doxygen comment  there - using `///` rather than `//`).


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:120-124
+  std::unique_ptr<LVTypes> Types;
+  std::unique_ptr<LVSymbols> Symbols;
+  std::unique_ptr<LVScopes> Scopes;
+  std::unique_ptr<LVLines> Lines;
+  std::unique_ptr<LVLocations> Ranges;
----------------
Not really related to the core issue being discussed here but do these members need to be heap allocated at all? Is the space saving between using `SmallVector<T, 0>` and `std::unique_ptr<SmallVector<T, N>` worth the additional complexity of having to explicably allocate the container when adding the first element? The answer might simply be yes.

In either case, if that change is worthwhile it doesn't need to be made in this patch.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:132
   //   the traversal does not require any specific element kind.
-  LVElements *Children = nullptr;
+  std::unique_ptr<LVElements> Children;
 
----------------
Another point for a future patch - as discussed offline it would probably be better to use a chaining iterator over the other containers rather than keep a separate container `Children` that mirrors their contents.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:103
 
+  std::vector<std::unique_ptr<LVLines>> DiscoveredLines;
+
----------------
What benefit do we get from `std::vector<std::unique_ptr<LVLines>>` over `std::vector<LVLines>`? Are the elements compared by address?

EDIT: see comment later on asking if we actually need this container at all.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:431
+  LVLines *Instructions = InstructionsPtr.get();
+  DiscoveredLines.emplace_back(std::move(InstructionsPtr));
 
----------------
CarlosAlbertoEnciso wrote:
> dblaikie wrote:
> > What's the ownership model for "Instructions"/why did `DiscoveredLines` get introduced in this patch? (was there a memory ownership issue in the raw-new-using code that this is addressing? Otherwise I Wouldn't expect to see new containers in this patch)
> The raw-new-using code stored those 'Instructions' in the 'LVDoubleMap` container. It was not very clear.
> Now they are stored at the `Reader` scope.
Why do we need `DiscoveredLines` at all now? Since the reader now owns the line objects when `createLineAssembler` is called below. 


================
Comment at: llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp:109-110
   // Create the logical types.
-  IntegerType = create<LVType>();
-  UnsignedType = create<LVType>();
-  GlobalType = create<LVType>();
-  LocalType = create<LVType>();
-  NestedType = create<LVType>();
-  EnumeratorOne = create<LVTypeEnumerator>();
-  EnumeratorTwo = create<LVTypeEnumerator>();
-  TypeDefinitionOne = create<LVTypeDefinition>();
-  TypeDefinitionTwo = create<LVTypeDefinition>();
-  TypeSubrange = create<LVTypeSubrange>();
-  TypeParam = create<LVTypeParam>();
-  TypeImport = create<LVTypeImport>();
+  IntegerType = createType();
+  EXPECT_NE(IntegerType, nullptr);
+  UnsignedType = createType();
----------------
I think a macro that combined `Z = createX(); EXPECT_NE(Z, nullptr)` would've been reasonable in order to reduce the line count of this change. It's probably not worth going back and adding retrospectively though.


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

https://reviews.llvm.org/D137933