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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:103
 
+  std::vector<std::unique_ptr<LVLines>> DiscoveredLines;
+
----------------
Orlando wrote:
> CarlosAlbertoEnciso wrote:
> > Orlando wrote:
> > > CarlosAlbertoEnciso wrote:
> > > > 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.
> > > >  It moves lines (debug and assembler) to their logical scope.
> > > 
> > > Where does this happen? I can't see any uses of `DiscoveredLines` in this patch other than where entries are added. Sorry if this is a silly question - just trying to get my head around this field still.
> > This function creates `instructions` (LVLineAssembler) for the specific `SectionIndex` in the `.text` section. Those
> > instructions are associated with the logica 'Scope'.
> > ```
> > Error LVBinaryReader::createInstructions(LVScope *Scope,
> >                                          LVSectionIndex SectionIndex,
> >                                          const LVNameInfo &NameInfo) {
> >   ...
> >   std::unique_ptr<LVLines> InstructionsSP = std::make_unique<LVLines>();
> >   LVLines *Instructions = InstructionsSP.get();
> >   DiscoveredLines.emplace_back(std::move(InstructionsSP));
> >   ...
> >   // Traverse the .text section and create the assembler lines.
> >   // Adding each line to the 'Instructions' vector.
> >   Instructions->push_back(Line);
> >   ...
> >   // Create the following association:
> >   // the logical 'Scope' for the function in the .text section
> >   // identified with 'SectionIndex', has the assembler lines
> >   // contained in 'Instructions'.
> >   ScopeInstructions.add(SectionIndex, Scope, Instructions);
> > 
> >   // Note: The last item in 'DiscoveredLines' is the smart
> >   // pointer to the allocated vector 'InstructionsSP' and its
> >   // raw pointer is 'Instructions'.
> > }
> > ```
> > During the traversal of the debug information sections, we created the logical lines representing the disassembled instructions from the text section and the logical lines representing the line records from the debug line section. Using the ranges associated with the logical scopes, we will allocate those logical lines to their logical scopes.
> > ```
> > void LVBinaryReader::processLines(LVLines *DebugLines,
> >                                   LVSectionIndex SectionIndex,
> >                                   LVScope *Function) {
> >   ...
> >   // The 'DebugLines' vector contains the debug lines
> >   // for the current compile unit.
> >   ...
> >   // Get the associated instructions for the found 'Scope'.
> >   LVLines InstructionLines;
> >   LVLines *Lines = ScopeInstructions.find(SectionIndex, Scope);
> >   if (Lines)
> >     InstructionLines = std::move(*Lines);
> >   // Note: We get the vector of instructions from 'ScopeInstructions'
> >   // using the 'SectionIndex' and 'Scope' and move its contents
> >   // to a local working vector 'InstructionLines'.
> >   // 'ScopeInstructions' was updated in 'createInstructions'.
> >   ...
> >   for (LVLine *InstructionLine : InstructionLines) {
> >      ...
> >      // Using the 'address' information, insert some
> >      // of these 'InstructionLine' into the 'DebugLines'.
> >      // Basically merge some 'Instructions' with the
> >      // lines representing the .debug_line records.
> >      DebugLines->push_back(InstructionLine);
> >      ...
> >   }
> >   ...
> >   // Get 'ranges' information.
> >   LVRange *ScopesWithRanges = getSectionRanges(SectionIndex);
> >   ScopesWithRanges->startSearch();
> >   ...
> >   for (LVLine *Line : *DebugLines) {
> >     ...
> >     // Add line object to scope.
> >     Scope->addElement(Line);
> >     ...
> >   }
> > 
> > 
> > ```
> > This function creates the logical scopes for all the compilation units.
> > ```
> > Error LVELFReader::createScopes() {
> >   ...
> >   // Traverse compile units.
> >   for (const std::unique_ptr<DWARFUnit> &CU : CompileUnits) {
> >     ...
> >     traverseDieAndChildren(CUDie, Root, SkeletonDie);
> >     ...
> >     // Create debug lines for current compile unit and
> >     // store them in the 'CULines' vector. 
> >     createLineAndFileRecords(DwarfContext->getLineTableForUnit(CU.get()));
> >     ...
> >     // Create instruction lines
> >     createInstructions()
> >     ...
> >     processLines(&CULines, SectionIndex);
> >     ...
> >     CULines.clear();
> >   }
> > }
> > ```
> > 
> > The moving of the lines (debug and/or assembler) is done in `LVBinaryReader::processLines`.
> > 
> > 
> Thanks a lot for the detailed breakdown. I fundamentally misunderstood what `DiscoveredLines` represented and had misread its type as `vector<unique_ptr<Line*>>` (which is incorrect) rather than `vector<unique_ptr<vector<Line*>>>`. I'm happy with this now.
Any chance of avoiding the extra unique_ptr indirection in there, and having a `vector<vector<Line*>>` instead?


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

https://reviews.llvm.org/D137933