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

Mon Jan 23 05:27:25 PST 2023

CarlosAlbertoEnciso added inline comments.

================
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;
----------------
Orlando wrote:
> 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?
Good observation. Changed to use the `bumpptr` mechanism.

================
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.
----------------
Orlando wrote:
> I think this comment should be copied or moved to the `class LVReader` level (and should be a doxygen comment  there - using `///` rather than `//`).
Moved to the `class LVReader` level.

================
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;
----------------
Orlando wrote:
> 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.
The issue is the space saving:

`SmallVector<T, 0>` --> size: 16
`std::unique_ptr<SmallVector<T, N>` --> size: 8

================
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;

----------------
Orlando wrote:
> 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.
Good point. This is something that it should be changed as it would reduce the space usage.
A future patch is the best way to proceed.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:103

+  std::vector<std::unique_ptr<LVLines>> DiscoveredLines;
+
----------------
Orlando wrote:
> 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.
To answer both questions, this is the logic behind the debug and assembler line processing:

In `createScopes` for each Compile Unit:
- call `traverseDieAndChildren` to parse the debug info
- Call `createLineAndFileRecords` that creates `LVLineDebug`s for each entry in the debug line section
- call `createInstructions` that creates a vector of `LVLineAssembler` for the associated text section.
  Its unique pointer is stored in `DiscoveredLines` and never updated.
  Its raw pointer is stored in `ScopeInstructions`.
  The `AssemblerMappings` is updated to record specific section index, current logical scope and text address.
- call `addSectionRange` to collect all debug ranges
- call `processLines` to allocate the collected lines (debug and assembler) to their logical scope.
  It uses the `ScopeInstructions` to locate specific assembler lines using data from `AssemblerMappings`.
  It moves lines (debug and assembler) to their logical scope.

At the end of the scopes creation, `DiscoveredLines` constains the unique pointers to empty vector.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:431
+  LVLines *Instructions = InstructionsPtr.get();
+  DiscoveredLines.emplace_back(std::move(InstructionsPtr));

----------------
Orlando wrote:
> 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. 
See previous comment that explains the role of `DiscoveredLines`.

================
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();
----------------
Orlando wrote:
> 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.
I think creating a macro that combines the creation and check is worth it, as it reduces the extra noise introduced by the current patch.

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

https://reviews.llvm.org/D137933