This is an archive of the discontinued LLVM Phabricator instance.

[llvm-debuginfo-analyzer] 03 - Logical elements
ClosedPublic

Authored by CarlosAlbertoEnciso on May 17 2022, 6:42 AM.

Details

Summary

LLVM debug information visual analyzer

For more information, see the RFC https://discourse.llvm.org/t/llvm-dev-rfc-llvm-dva-debug-information-visual-analyzer/62570

Patches:
01 - Interval Tree
02 - Driver and Documentation
03 - Logical Elements
04 - Locations and Ranges
05 - Select Elements
06 - Warning and Internal Options
07 - Compare Elements
08 - ELF Reader
09 - CodeView Reader
10 - Smart Pointers

This is a high level summary of the changes in this patch.

Logical elements

  • All basic functionality for the logical elements: LVScope, LVLine, LVSymbol, LVType.
  • The logical reader: LVReader.h

Diff Detail

Build Status
Buildable 188370

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
psamolysov added inline comments.May 24 2022, 6:30 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
90–101

Just a question. The LVScope class is not copyable at all. Why values of these LVAutoTypes, LVAutoSymbols etc. cannot be class members, not pointers to the values? At least, having the members as values eliminates a lot of if (Children) ... checks in the implementation.

If they cannot be just values, maybe it makes sense to use smart pointers here (std::unique_ptr?) instead of create the members with plain new and delete then in the destructor.

262

setReference(LVScope *) above is not marked as virtual, only override is used.

306

Is this a vector of indices?

478–479
llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
33
llvm/lib/DebugInfo/LogicalView/CMakeLists.txt
33

I see no headers in the Readers path in the patch. Should the line be moved in a next patch in the series or LVReader.h moved in the Readers directory?

llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp
432

Early return if the inverted condition takes place is more preferable by the coding style.

llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp
112
llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp
29
91

if unable?

116
llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
111–112

It definitely makes sense to swap these two lines.

135–136

It definitely makes sense to swap these two lines.

162–163

It definitely makes sense to swap these two lines.

189–190

It definitely makes sense to swap these two lines.

215

Element is just a pointer, it can be captured by value.

216

Only captured variables are Element that is a pointer and predicate that is a lambda with a very short capture list. Both can be captured by value but by reference is ok too.

255–256
261

Symbol is just a pointer and can be captured by value.

462

Are you sure the explicitly invoked constructor std::string(getName()) is required? It looks like this converts a StringRef into a temporary std::string, then makes a copy of the std::string and put the copy into the QualifiedName.append method. QualifiedName.append(getName()); should be enough, shouldn't it?

875

There is no notifications are sent below. Should this comment line be added to a next patch in the series?

903

It might work.

905
1162
1248

AddComma has already been initialized, this line looks as not required.

llvm/lib/DebugInfo/LogicalView/Core/LVSort.cpp
42

If getName() returns an llvm::StringRef then there is the operator< for llvm::StringRef, so there is no need to create a temporary std::string for LHS as well as RHS.

llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp
255
265

Just to try. llvm::StringRef should be convertible to std::string. If this works, the other creations of temporary std::string in this function will be able to be eliminated too.

llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp
65

A std::bad_alloc will be thrown if the test is out of memory. It makes sense to wrap the line in the try-catch block or use the non-throwing overload of the new operator.

probinson added inline comments.May 24 2022, 6:58 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h
110

Should Name be a reference parameter? Passing std::string by value could result in a creating/destroying a temp copy.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
223

When a declaration overrides an inherited virtual declaration, it really should use override to guarantee that the prior virtual declaration exactly matches. override implies virtual and so we omit virtual to reduce clutter.

psamolysov added inline comments.May 25 2022, 1:09 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h
110

Using std::move after getting a parameter by value has the following advantage: if the actual parameter (Name in our case) is a temporary or also moved value (rvalue?) so:

LVReader r;
r.setFilename(getFilename(...));

or

LVReader r;
std::string name = getFilename(...);
r.setFilename(std::move(name));

the move constructor of std::string will be called twice and the copy constructor will not be called at all. If we use pass-by-const-reference for the Name parameter in the setFilename method, the copy constructor will be called but only once. In the general case, move constructor is much cheaper and two calls of a move constructor could be more preferable than even a single call of a copy constructor but for std::string this is not obvious due to short string optimization. If filenames are usually short strings, shorter than 32 characters, using pass by reference for the Name looks better for me.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
20–24

Added the header file.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h
22

Added the header file.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
54

Added header file.

306

Yes. The vector contains indices into the String Pool table.
Filenames.push_back(getStringPool().getIndex(Name));

llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
33

Replaced.

llvm/lib/DebugInfo/LogicalView/CMakeLists.txt
33

Removed the line "${LLVM_MAIN_INCLUDE_DIR}/llvm/DebugInfo/LogicalView/Readers" and moved to next patch.

llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp
432

I see your point.

if (!(options().getPrintFormatting() && options().getAttributeAnySource() &&
      getFilenameIndex()))
  return;

But with the inverted condition, it seems the statetment gets obscure.

llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp
112

Corrected spelling.

llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp
29

Corrected spelling.

91

Correct. Fixed the spelling.

116

Corrected spelling.

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
111–112

Very good catch. Fixed.

135–136

Very good catch. Fixed.

162–163

Very good catch. Fixed.

189–190

Very good catch. Fixed.

875

Correct. That comment should be in the compare elements patch.

1162

Added the extra check.

1248

Moved the definition/initialization inside the if.

if (const LVSymbols *Symbols = getSymbols()) {
  bool AddComma = false;
llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp
255

Removed the extra the.

psamolysov added inline comments.Jun 27 2022, 3:14 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
306

Well, thank you for the answer.

llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp
432

I see such long ifs are actively used though the code. So, I think the code could be left as it was initially written for consistency.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h
69

Before uploading the patches we discussed the smart pointer usage across the tool but we decided to wait until all patches are accepted. After that. the next step is to remove the new/delete machinary across the tool.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
223

Omitting virtual.

262

Omitting virtual.

478–479

Omitting virtual.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h
74

Added the default value and removed virtual.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h
122

Each LVScope, LVType, LVSymbol and LVLine have a specific bit that gives more information.
A LVScope that represents a compile unit has the following bits:

KIND_3(LVScopeKind, IsCompileUnit, CanHaveRanges, CanHaveLines, TransformName);

Which are expanded to a series of get/set functions used to get specific information.

Added:

assert(Scope && Scope->isCompileUnit() && "Scope is not a compile unit");
llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
90–101

As the LVScope represents any scope, which can be a class, function, lexical scope, etc, in order to reduce the class size we use pointers to those LVAutoXXXX. For example:

class ClassPointer {
  void *Pointer = nullptr;
};
class ClassVector {
  SmallVector<void *,2> Vector; 
};
size_t ClassPointerSize = sizeof(ClassPointer);
size_t ClassVectorSize = sizeof(ClassVector);

ClassPointerSize = 8 bytes
ClassVectorSize = 32 bytes

90–101

Before uploading the patches we discussed the smart pointer usage across the tool but we decided to wait until all patches are accepted. After that. the next step is to remove the new/delete machinary across the tool.

204

Added:

// Sort the logical elements using the criteria specified by the
// command line option '--output-sort'.
306

Thanks.

llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp
432

Thanks

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
462

Way back the following upstream commits:

commit adcd02683856c30ba6f349279509acecd90063df
Author: Benjamin Kramer <benny.kra@googlemail.com>
Date:   Tue Jan 28 20:23:46 2020 +0100

    Make llvm::StringRef to std::string conversions explicit.
    
    This is how it should've been and brings it more in line with
    std::string_view. There should be no functional change here.
    
    This is mostly mechanical from a custom clang-tidy check, with a lot of
    manual fixups. It uncovers a lot of minor inefficiencies.
    
    This doesn't actually modify StringRef yet, I'll do that in a follow-up.

commit 777180a32b61070a10dd330b4f038bf24e916af1
Author: Benjamin Kramer <benny.kra@googlemail.com>
Date:   Tue Jan 28 23:30:02 2020 +0100

    [ADT] Make StringRef's std::string conversion operator explicit
    
    This has the same behavior as converting std::string_view to
    std::string. This is an expensive conversion, so explicit conversions
    are helpful for avoiding unneccessary string copies.

Introduced the need to use the explicit conversion.

903

See explanation about the needed explicit conversion.

905

Changed.

llvm/lib/DebugInfo/LogicalView/Core/LVSort.cpp
42

Very good catch. Changed to return LHS->getName() < RHS->getName();

llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp
265

See previous explanation for the needed explicit conversion.

psamolysov added inline comments.Jun 28 2022, 6:13 AM
llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
462

Hm... Sorry, I wasn't aware about this explicit conversion. Thank you for the explanation.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
190

Good question. We do not allow any copy/assignment of logical elements.

The only place where we allow copy an LVObject is in printAttributes when we need to print some modified attributes for the current object. Those modifications are temporary and are used to create a fake scope.

void LVObject::printAttributes(raw_ostream &OS, bool Full, StringRef Name,
                               LVObject *Parent, StringRef Value,
                               bool UseQuotes, bool PrintRef) const {
  // The current object will be the enclosing scope, use its offset and level.
  LVObject Object(*Parent);
  Object.setLevel(Parent->getLevel() + 1);
  Object.setLineNumber(0);
  Object.printAttributes(OS, Full);
  ..
}
llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h
110

Thanks for both great explanations.

Looking at the whole code setFilename is only used in one the unit tests. Replaced with std::move.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
213

Sorry, I'm afraid I don't follow you.

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
215

Capture Element by value.

auto Predicate = [Element](LVElement *Item) -> bool { return Item == Element; };
216

Capture both Element and Predicate by value.

auto RemoveElement = [Element, Predicate](auto &Container) -> bool {
255–256

Replaced with append.

261

Capture Symbol by value.

[Symbol](LVSymbol *Item) -> bool { return Item == Symbol; });
llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp
65

Changed to use the non-throwing overload.

psamolysov added inline comments.Jun 29 2022, 1:50 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
190

In this case, the copy constructor and assignment operator could be protected. Thank you for the explanation why the copy constructor is required but assignment operator is not.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
213

LVElement is a base class for LVScope and it already has the public method getCompileUnitParent. I cannot get why you override this method in the LVScope class if this just delegates a call into the method of the basic class? I guessed this is due some overload resolution problems and it could be better just to inject the getCompileUnitParent name into the scope using the using keyword:

using LVElement::getCompileUnitParent;

(sorry, () after the function's name is not needed).

But if you aren't going to change the logic of the LVScope::getCompileUnitParent method in future patches, even using is not required, I think. However, if you are, I believe it could be better to override the method in the patch where you change the logic, it makes no sense to have an override method which just delegates the call to the basic one.

psamolysov added inline comments.Jun 29 2022, 1:51 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
190

Sorry, protected or even private I mean.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
190

Marking the copy constructor as private.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
213

Thanks for the explanation.

The getCompileUnitParent is changed in a later patch Compare Elements.
Removed the LVScope::getCompileUnitParent method.

  • Addressed the reviewer’s feedback.
  • Tool renamed as: llvm-debuginfo-analyzer.
CarlosAlbertoEnciso retitled this revision from [llvm-dva] 03 - Logical elements to [llvm-debuginfo-analyzer] 03 - Logical elements.Jul 25 2022, 2:16 AM
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)
probinson added inline comments.Sep 16 2022, 2:40 PM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h
84
llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
155
llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
108

No ; after the {}

236
518

Did you mean "subroutine type"?

552

Needs comment saying what does LVScopeRoot represent.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
39

This appears to be assuming that the vector elements are pointers? And the memory for the pointed-to objects is owned by the vector? I'd think it would be simpler to have a SmallVector of objects, not pointers-to-objects, in which case SmallVector takes care of destroying them for you.
(Or, use a SmallVector of smart pointers.)

If you really want a SmallVector of normal pointers, then the template needs to guarantee that T is a pointer type, somehow. But this all feels rather fragile.

51

I think the static_cast<unsigned> should be unnecessary? because Idx has type T, therefore Idx < T::LastEntry should work correctly.
Same comment in the other methods.

Actually, you could defer the bounds checking to SmallBitVector's operator[] and remove a lot of this code. For example

void set(T Idx) {
  Bits[static_cast<unsigned>(Idx)] = 1;
}
64

This assert is redundant, SmallBitVector::operator[] does the same assertion.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h
88

This assertion looks a little strange. I think it's trying to verify that Element is either null, or is-a LVSymbol. In that case the assert would be
assert((!Element || dyn_cast<LVSymbol *>(Element)) && "Invalid element");

llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp
29

If the LVElement hierarchy is set up for LLVM-style casting, then this could be just return dyn_cast<LVType *>(ElementType);

34

If the LVElement hierarchy is set up for LLVM-style casting, then this could be just return dyn_cast<LVScope *>(ElementType);

98
llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp
41

Kind doesn't need to be static here.

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
57

Kind doesn't need to be static here.

412
639
653
923

Could this be const LVScope *Scope ? Which would avoid some casting in printSizes.

931

Did this solve a real problem? I would expect this expression to be folded to
rint((float(Size) / CUContributionSize) * 100.0)
but maybe my notion of floating-point constant folding is too naive.

948

Is there a reason this isn't a lambda?

Also, if the parameter is const then some of the casting below can go away.

970

Comments above would allow removing these const_cast calls.

1087

Looks like the method shouldn't be declared const. Ordinarily you wouldn't think a print() method would be non-const, but obviously this one is.

1146

This looks like it just repeats the comment from a few lines above.

llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp
24

There should be existing functions in Support for this.

55

While this is easy to understand, it's iterating the string 13 times. Maybe a lambda with a switch statement, and one call to transform ? Or a lambda that calls strpbrk.

llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp
36

Kind doesn't need to be static here.

llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp
49

Kind doesn't need to be static here.

91

This comment might belong in another patch? It doesn't seem to describe anything in this method.

135

Ignore the template parameters. (?)

274

Even though it's a one-line else clause, please put {} around it so that the next line doesn't look like incorrect indentation.

llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp
66

I think ASSERT_NE here would be better; bail out of the test in a controlled way if the new returns nullptr. Otherwise you'll just get a segfault somewhere later that will be harder to diagnose.

112

I think ASSERT_NE here too.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h
84

Changed.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
155

Changed.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
108

Changed.

236

Changed.

518

Yes. Changed to // Class to represent a DWARF subroutine type.

552

Added // Class to represent the binary file being analyzed.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
39

The LVAutoSmallVector is used to record the created logical elements (LVScope, LVSymbol, etc) which currently are created by new and to do the automatic destroy.

Once the patches are accepted, one the tasks is to use Small pointers across all patches and LVAutoSmallVector will be replaced with SmallVector of smart pointers.

Adding the check to guarantee that T is a pointer type.

51

Nice code reduction. Changed to use operator[].

64

Nice code reduction. Changed to use operator[] for the reset method as well.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h
88

Good point.
I think the correct form is:
assert((!Element || dyn_cast<LVSymbol>(Element)) && "Invalid element");

Changed.

llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp
29

I see your point. But in this case we are not relying on the LVElement hierarchy.

class LVElement {
  PROPERTY(Property, IsType);
  // Type of this element.
  LVElement *ElementType = nullptr;
};

We are returning the ElementType that can be a LVType or a LVScope and it is determined by the PROPERTY IsType/IsScope.

34

Same as above comments.

98

Changed.

llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp
41

Removed static.

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
57

Removed static.

412

Changed.

639

Changed.

653

Changed.

931

It solved the issue of differences in the percentage when analyzing the same binary on Windows and Linux. For some cases:

Expect value:
Totals by lexical level:
[001]:        106 (100.00%)
[002]:         71 ( 66.99%)    <---
[003]:         20 ( 18.87%)
Actual value
Totals by lexical level:
[001]:        106 (100.00%)
[002]:         71 ( 66.98%)    <---
[003]:         20 ( 18.87%)
1087

All the print methods for the logical elements are marked as const. Only this one requires to modify some class members (stats).

1146

Removed.

llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp
24

The only similar function in Support is Path.cpp::native

But we need additional changes to the given Path.
Added the comments for further clarification:

// Perform the following transformations to the given 'Path':
// - all characters to lowercase.
// - '\\' into '/' (Platform independent).
// - '//' into '/'
llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp
36

Removed static.

llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp
49

Removed static.

91

The comment is intended to explain that a LVType does not have references to any DW_AT_specification, DW_AT_abstract_origin, DW_AT_extension, etc.

135

Correct. Changed to Ignore the template parameters.

274

Added {}.

llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp
66

ASSERT_NE can be used only inside functions that don't return a value.

See https://google.github.io/googletest/advanced.html#assertion-placement

The one constraint is that assertions that generate a fatal failure (FAIL* and ASSERT_*) can only be used in void-returning functions.

112

ASSERT_NE can be used only inside functions that don't return a value.

See https://google.github.io/googletest/advanced.html#assertion-placement

The one constraint is that assertions that generate a fatal failure (FAIL* and ASSERT_*) can only be used in void-returning functions.

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
923

Changing to const LVScope *Scope and with additional couple minor changes, removed the casting in printSizes.

948

I had compiling issues while trying to use a recursive lambda.
Changing to const LVScope *Scope the castings are eliminated.

970

Done.

llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp
55

Good finding.
Replaced with a call to transform and a call to strpbrk.

// Convert the given 'Path' to lowercase and change any matching character
// from 'CharSet' into '_'.
// CharSet:
//   '/', '\', '<', '>', '.', ':', '%', '*', '?', '|', '"', ' '.
std::string llvm::logicalview::flattenedFilePath(StringRef Path) {
  std::string Name(Path);
  std::transform(Name.begin(), Name.end(), Name.begin(), tolower);

  const char *CharSet = "/\\<>.:%*?|\" ";
  char *Input = Name.data();
  while (Input && *Input) {
    Input = strpbrk(Input, CharSet);
    if (Input)
      *Input++ = '_';
  };
  return Name;
}

Address points raised by @probinson.

probinson accepted this revision.Sep 23 2022, 8:17 AM

Two nits and LGTM.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h
87

I forgot about isa<> when I made my previous comment. This does less work and is clearer to the reader.

llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp
112

createElements does return void so ASSERT_NE can be used here.

This revision is now accepted and ready to land.Sep 23 2022, 8:17 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h
87

Changed dyn_cast to isa.

llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp
112

Good point. Changed to ASSERT_NE.

Updated patch after '01 Interval Tree' was submitted.

Updated patch the removes some redundant public: modifiers.

This revision was landed with ongoing or failed builds.Oct 19 2022, 10:20 PM
This revision was automatically updated to reflect the committed changes.

@psamolysov , @probinson Thanks very much for all your valuable reviews.

dblaikie added inline comments.Nov 2 2022, 11:51 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
90–101

If this is the part of the thread you were referencing here https://reviews.llvm.org/D125783#inline-1320570 - I don't think this thread does an adequate job of discussing this issue to justify postponing it to after these patches have been approved or committed.

These patches should be/should have been made to LLVM coding standards before commit, not after - an internal discussion (I assume that's what you meant by "we" - some collection of folks who worked on/owned this project, but not the LLVM community at large/code owners?) isn't a justification for this sort of thing.

I'm pretty concerned about some things (partly as mentioned here: https://reviews.llvm.org/D125783#3893003 ) I'm seeing in these patches and the responses to both pre and post-commit review feedback.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
90–101

@dblaikie Thanks very much for your comments and I appreciate and understand your concerns and be sure I am not trying to ignore the LLVM policies in relation to reviews and code quality.

Taking into consideration your comments, I would like to ask you:

  • Do you want the tool removed so the memory management issues can be addressed and then a new patch series posted?

or

  • Would you be OK with a promise to address the issues ASAP without reverting everything first?

Thanks

CarlosAlbertoEnciso edited the summary of this revision. (Show Details)Nov 14 2022, 4:01 AM

@dblaikie A new patch has been uploaded that addresses the memory management issues: https://reviews.llvm.org/D137933 (10 Smart pointers).

With that patch the tool uses only smart pointers. That patch sits on top of https://reviews.llvm.org/D125784 (09 CodeView Reader).
Thanks