[lldb/DWARF] Fix implementation of DW_OP_convert

[labath]

Summary

According to the standard the DW_OP_convert operand is an offset
relative to the current compilation unit, not to the start of the
debug_info section.

To test this, extend YAMLModuleTester with the ability to hold multiple
DWARF units.

[labath]

Judging by http://lists.llvm.org/pipermail/llvm-dev/2020-October/145990.html, dsymutil has a similar bug...

[labath]

Add (test) changes I accidentally left out.

[dblaikie]

In D90598#2368020, @labath wrote:

Judging by http://lists.llvm.org/pipermail/llvm-dev/2020-October/145990.html, dsymutil has a similar bug...

That's my understanding. It means I'm not sure (though I'm not the expert here - certainly leave final decisions up to @aprantl and @JDevlieghere ) how this will be fixed, since lldb will presumably still need to be able to parse/understand old dsyms using the incorrect absolute offsets. (we had a similar problem once with DW_OP_bit_piece, where it was implemented incorrectly in both LLVM and lldb and I'm not sure how the backwards compatibility story was addressed there).

[JDevlieghere]

In D90598#2368826, @dblaikie wrote:

In D90598#2368020, @labath wrote:

Judging by http://lists.llvm.org/pipermail/llvm-dev/2020-October/145990.html, dsymutil has a similar bug...

That's my understanding. It means I'm not sure (though I'm not the expert here - certainly leave final decisions up to @aprantl and @JDevlieghere ) how this will be fixed, since lldb will presumably still need to be able to parse/understand old dsyms using the incorrect absolute offsets. (we had a similar problem once with DW_OP_bit_piece, where it was implemented incorrectly in both LLVM and lldb and I'm not sure how the backwards compatibility story was addressed there).

I think @aprantl has proposed this in the past, but we should make dsymutil convert DW_OP_convert to DW_OP_LLVM_convert in its output. I also don't have a good plan for backward compatibility. If there was a way we could "know" that the DWARF came from a dSYM we could continue to support the old behavior knowing that it has always been wrong, but I'm not aware of anything like that in LLDB and I'm also not sure that's something I'd like to have in the first place...

[labath]

In D90598#2369590, @JDevlieghere wrote:

In D90598#2368826, @dblaikie wrote:

That's my understanding. It means I'm not sure (though I'm not the expert here - certainly leave final decisions up to @aprantl and @JDevlieghere ) how this will be fixed, since lldb will presumably still need to be able to parse/understand old dsyms using the incorrect absolute offsets. (we had a similar problem once with DW_OP_bit_piece, where it was implemented incorrectly in both LLVM and lldb and I'm not sure how the backwards compatibility story was addressed there).

I think @aprantl has proposed this in the past, but we should make dsymutil convert DW_OP_convert to DW_OP_LLVM_convert in its output.

How much space are we talking about saving here? A single DW_TAG_base_type DIE is like 4--8 bytes long (depending of whether it contains the name field, and how its encoded, etc.). If every compile unit needed a dozen of these entries, that's still like under 100 bytes per CU. Is that too much? Could we just put these entries into every CU that needs them? I'd expect this to be negligible compared to the rest of DWARF...

If the space savings are important enough, then staking out a new dwarf opcode seems like the best solution. However, since DWARF Linker is also coming to other platforms and those platforms may have consumers which may not understand llvm extensions, I think we may have to implement both solutions anyway. Also, to support non-macho use cases, I think the operand to this extension should not be uleb-encoded (so that it can be relocated by a linker), which can make it larger than usual.

Or.. here's a crazy idea. Do all compile units produced by dsymutil share the same abbreviation table? (If not, why not?) We could encode the DW_TAG_base_types "into" the abbreviation table via DW_FORM_implicit_const. Then the DW_TAG_base_types would be just sizeof(uleb) ≈ 2 (plus maybe the size of a string form, if they have a name).

I also don't have a good plan for backward compatibility. If there was a way we could "know" that the DWARF came from a dSYM we could continue to support the old behavior knowing that it has always been wrong, but I'm not aware of anything like that in LLDB and I'm also not sure that's something I'd like to have in the first place...

Detecting dSYM files should not be that hard. triple.isMachO() && !symbol_file_dwarf->GetDebugMapSymfile() should be a pretty good approximation. It won't handle case the case of opening .o files directly, but that's only useful for tests I believe, and we could throw in an additional condition for that if needed. If we want to have backward compatibility, I think we're going to have to have something like that at least for a transitional period. The question is how to ensure that this code can be removed eventually. One possibility would be for dsymutil to signal that it has this fixed via some cu-level attribute (DW_AT_LLVM_convert_is_fixed) -- after a certain time, we can remove the fallback, and assume the correct behavior everywhere...

lldb/source/Expression/DWARFExpression.cpp

 //===-- DWARFExpression.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 "lldb/Expression/DWARFExpression.h"
 
 #include <inttypes.h>
 
 #include <vector>
 
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/dwarf.h"
case DW_OP_convert: {
         if (!bit_size) {
           if (error_ptr)
             error_ptr->SetErrorString("unspecified architecture");
           return false;
         }
       } else {
         // Retrieve the type DIE that the value is being converted to.
         // FIXME: the constness has annoying ripple effects.
-        DWARFDIE die = const_cast<DWARFUnit *>(dwarf_cu)->GetDIE(die_offset);
+        DWARFDIE die = const_cast<DWARFUnit *>(dwarf_cu)->GetDIE(
+            dwarf_cu->GetOffset() + die_offset);
         if (!die) {
           if (error_ptr)
             error_ptr->SetErrorString("Cannot resolve DW_OP_convert type DIE");
           return false;
         }
         uint64_t encoding =
             die.GetAttributeValueAsUnsigned(DW_AT_encoding, DW_ATE_hi_user);
         bit_size = die.GetAttributeValueAsUnsigned(DW_AT_byte_size, 0) * 8;

lldb/unittests/Expression/DWARFExpressionTest.cpp

 //===-- DWARFExpressionTest.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 "lldb/Expression/DWARFExpression.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "TestingSupport/Symbol/YAMLModuleTester.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/dwarf.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/StreamString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Testing/Support/Error.h"
static llvm::Expected<Scalar> Evaluate(llvm::ArrayRef<uint8_t> expr,
   default:
     return status.ToError();
   }
 }
 
 class DWARFExpressionTester : public YAMLModuleTester {
 public:
   using YAMLModuleTester::YAMLModuleTester;
-  llvm::Expected<Scalar> Eval(llvm::ArrayRef<uint8_t> expr) {
-    return ::Evaluate(expr, m_module_sp, m_dwarf_unit);
+  llvm::Expected<Scalar> Eval(llvm::ArrayRef<uint8_t> expr, DWARFUnit *unit) {
+    return ::Evaluate(expr, m_module_sp, unit);
   }
 };
 
 /// Unfortunately Scalar's operator==() is really picky.
 static Scalar GetScalar(unsigned bits, uint64_t value, bool sign) {
   Scalar scalar(value);
   scalar.TruncOrExtendTo(bits, sign);
   return scalar;

 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_386
 DWARF:
   debug_abbrev:
-    - Table:
+    - ID:    0
+      Table:
         - Code:            0x00000001
           Tag:             DW_TAG_compile_unit
           Children:        DW_CHILDREN_yes
           Attributes:
             - Attribute:       DW_AT_language
               Form:            DW_FORM_data2
         - Code:            0x00000002
           Tag:             DW_TAG_base_type
           Children:        DW_CHILDREN_no
           Attributes:
             - Attribute:       DW_AT_encoding
               Form:            DW_FORM_data1
             - Attribute:       DW_AT_byte_size
               Form:            DW_FORM_data1
   debug_info:
     - Version:         4
       AddrSize:        8
+      AbbrevTableID:   0
+      Entries:
+        - AbbrCode:        0x00000001
+          Values:
+            - Value:           0x000000000000000C
+        - AbbrCode:        0x00000002
+          Values:
+            - Value:           0x0000000000000007 # DW_ATE_unsigned
+            - Value:           0x0000000000000004
+        - AbbrCode:        0x00000000
+    - Version:         4
+      AddrSize:        8
+      AbbrevTableID:   0
       Entries:
         - AbbrCode:        0x00000001
           Values:
             - Value:           0x000000000000000C
-        # 0x0000000e:
         - AbbrCode:        0x00000002
           Values:
             - Value:           0x0000000000000007 # DW_ATE_unsigned
             - Value:           0x0000000000000004
-        # 0x00000011:
         - AbbrCode:        0x00000002
           Values:
             - Value:           0x0000000000000007 # DW_ATE_unsigned
             - Value:           0x0000000000000008
-        # 0x00000014:
         - AbbrCode:        0x00000002
           Values:
             - Value:           0x0000000000000005 # DW_ATE_signed
             - Value:           0x0000000000000008
-        # 0x00000017:
         - AbbrCode:        0x00000002
           Values:
             - Value:           0x0000000000000008 # DW_ATE_unsigned_char
             - Value:           0x0000000000000001
-        # 0x0000001a:
         - AbbrCode:        0x00000002
           Values:
             - Value:           0x0000000000000006 # DW_ATE_signed_char
             - Value:           0x0000000000000001
-        # 0x0000001d:
         - AbbrCode:        0x00000002
           Values:
             - Value:           0x000000000000000b # DW_ATE_numeric_string
             - Value:           0x0000000000000001
         - AbbrCode:        0x00000000
 )";
-  uint8_t offs_uint32_t = 0x0000000e;
-  uint8_t offs_uint64_t = 0x00000011;
-  uint8_t offs_sint64_t = 0x00000014;
-  uint8_t offs_uchar = 0x00000017;
-  uint8_t offs_schar = 0x0000001a;
 
   DWARFExpressionTester t(yamldata);
-  ASSERT_TRUE((bool)t.GetDwarfUnit());
+  ASSERT_EQ(2u, t.GetNumUnits());
+  DWARFUnit *unit = t.GetUnitAtIndex(1);
+  DWARFDIE uint32_die = unit->DIE().GetFirstChild();
+  DWARFDIE uint64_die = uint32_die.GetSibling();
+  DWARFDIE sint64_die = uint64_die.GetSibling();
+  DWARFDIE uchar_die = sint64_die.GetSibling();
+  DWARFDIE schar_die = uchar_die.GetSibling();
+  DWARFDIE numeric_string_die = schar_die.GetSibling();
+  auto offset = [](DWARFDIE die) -> uint8_t {
+    dw_offset_t result = die.GetOffset() - die.GetCU()->GetOffset();
+    assert(result <= 0x7f);
+    return result;
+  };
 
   // Constant is given as little-endian.
   bool is_signed = true;
   bool not_signed = false;
 
   //
   // Positive tests.
   //
 
   // Truncate to default unspecified (pointer-sized) type.
   EXPECT_THAT_EXPECTED(
       t.Eval({DW_OP_const8u, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, //
-              DW_OP_convert, 0x00}),
+              DW_OP_convert, 0x00},
+             unit),
       llvm::HasValue(GetScalar(32, 0x44332211, not_signed)));
   // Truncate to 32 bits.
-  EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const8u, //
-                               0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88,//
-                               DW_OP_convert, offs_uint32_t}),
-                       llvm::HasValue(GetScalar(32, 0x44332211, not_signed)));
+  EXPECT_THAT_EXPECTED(
+      t.Eval({DW_OP_const8u, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, //
+              DW_OP_convert, offset(uint32_die)},
+             unit),
 
   // Leave as is.
   EXPECT_THAT_EXPECTED(
       t.Eval({DW_OP_const8u, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, //
-              DW_OP_convert, offs_uint64_t}),
+              DW_OP_convert, offset(uint64_die)},
+             unit),
       llvm::HasValue(GetScalar(64, 0x8877665544332211, not_signed)));
 
   // Sign-extend to 64 bits.
   EXPECT_THAT_EXPECTED(
       t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
-              DW_OP_convert, offs_sint64_t}),
+              DW_OP_convert, offset(sint64_die)},
+             unit),
       llvm::HasValue(GetScalar(64, 0xffffffffffeeddcc, is_signed)));
 
   // Truncate to 8 bits.
-  EXPECT_THAT_EXPECTED(
-      t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert, offs_uchar}),
+  EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', //
+                               DW_OP_convert, offset(uchar_die)},
+                              unit),
                        llvm::HasValue(GetScalar(8, 'A', not_signed)));
 
   // Also truncate to 8 bits.
-  EXPECT_THAT_EXPECTED(
-      t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert, offs_schar}),
+  EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', //
+                               DW_OP_convert, offset(schar_die)},
+                              unit),
                        llvm::HasValue(GetScalar(8, 'A', is_signed)));
 
   //
   // Errors.
   //
 
   // No Module.
-  EXPECT_THAT_ERROR(Evaluate({DW_OP_const1s, 'X', DW_OP_convert, 0x00}, nullptr,
-                             t.GetDwarfUnit())
-                        .takeError(),
-                    llvm::Failed());
+  EXPECT_THAT_ERROR(
+      Evaluate({DW_OP_const1s, 'X', DW_OP_convert, 0x00}, nullptr, unit)
 
   // No DIE.
   EXPECT_THAT_ERROR(
-      t.Eval({DW_OP_const1s, 'X', DW_OP_convert, 0x01}).takeError(),
+      t.Eval({DW_OP_const1s, 'X', DW_OP_convert, 0x01}, unit).takeError(),
       llvm::Failed());
 
   // Unsupported.
   EXPECT_THAT_ERROR(
-      t.Eval({DW_OP_const1s, 'X', DW_OP_convert, 0x1d}).takeError(),
+      t.Eval({DW_OP_const1s, 'X', DW_OP_convert, offset(numeric_string_die)},
+             unit)
+          .takeError(),
       llvm::Failed());
 }
 
 TEST(DWARFExpression, DW_OP_stack_value) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_stack_value}), llvm::Failed());
 }
 
 TEST(DWARFExpression, DW_OP_piece) {

lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp

 //===-- DWARFASTParserClangTests.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 "Plugins/SymbolFile/DWARF/DWARFASTParserClang.h"
 #include "Plugins/SymbolFile/DWARF/DWARFCompileUnit.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDIE.h"
 #include "TestingSupport/Symbol/YAMLModuleTester.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace lldb;
 using namespace lldb_private;
- Version:         4
         - AbbrCode:        0x00000002
           Values:
             - Value:           0x0000000000000008 # DW_ATE_unsigned_char
             - Value:           0x0000000000000001
         - AbbrCode:        0x00000000
 )";
 
   YAMLModuleTester t(yamldata);
-  ASSERT_TRUE((bool)t.GetDwarfUnit());
+  ASSERT_EQ(1u, t.GetNumUnits());
 
   TypeSystemClang ast_ctx("dummy ASTContext", HostInfoBase::GetTargetTriple());
   DWARFASTParserClangStub ast_parser(ast_ctx);
 
-  DWARFUnit *unit = t.GetDwarfUnit();
+  DWARFUnit *unit = t.GetUnitAtIndex(0);
   const DWARFDebugInfoEntry *die_first = unit->DIE().GetDIE();
   const DWARFDebugInfoEntry *die_child0 = die_first->GetFirstChild();
   const DWARFDebugInfoEntry *die_child1 = die_child0->GetSibling();
   const DWARFDebugInfoEntry *die_child2 = die_child1->GetSibling();
   const DWARFDebugInfoEntry *die_child3 = die_child2->GetSibling();
   std::vector<DWARFDIE> dies = {
       DWARFDIE(unit, die_child0), DWARFDIE(unit, die_child1),
       DWARFDIE(unit, die_child2), DWARFDIE(unit, die_child3)};

lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp

 //===-- XcodeSDKModuleTests.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 "Plugins/Platform/MacOSX/PlatformMacOSX.h"
 #include "Plugins/SymbolFile/DWARF/DWARFCompileUnit.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDIE.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "TestingSupport/Symbol/YAMLModuleTester.h"
 #include "lldb/Core/PluginManager.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
- Version:         2
           Values:
             - Value:           0x000000000000000C
             - Value:           0x0000000000000000
         - AbbrCode:        0x00000000
 ...
 )";
 
   YAMLModuleTester t(yamldata);
-  DWARFUnit *dwarf_unit = t.GetDwarfUnit();
+  ASSERT_EQ(1u, t.GetNumUnits());
+  DWARFUnit *dwarf_unit = t.GetUnitAtIndex(0);
   auto *dwarf_cu = llvm::cast<DWARFCompileUnit>(dwarf_unit);
   ASSERT_TRUE(static_cast<bool>(dwarf_cu));
   SymbolFileDWARF &sym_file = dwarf_cu->GetSymbolFileDWARF();
   CompUnitSP comp_unit = sym_file.GetCompileUnitAtIndex(0);
   ASSERT_TRUE(static_cast<bool>(comp_unit.get()));
   ModuleSP module = t.GetModule();
   ASSERT_EQ(module->GetSourceMappingList().GetSize(), 0u);
   XcodeSDK sdk = sym_file.ParseXcodeSDK(*comp_unit);
   ASSERT_EQ(sdk.GetType(), XcodeSDK::Type::MacOSX);
   ASSERT_EQ(module->GetSourceMappingList().GetSize(), 1u);
 }
 #endif

lldb/unittests/TestingSupport/Symbol/YAMLModuleTester.h

 //===- YAMLModuleTester.h ---------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UNITTESTS_TESTINGSUPPORT_SYMBOL_YAMLMODULETESTER_H
 #define LLDB_UNITTESTS_TESTINGSUPPORT_SYMBOL_YAMLMODULETESTER_H
 
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
 #include "Plugins/SymbolFile/DWARF/DWARFUnit.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARF.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "TestingSupport/SubsystemRAII.h"
 #include "TestingSupport/TestUtilities.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Host/HostInfo.h"
 
 namespace lldb_private {
 
 /// Helper class that can construct a module from YAML and evaluate
 /// DWARF expressions on it.
 class YAMLModuleTester {
 protected:
   SubsystemRAII<FileSystem, HostInfo, TypeSystemClang, ObjectFileELF,
                 SymbolFileDWARF>
       subsystems;
   llvm::Optional<TestFile> m_file;
   lldb::ModuleSP m_module_sp;
-  DWARFUnit *m_dwarf_unit;
 
 public:
   /// Parse the debug info sections from the YAML description.
   YAMLModuleTester(llvm::StringRef yaml_data);
-  DWARFUnit *GetDwarfUnit() const { return m_dwarf_unit; }
   lldb::ModuleSP GetModule() const { return m_module_sp; }
+
+  size_t GetNumUnits();
+  DWARFUnit *GetUnitAtIndex(size_t idx);
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_UNITTESTS_TESTINGSUPPORT_SYMBOL_YAMLMODULETESTER_H

lldb/unittests/TestingSupport/Symbol/YAMLModuleTester.cpp

 //===-- YAMLModuleTester.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 "TestingSupport/Symbol/YAMLModuleTester.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugInfo.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/Core/Section.h"
 #include "llvm/ObjectYAML/DWARFEmitter.h"
 
 using namespace lldb_private;
 
 YAMLModuleTester::YAMLModuleTester(llvm::StringRef yaml_data) {
   llvm::Expected<TestFile> File = TestFile::fromYaml(yaml_data);
   EXPECT_THAT_EXPECTED(File, llvm::Succeeded());
   m_file = std::move(*File);
 
   m_module_sp = std::make_shared<Module>(m_file->moduleSpec());
-  auto &symfile = *llvm::cast<SymbolFileDWARF>(m_module_sp->GetSymbolFile());
+}
+
+size_t YAMLModuleTester::GetNumUnits() {
+  return llvm::cast<SymbolFileDWARF>(m_module_sp->GetSymbolFile())
+      ->DebugInfo()
+      .GetNumUnits();
+}
 
-  m_dwarf_unit = symfile.DebugInfo().GetUnitAtIndex(0);
+DWARFUnit *YAMLModuleTester::GetUnitAtIndex(size_t idx) {
+  return llvm::cast<SymbolFileDWARF>(m_module_sp->GetSymbolFile())
+      ->DebugInfo()
+      .GetUnitAtIndex(idx);
 }