[DebugInfo] Fix DIE value emitters to be compatible with DWARF64 (2/19).

[ikudrin]

Summary

DW_FORM_sec_offset and DW_FORM_strp imply values of different sizes with DWARF32 and DWARF64. The patch fixes DIE value classes to use correct sizes when emitting their values. For DIELocList it ensures that the requested DWARF form matches the current DWARF format because that class uses a method that selects the size automatically.

[dblaikie]

Might be too fine-grained and better off rolled into whatever patch exercises this functionality when emitting assembly.

llvm/lib/CodeGen/AsmPrinter/DIE.cpp

479

did one of your other patches added a function to AsmPrinter to get the size directly, rather than having to query the version then compute the size from that? (or maybe you added that to MCStreamer) Would be nice to avoid rewriting this code to compute size from DWARF64-ness.

[ikudrin]

• Changed AP->isDwarf64() ? 8 : 4 to AP->getDwarfOffsetByteSize().

[ikudrin]

In D87009#2252622, @dblaikie wrote:

Might be too fine-grained and better off rolled into whatever patch exercises this functionality when emitting assembly.

That would require to update a particular method several times and potentially would leave it in a partially updated state because some code paths are hard to reach. I believe that the way of this patch is simple and straightforward, as it changes easily observable code places and ensures that they are now accurate and can be reliably used from the other code parts.

[dblaikie]

In D87009#2253973, @ikudrin wrote:

In D87009#2252622, @dblaikie wrote:

Might be too fine-grained and better off rolled into whatever patch exercises this functionality when emitting assembly.

That would require to update a particular method several times and potentially would leave it in a partially updated state because some code paths are hard to reach.

Could you explain this in more detail?

I'm not really comfortable with updating the size calculation while not updating the emission calculation - that seems like this code is then currently unusable as-committed, so not a great granularity to commit at. I really appreciate all the work you've done in splitting this patch series up, but it might be a bit too fine-grained overall.

[ikudrin]

In D87009#2255174, @dblaikie wrote:

In D87009#2253973, @ikudrin wrote:

That would require to update a particular method several times and potentially would leave it in a partially updated state because some code paths are hard to reach.

Could you explain this in more detail?

Let's take, for example, DIEExpr. As far as I can find, with DW_FORM_sec_offset it is used only in the DebugInfo/DWARF unit tests, in dwarfgen::DIE::addStrOffsetsBaseAttribute(). If we took the approach with end-to-end functional testing, this class would not be updated. And the path is not simple to track, by the way.

Other classes in this patch are used to emit compilation units, so moving the changes there would bloat that patch up, making it less focused and harder to track how each individual change is tested because, when looking to the test source, it is not evident which attributes use a particular class to emit their values.

[dblaikie]

In D87009#2255662, @ikudrin wrote:

In D87009#2255174, @dblaikie wrote:

In D87009#2253973, @ikudrin wrote:

That would require to update a particular method several times and potentially would leave it in a partially updated state because some code paths are hard to reach.

Could you explain this in more detail?

Let's take, for example, DIEExpr. As far as I can find, with DW_FORM_sec_offset it is used only in the DebugInfo/DWARF unit tests, in dwarfgen::DIE::addStrOffsetsBaseAttribute(). If we took the approach with end-to-end functional testing, this class would not be updated. And the path is not simple to track, by the way.

API only intended for unit tests would be tested/updated with unit tests.

Other classes in this patch are used to emit compilation units, so moving the changes there would bloat that patch up, making it less focused and harder to track how each individual change is tested because, when looking to the test source, it is not evident which attributes use a particular class to emit their values.

I think what might've been tidier could've been dumping sections alone - guess it's always going to be broken in some way in this intermediate state of some sections with DWARF64 and some without, until the whole patch series is in. But perhaps adding support for one section (while potentially mangling another section/causing it to be partly DWARF32/DWARF64, if it used some common API but had bits of its own implementation - like the loclist/rnglist situation) - adding tests for that section (llvm-dwarfdumping only that section - perhaps that deosn't work for some DWARFv4 sections where it's not self-describing), then adding another, etc. (so, for instance, doing str_offsets first, then doing debug_info that depends on str_offsets, etc)

Also would've probably be good to do more of this incrementally as you went - as much as I appreciate figuring out the end picture and then trying to piece together an ideal patch series, it makes it harder to change things (having to adjust all the subsequent patches) than if we could've discussed them earlier, etc. (& just a lot to review at one time, compared to a small patch here and there)

Anyway - in the interests of practicality, I'll try to get through these mostly on the "what does it look like overall" rather than the nitpicking nuance of what the perfect patch series might look like from my perspective.

[ikudrin]

In D87009#2266648, @dblaikie wrote:

Let's take, for example, DIEExpr. As far as I can find, with DW_FORM_sec_offset it is used only in the DebugInfo/DWARF unit tests, in dwarfgen::DIE::addStrOffsetsBaseAttribute(). If we took the approach with end-to-end functional testing, this class would not be updated. And the path is not simple to track, by the way.

API only intended for unit tests would be tested/updated with unit tests.

The DIEExpr class is not intended to be used only in unit tests; the CodeGen library uses it, but with different DWARF forms. The generator in DebugInfo/DWARF unit tests does not test the class, it just uses it to create some bits of DWARF data. Moreover, these unit tests are for a different library and as such are not the best place to be updated if we want to fix the issue in a class of the CodeGen library. It is much better to test the implementation in the library's own unit tests, so this patch does exactly that. I do not want to leave DIEExpr in the not updated state, because the new code may use it without noticing that the implementation is broken for DWARF64.

Other classes in this patch are used to emit compilation units, so moving the changes there would bloat that patch up, making it less focused and harder to track how each individual change is tested because, when looking to the test source, it is not evident which attributes use a particular class to emit their values.

I think what might've been tidier could've been dumping sections alone - guess it's always going to be broken in some way in this intermediate state of some sections with DWARF64 and some without, until the whole patch series is in. But perhaps adding support for one section (while potentially mangling another section/causing it to be partly DWARF32/DWARF64, if it used some common API but had bits of its own implementation - like the loclist/rnglist situation) - adding tests for that section (llvm-dwarfdumping only that section - perhaps that deosn't work for some DWARFv4 sections where it's not self-describing), then adding another, etc. (so, for instance, doing str_offsets first, then doing debug_info that depends on str_offsets, etc)

I believe that my set of patches does exactly that. After a few preparation steps, which are tested separately with unit tests, there is a series of patches for individual sections, where interferences between patches are minimal. And a patch for each section is tested with llvm-dwarfdump which extracts only the required section(s), ignoring others, which might still be broken.

Also would've probably be good to do more of this incrementally as you went - as much as I appreciate figuring out the end picture and then trying to piece together an ideal patch series, it makes it harder to change things (having to adjust all the subsequent patches) than if we could've discussed them earlier, etc. (& just a lot to review at one time, compared to a small patch here and there)

Well, the current state of the series is definitely not the way I worked on the patches. It is maybe the third iteration, cleaned, polished, and rearranged so that each step is easily observable and thus more convenient for review. I guess if I had been posting the patches in progress, that would be a real mess.

Anyway - in the interests of practicality, I'll try to get through these mostly on the "what does it look like overall" rather than the nitpicking nuance of what the perfect patch series might look like from my perspective.

Thank you for reviewing the patches!

[dblaikie]

In D87009#2267663, @ikudrin wrote:

In D87009#2266648, @dblaikie wrote:

Let's take, for example, DIEExpr. As far as I can find, with DW_FORM_sec_offset it is used only in the DebugInfo/DWARF unit tests, in dwarfgen::DIE::addStrOffsetsBaseAttribute(). If we took the approach with end-to-end functional testing, this class would not be updated. And the path is not simple to track, by the way.

API only intended for unit tests would be tested/updated with unit tests.

The DIEExpr class is not intended to be used only in unit tests; the CodeGen library uses it, but with different DWARF forms. The generator in DebugInfo/DWARF unit tests does not test the class, it just uses it to create some bits of DWARF data. Moreover, these unit tests are for a different library and as such are not the best place to be updated if we want to fix the issue in a class of the CodeGen library. It is much better to test the implementation in the library's own unit tests, so this patch does exactly that. I do not want to leave DIEExpr in the not updated state, because the new code may use it without noticing that the implementation is broken for DWARF64.

Agreed - I don't mean to suggest leaving pieces un-updated (unless they're like a whole distinct thing, like the Apple names accelerator tables - and in that case including assertions to ensure anyone attempting to use DWARF64 there would find out these bits aren't implemented). Just haggling over the particular implementation/testing strategy. But I know it's a complicated set of tradeoffs.

Other classes in this patch are used to emit compilation units, so moving the changes there would bloat that patch up, making it less focused and harder to track how each individual change is tested because, when looking to the test source, it is not evident which attributes use a particular class to emit their values.

I think what might've been tidier could've been dumping sections alone - guess it's always going to be broken in some way in this intermediate state of some sections with DWARF64 and some without, until the whole patch series is in. But perhaps adding support for one section (while potentially mangling another section/causing it to be partly DWARF32/DWARF64, if it used some common API but had bits of its own implementation - like the loclist/rnglist situation) - adding tests for that section (llvm-dwarfdumping only that section - perhaps that deosn't work for some DWARFv4 sections where it's not self-describing), then adding another, etc. (so, for instance, doing str_offsets first, then doing debug_info that depends on str_offsets, etc)

I believe that my set of patches does exactly that. After a few preparation steps, which are tested separately with unit tests, there is a series of patches for individual sections, where interferences between patches are minimal. And a patch for each section is tested with llvm-dwarfdump which extracts only the required section(s), ignoring others, which might still be broken.

*nod* Certainly not far off between your approach and what I'm suggesting. The differences were that I think str_offsets and debug_info ended up being bundled together somewhat? and the preparatory patches I probably would've rolled into some functionality-changing patches instead. No worries.

Also would've probably be good to do more of this incrementally as you went - as much as I appreciate figuring out the end picture and then trying to piece together an ideal patch series, it makes it harder to change things (having to adjust all the subsequent patches) than if we could've discussed them earlier, etc. (& just a lot to review at one time, compared to a small patch here and there)

Well, the current state of the series is definitely not the way I worked on the patches. It is maybe the third iteration, cleaned, polished, and rearranged so that each step is easily observable and thus more convenient for review. I guess if I had been posting the patches in progress, that would be a real mess.

Yeah, I really appreciate the work that went into it, for sure - I know it's not easy to wrangle and rearrange all that code, and getting patches as they go means accepting less polish/more redundancy/changes in direction, etc. No super clear right answer/best direction by any means.

Anyway - in the interests of practicality, I'll try to get through these mostly on the "what does it look like overall" rather than the nitpicking nuance of what the perfect patch series might look like from my perspective.

Thank you for reviewing the patches!

Sure thing - thanks for all the work on them, and in presenting them in this way!

llvm/lib/CodeGen/AsmPrinter/DIE.cpp

 ///
 unsigned DIEExpr::SizeOf(const AsmPrinter *AP, dwarf::Form Form) const {
   switch (Form) {
   case dwarf::DW_FORM_data4:
     return 4;
   case dwarf::DW_FORM_data8:
     return 8;
   case dwarf::DW_FORM_sec_offset:
-    // FIXME: add support for DWARF64
-    return 4;
+    return AP->getDwarfOffsetByteSize();
   default:
     llvm_unreachable("DIE Value form not supported yet");
   }
 }
 
 LLVM_DUMP_METHOD
 void DIEExpr::print(raw_ostream &O) const { O << "Expr: " << *Expr; }
 

 /// SizeOf - Determine size of label value in bytes.
 ///
 unsigned DIELabel::SizeOf(const AsmPrinter *AP, dwarf::Form Form) const {
   switch (Form) {
   case dwarf::DW_FORM_data4:
     return 4;
   case dwarf::DW_FORM_sec_offset:
   case dwarf::DW_FORM_strp:
-    // FIXME: add support for DWARF64
-    return 4;
+    return AP->getDwarfOffsetByteSize();
   case dwarf::DW_FORM_addr:
     return AP->MAI->getCodePointerSize();
   default:
     llvm_unreachable("DIE Value form not supported yet");
   }
 }
 
 LLVM_DUMP_METHOD

 
 /// SizeOf - Determine size of delta value in bytes.
 ///
 unsigned DIEDelta::SizeOf(const AsmPrinter *AP, dwarf::Form Form) const {
   switch (Form) {
   case dwarf::DW_FORM_data4:
     return 4;
   case dwarf::DW_FORM_sec_offset:
-    // FIXME: add support for DWARF64
-    return 4;
+    return AP->getDwarfOffsetByteSize();
   default:
     llvm_unreachable("DIE Value form not supported yet");
   }
 }
 
 LLVM_DUMP_METHOD
 void DIEDelta::print(raw_ostream &O) const {
   O << "Del: " << LabelHi->getName() << "-" << LabelLo->getName();

 // DIELocList Implementation
 //===----------------------------------------------------------------------===//
 
 unsigned DIELocList::SizeOf(const AsmPrinter *AP, dwarf::Form Form) const {
   switch (Form) {
   case dwarf::DW_FORM_loclistx:
     return getULEB128Size(Index);
   case dwarf::DW_FORM_data4:
+    assert(!AP->isDwarf64() &&
+           "DW_FORM_data4 is not suitable to emit a pointer to a location list "
+           "in the 64-bit DWARF format");
     return 4;
+  case dwarf::DW_FORM_data8:
+    assert(AP->isDwarf64() &&
+           "DW_FORM_data8 is not suitable to emit a pointer to a location list "
+           "in the 32-bit DWARF format");
+    return 8;
   case dwarf::DW_FORM_sec_offset:
-    // FIXME: add support for DWARF64
-    return 4;
+    return AP->getDwarfOffsetByteSize();
   default:
     llvm_unreachable("DIE Value form not supported yet");
   }
 }
 
 /// EmitValue - Emit label value.
 ///
 void DIELocList::emitValue(const AsmPrinter *AP, dwarf::Form Form) const {

llvm/unittests/CodeGen/CMakeLists.txt

set(LLVM_LINK_COMPONENTS
   Support
   Target
   )
 
 add_llvm_unittest(CodeGenTests
   AArch64SelectionDAGTest.cpp
   AsmPrinterDwarfTest.cpp
   DIEHashTest.cpp
+  DIETest.cpp
   LowLevelTypeTest.cpp
   LexicalScopesTest.cpp
   MachineInstrBundleIteratorTest.cpp
   MachineInstrTest.cpp
   MachineOperandTest.cpp
   PassManagerTest.cpp
   ScalableVectorMVTsTest.cpp
   TypeTraitsTest.cpp
   TargetOptionsTest.cpp
   TestAsmPrinter.cpp
   )
 
 add_subdirectory(GlobalISel)
 
 target_link_libraries(CodeGenTests PRIVATE LLVMTestingSupport)

llvm/unittests/CodeGen/DIETest.cpp

+//===- llvm/unittest/CodeGen/DIETest.cpp ----------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/CodeGen/DIE.h"
+#include "TestAsmPrinter.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCExpr.h"
+#include "llvm/Testing/Support/Error.h"
+
+using namespace llvm;
+using testing::_;
+using testing::SaveArg;
+
+namespace {
+
+using DIETestParams =
+    std::tuple<unsigned, dwarf::DwarfFormat, dwarf::Form, unsigned>;
+
+class DIEFixtureBase : public testing::TestWithParam<DIETestParams> {
+protected:
+  void SetUp() override {
+    unsigned Version;
+    dwarf::DwarfFormat Format;
+    std::tie(Version, Format, Form, Size) = GetParam();
+    auto ExpectedTestPrinter =
+        TestAsmPrinter::create("x86_64-pc-linux", Version, Format);
+    ASSERT_THAT_EXPECTED(ExpectedTestPrinter, Succeeded());
+    TestPrinter = std::move(ExpectedTestPrinter.get());
+  }
+
+  dwarf::Form Form;
+  unsigned Size;
+  std::unique_ptr<TestAsmPrinter> TestPrinter;
+};
+
+struct DIEExprFixture : public DIEFixtureBase {
+  void SetUp() override {
+    DIEFixtureBase::SetUp();
+    if (!TestPrinter)
+      return;
+
+    Val = MCConstantExpr::create(42, TestPrinter->getCtx());
+  }
+
+  const MCExpr *Val = nullptr;
+};
+
+TEST_P(DIEExprFixture, SizeOf) {
+  if (!TestPrinter)
+    return;
+
+  DIEExpr Tst(Val);
+  EXPECT_EQ(Size, Tst.SizeOf(TestPrinter->getAP(), Form));
+}
+
+TEST_P(DIEExprFixture, EmitValue) {
+  if (!TestPrinter)
+    return;
+
+  DIEExpr Tst(Val);
+  EXPECT_CALL(TestPrinter->getMS(), emitValueImpl(Val, Size, _));
+  Tst.emitValue(TestPrinter->getAP(), Form);
+}
+
+INSTANTIATE_TEST_CASE_P(
+    DIETestParams, DIEExprFixture,
+    testing::Values(
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_data4, 4u},
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_data8, 8u},
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_sec_offset, 4u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_data4, 4u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_data8, 8u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_sec_offset, 8u}), );
+
+struct DIELabelFixture : public DIEFixtureBase {
+  void SetUp() override {
+    DIEFixtureBase::SetUp();
+    if (!TestPrinter)
+      return;
+
+    Val = TestPrinter->getCtx().createTempSymbol();
+  }
+
+  const MCSymbol *Val = nullptr;
+};
+
+TEST_P(DIELabelFixture, SizeOf) {
+  if (!TestPrinter)
+    return;
+
+  DIELabel Tst(Val);
+  EXPECT_EQ(Size, Tst.SizeOf(TestPrinter->getAP(), Form));
+}
+
+TEST_P(DIELabelFixture, EmitValue) {
+  if (!TestPrinter)
+    return;
+
+  DIELabel Tst(Val);
+
+  const MCExpr *Arg0 = nullptr;
+  EXPECT_CALL(TestPrinter->getMS(), emitValueImpl(_, Size, _))
+      .WillOnce(SaveArg<0>(&Arg0));
+  Tst.emitValue(TestPrinter->getAP(), Form);
+
+  const MCSymbolRefExpr *ActualArg0 = dyn_cast_or_null<MCSymbolRefExpr>(Arg0);
+  ASSERT_NE(ActualArg0, nullptr);
+  EXPECT_EQ(&(ActualArg0->getSymbol()), Val);
+}
+
+INSTANTIATE_TEST_CASE_P(
+    DIETestParams, DIELabelFixture,
+    testing::Values(
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_data4, 4u},
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_sec_offset, 4u},
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_strp, 4u},
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_addr, 8u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_data4, 4u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_sec_offset, 8u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_strp, 8u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_addr, 8u}), );
+
+struct DIEDeltaFixture : public DIEFixtureBase {
+  void SetUp() override {
+    DIEFixtureBase::SetUp();
+    if (!TestPrinter)
+      return;
+
+    Hi = TestPrinter->getCtx().createTempSymbol();
+    Lo = TestPrinter->getCtx().createTempSymbol();
+  }
+
+  const MCSymbol *Hi = nullptr;
+  const MCSymbol *Lo = nullptr;
+};
+
+TEST_P(DIEDeltaFixture, SizeOf) {
+  if (!TestPrinter)
+    return;
+
+  DIEDelta Tst(Hi, Lo);
+  EXPECT_EQ(Size, Tst.SizeOf(TestPrinter->getAP(), Form));
+}
+
+TEST_P(DIEDeltaFixture, EmitValue) {
+  if (!TestPrinter)
+    return;
+
+  DIEDelta Tst(Hi, Lo);
+  EXPECT_CALL(TestPrinter->getMS(), emitAbsoluteSymbolDiff(Hi, Lo, Size));
+  Tst.emitValue(TestPrinter->getAP(), Form);
+}
+
+INSTANTIATE_TEST_CASE_P(
+    DIETestParams, DIEDeltaFixture,
+    testing::Values(
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_data4, 4u},
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_sec_offset, 4u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_data4, 4u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_sec_offset, 8u}), );
+
+struct DIELocListFixture : public DIEFixtureBase {
+  void SetUp() override { DIEFixtureBase::SetUp(); }
+};
+
+TEST_P(DIELocListFixture, SizeOf) {
+  if (!TestPrinter)
+    return;
+
+  DIELocList Tst(999);
+  EXPECT_EQ(Size, Tst.SizeOf(TestPrinter->getAP(), Form));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    DIETestParams, DIELocListFixture,
+    testing::Values(
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_loclistx, 2u},
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_data4, 4u},
+        DIETestParams{4, dwarf::DWARF32, dwarf::DW_FORM_sec_offset, 4u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_loclistx, 2u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_data8, 8u},
+        DIETestParams{4, dwarf::DWARF64, dwarf::DW_FORM_sec_offset, 8u}), );
+
+} // end namespace