[PATCH] D125784: [llvm-dva] 09 - CodeView Reader
Carlos Alberto Enciso via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 15 03:02:33 PDT 2022
CarlosAlbertoEnciso added inline comments.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewReader.h:184
+ Error createLines(const FixedStreamArray<LineNumberEntry> &LineNumbers,
+ LVAddress addendum, uint32_t Segment, uint32_t Begin,
+ uint32_t Size, uint32_t NameIndex,
----------------
psamolysov wrote:
>
Replaced.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:113
+ void printBinaryBlockWithRelocs(StringRef Label, ArrayRef<uint8_t> Block) {
+ StringRef SBlock(reinterpret_cast<const char *>(Block.data()),
+ Block.size());
----------------
psamolysov wrote:
> The code creates a temporary `StringRef` and does nothing more. Because this is the latest patch in the series, this code should be the final variant. What is this `StringRef` required for?
The original intention was to used it to dump (for debug traces) a section contents.
Removed that dead code.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:244
+// Visitor for CodeView types and symbols to populate elements.
+class LVLogicalVisitor {
+ LVCodeViewReader *Reader = nullptr;
----------------
psamolysov wrote:
> The class should be marked as `final` because there is no classes derived from. Or the destructor should be marked as `virtual` if some derived classes might be defined later.
Marked as `final`.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:300
+
+ // Input source in the case of type server o precompiled header.
+ void setInput(std::shared_ptr<llvm::pdb::InputFile> TypeServer) {
----------------
psamolysov wrote:
>
Corrected typo.
================
Comment at: llvm/lib/DebugInfo/CodeView/TypeRecordHelpers.cpp:54
+
+uint64_t llvm::codeview::getSize(TypeIndex TI) {
+ if (!TI.isSimple())
----------------
djtodoro wrote:
> CarlosAlbertoEnciso wrote:
> > djtodoro wrote:
> > > I guess this should be a standalone patch.
> > >
> > > What about `getSizeInBytes()` instead?
> > That is a very good point.
> > Would you prefer all the changes to `TypeRecordHelpers.cpp[h]` be moved in a standalone patch?
> > `getSizeInBytes()` sounds good.
> > Would you prefer all the changes to TypeRecordHelpers.cpp[h] be moved in a standalone patch?
> I vote for that.
I will prepare a standalone patch to include the changes to `TypeRecordHelpers.cpp[h]`.
================
Comment at: llvm/lib/DebugInfo/CodeView/TypeRecordHelpers.cpp:148
+uint64_t llvm::codeview::getSize(CVType CVT) {
+ switch (CVT.kind()) {
+ case LF_STRUCTURE:
----------------
Changed to `getSizeInBytes`.
================
Comment at: llvm/lib/DebugInfo/CodeView/TypeRecordHelpers.cpp:153
+ return getUdtSize<ClassRecord>(std::move(CVT));
+ break;
+ case LF_UNION:
----------------
psamolysov wrote:
> The `break` after `return` makes no sense.
Removed the `break`.
================
Comment at: llvm/lib/DebugInfo/CodeView/TypeRecordHelpers.cpp:156
+ return getUdtSize<UnionRecord>(std::move(CVT));
+ break;
+ default:
----------------
psamolysov wrote:
> The `break` after `return` makes no sense.
Removed the `break`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:111
+ StringRef InnerComponent;
+ std::tie(OuterComponent, InnerComponent) = getInnerComponent(Name);
+ setName(InnerComponent);
----------------
psamolysov wrote:
> `OuterComponent` has no usages here. The `std::ignore` placeholder can be used instead: https://en.cppreference.com/w/cpp/utility/tuple/ignore
Nice change.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp:61
+
+LexicalIndexes getAllLexicalIndices(StringRef Name) {
+ if (Name.empty())
----------------
psamolysov wrote:
> psamolysov wrote:
> > Just a nit about the naming. The type has the name `...Indexes` while the function has the name `...Indices`. I believe the same word should be used for the plural form of `Index`.
> The function is used in the `LVSupport.cpp` compilation unit, so it can be marked as `static`.
Changed to `getAllLexicalIndexes`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp:61
+
+LexicalIndexes getAllLexicalIndices(StringRef Name) {
+ if (Name.empty())
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > psamolysov wrote:
> > > Just a nit about the naming. The type has the name `...Indexes` while the function has the name `...Indices`. I believe the same word should be used for the plural form of `Index`.
> > The function is used in the `LVSupport.cpp` compilation unit, so it can be marked as `static`.
> Changed to `getAllLexicalIndexes`.
Marked as `static`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp:112-117
+ LLVM_DEBUG({
+ LexicalEntry Entry = Indexes.back();
+ llvm::dbgs() << formatv(
+ "'{0}:{1}', '{2}'\n", Entry.first, Entry.second,
+ Name.substr(Entry.first, Entry.second - Entry.first + 1));
+ });
----------------
psamolysov wrote:
> Just a nit, to eliminate the code duplication, a lambda with the body of these lines could be extracted and reused here and on line 96.
Created a lambda `PrintLexicalEntry`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:228
+ dbgs() << "\nSections Information:\n";
+ for (LVSections::value_type &Entry : Sections) {
+ LVSectionIndex SectionIndex = Entry.first;
----------------
psamolysov wrote:
>
Nice change.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:237
+ << format_decimal(SectionIndex, 3)
+ // << " Comdat: " << Section.isComdat()
+ << " Name: " << *SectionNameOrErr << "\n"
----------------
psamolysov wrote:
> It could be better to remove the commented code from the patch.
Removed commented code.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:248
+ dbgs() << "\nObject Section Information:\n";
+ for (const LVSectionAddresses::value_type &Entry : SectionAddresses)
+ dbgs() << "[" << hexValue(Entry.first) << ":"
----------------
psamolysov wrote:
>
Nice change.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:242
+ Name = Name.empty() ? Element->getName() : Name;
+ auto find = [&](const char *String) -> bool {
+ return StringRef::npos != Name.find(String);
----------------
psamolysov wrote:
> `StringRef` could be caught by value.
Changed to `[=]`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:245
+ };
+ auto starts = [&](const char *Pattern) -> bool {
+ return Name.startswith(Pattern);
----------------
psamolysov wrote:
> `StringRef` could be caught by value.
Changed to `[=]`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:310
+Error LVCodeViewReader::createLines(
+ const FixedStreamArray<LineNumberEntry> &LineNumbers, LVAddress addendum,
+ uint32_t Segment, uint32_t Begin, uint32_t Size, uint32_t NameIndex,
----------------
psamolysov wrote:
>
Changed to `Addendum`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:1127
+ for (const LineColumnEntry &Block : Lines)
+ if (Error Err = createLines(Block.LineNumbers, /*addendum=*/0, Segment,
+ Begin, Size, Block.NameIndex, &SG))
----------------
psamolysov wrote:
> If the corresponding change for the `createLines` function's signature is applied.
Changed to `/*Addendum=*/0`
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:373
+void LVStringRecords::addFilenames() {
+ for (const StringIds::value_type &Entry : Strings) {
+ StringRef Name = std::get<1>(Entry.second);
----------------
psamolysov wrote:
>
Nice change.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:382
+void LVStringRecords::addFilenames(LVScopeCompileUnit *Scope) {
+ for (StringIds::value_type &Entry : Strings)
+ if (!std::get<2>(Entry.second))
----------------
psamolysov wrote:
>
Nice change.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:1578-1580
+ StringRef InnerComponent;
+ StringRef OuterComponent;
+ std::tie(OuterComponent, InnerComponent) = getInnerComponent(Proc.Name);
----------------
psamolysov wrote:
>
Nice change.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:2250
+ // At this point the types recording the qualifiers do not have a
+ // scope parent. They must assigned to the current compile unit.
+ LVScopeCompileUnit *CompileUnit = Reader->getCompileUnit();
----------------
psamolysov wrote:
>
Corrected typo.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:2329
+ // At this point the types recording the qualifiers do not have a
+ // scope parent. They must assigned to the current compile unit.
+ LVScopeCompileUnit *CompileUnit = Reader->getCompileUnit();
----------------
psamolysov wrote:
>
Corrected typo.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:2332
+
+ // Orden for the different modifiers:
+ // <restrict> <pointer, Reference, ValueReference> <const, volatile>
----------------
psamolysov wrote:
> Order?
Corrected typo.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:2703-2705
+ StringRef InnerComponent;
+ StringRef OuterComponent;
+ std::tie(OuterComponent, InnerComponent) =
----------------
psamolysov wrote:
>
Nice change.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp:74
+ EXPECT_EQ(CompileUnit->getBaseAddress(), 0);
+ EXPECT_EQ(CompileUnit->getProducer().startswith("clang"), 1);
+ EXPECT_STREQ(CompileUnit->getName().data(), "test.cpp");
----------------
psamolysov wrote:
>
Changed to `EXPECT_TRUE`.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp:140
+ EXPECT_EQ(CompileUnit->getBaseAddress(), 0);
+ EXPECT_EQ(CompileUnit->getProducer().startswith("Microsoft"), 1);
+ EXPECT_STREQ(CompileUnit->getName().data(), "test.cpp");
----------------
psamolysov wrote:
>
Changed to `EXPECT_TRUE`.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp:206
+ EXPECT_EQ(CompileUnit->getBaseAddress(), 0);
+ EXPECT_EQ(CompileUnit->getProducer().startswith("Microsoft"), 1);
+ EXPECT_STREQ(CompileUnit->getName().data(), "test.cpp");
----------------
psamolysov wrote:
>
Changed to `EXPECT_TRUE`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125784/new/
https://reviews.llvm.org/D125784
More information about the llvm-commits
mailing list