ObjectFileELF: Fix symbol lookup in bss section

[labath]

Summary

If we have symbol information in a separate file, we need to be very
careful about presenting a unified section view of module to the rest of
the debugger. ObjectFileELF had code to handle that, but it was being
overly cautious -- the section->GetFileSize()!=0 meant that the
unification would fail for sections which do not occupy any space in the
object file (e.g., .bss). In my case, that manifested itself as not
being able to display the values of .bss variables properly as the
section associated with the variable did not have it's load address set
(because it was not present in the unified section list).

I test this by making sure the unified section list and the variables
refer to the same section.

[eugene]

Is it really necessary to check in binary ELF files?
I understand that we don't always have a C compiler capable of producing ELF files, but maybe it's ok to skip this test on those platforms.

[labath]

In D32434#735877, @eugene wrote:

Is it really necessary to check in binary ELF files?
I understand that we don't always have a C compiler capable of producing ELF files, but maybe it's ok to skip this test on those platforms.

That is something very I am very much trying to avoid, as that means people on those platforms cannot validate their changes. (And I'm not sure if I even want to be running a compiler for a test at this level, as that introduces a level of nondeterminism.)

That said, I do agree we should be carefully about adding lots of binary bloat. I have been trying to put this off until we have more of these, but maybe I could try integrating this with obj2yaml -- it does not seem to preserve program headers, but I don't actually need those for this test.

[labath]

We don't support running the test suite on Windows with MSVC. We run it
with clang targeting windows instead. So anyone running the test suite on
Windows is already using clang, and we can just specify a linux triple to
get an ELF binary.

You'll need a linker as well, which we do have in the form of lld, but I don't really want to pull in lld for the sake of this test. And I also used objcopy, although for this particular test we could probably do without it.

However, my main question is: can you still call this a unit test if it calls out to a compiler to produce an object file?

[labath]

Use yaml2obj to avoid checking in a binary.

[labath]

Use yaml2obj to avoid checking in a binary.

[labath]

Ok, wiring yaml2obj up was easier than I expected (for cmake, we'll still need to figure out what to do with the xcode build). Let me know what you make of this.

Also adding @beanz, in case he has any thoughts on the subject.

[eugene]

Neatly done!

[zturner]

If you look at the source code of yaml2obj, all this boils down to a single call to ELFState<T>::writeELF. If you just link against that, we could avoid even shelling out to an external tool, and the YAML could be a string literal defined inside the unit test cpp file. This would also greatly simplify the xcode build.

Thoughts?

[labath]

In D32434#737179, @zturner wrote:

If you look at the source code of yaml2obj, all this boils down to a single call to ELFState<T>::writeELF. If you just link against that, we could avoid even shelling out to an external tool, and the YAML could be a string literal defined inside the unit test cpp file. This would also greatly simplify the xcode build.

Thoughts?

I like that. Let me see how that goes..

[labath]

In D32434#737205, @labath wrote:

In D32434#737179, @zturner wrote:

If you look at the source code of yaml2obj, all this boils down to a single call to ELFState<T>::writeELF. If you just link against that, we could avoid even shelling out to an external tool, and the YAML could be a string literal defined inside the unit test cpp file. This would also greatly simplify the xcode build.

Thoughts?

I like that. Let me see how that goes..

Unfortunately, it's not going to be that easy. All of the yaml->elf conversion code lives in tools/yaml2obj, and is not a part of any library any library that we could pull in. Maybe there's a case to be made for making yaml2obj a library, but I am not sure if this is the time to do that.

[zturner]

Alright, thanks for trying anyway!

lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

if (symbol_section_sp && module_section_list &&
       const ConstString &sect_name = symbol_section_sp->GetName();
       auto section_it = section_name_to_section.find(sect_name.GetCString());
       if (section_it == section_name_to_section.end())
         section_it =
             section_name_to_section
                 .emplace(sect_name.GetCString(),
                          module_section_list->FindSectionByName(sect_name))
                 .first;
-      if (section_it->second && section_it->second->GetFileSize())
+      if (section_it->second)
         symbol_section_sp = section_it->second;
     }
 
     bool is_global = symbol.getBinding() == STB_GLOBAL;
     uint32_t flags = symbol.st_other << 8 | symbol.st_info | additional_flags;
     bool is_mangled = (symbol_name[0] == '_' && symbol_name[1] == 'Z');
 
     llvm::StringRef symbol_ref(symbol_name);

lldb/trunk/unittests/ObjectFile/ELF/CMakeLists.txt

 add_lldb_unittest(ObjectFileELFTests
   TestELFHeader.cpp
+  TestObjectFileELF.cpp
 
   LINK_LIBS
     lldbPluginObjectFileELF
+    lldbPluginSymbolVendorELF
     lldbCore
   )
+
+add_dependencies(ObjectFileELFTests yaml2obj)
+add_definitions(-DYAML2OBJ="$<TARGET_FILE:yaml2obj>")
+
+set(test_inputs
+  sections-resolve-consistently.yaml
+  )
+add_unittest_inputs(ObjectFileELFTests "${test_inputs}")

lldb/trunk/unittests/ObjectFile/ELF/Inputs/sections-resolve-consistently.yaml

+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x0000000000400180
+Sections:
+  - Name:            .note.gnu.build-id
+    Type:            SHT_NOTE
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x0000000000400158
+    AddressAlign:    0x0000000000000004
+    Content:         040000001400000003000000474E55003F3EC29E3FD83E49D18C4D49CD8A730CC13117B6
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x0000000000400180
+    AddressAlign:    0x0000000000000010
+    Content:         554889E58B042500106000890425041060005DC3
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    Address:         0x0000000000601000
+    AddressAlign:    0x0000000000000004
+    Content:         2F000000
+  - Name:            .bss
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    Address:         0x0000000000601004
+    AddressAlign:    0x0000000000000004
+    Size:            0x0000000000000004
+Symbols:
+  Global:
+    - Name:            Y
+      Type:            STT_OBJECT
+      Section:         .data
+      Value:           0x0000000000601000
+      Size:            0x0000000000000004
+    - Name:            _start
+      Type:            STT_FUNC
+      Section:         .text
+      Value:           0x0000000000400180
+      Size:            0x0000000000000014
+    - Name:            X
+      Type:            STT_OBJECT
+      Section:         .bss
+      Value:           0x0000000000601004
+      Size:            0x0000000000000004
+...

lldb/trunk/unittests/ObjectFile/ELF/TestObjectFileELF.cpp

+//===-- TestObjectFileELF.cpp -----------------------------------*- C++ -*-===//
+//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "lldb/Core/Module.h"
+#include "lldb/Core/ModuleSpec.h"
+#include "lldb/Core/Section.h"
+#include "lldb/Host/HostInfo.h"
+#include "llvm/Support/FileUtilities.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Program.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+
+#include "Plugins/SymbolVendor/ELF/SymbolVendorELF.h"
+
+extern const char *TestMainArgv0;
+
+using namespace lldb_private;
+using namespace lldb;
+
+class ObjectFileELFTest : public testing::Test {
+public:
+  void SetUp() override {
+    HostInfo::Initialize();
+    ObjectFileELF::Initialize();
+    SymbolVendorELF::Initialize();
+
+    m_inputs_folder = llvm::sys::path::parent_path(TestMainArgv0);
+    llvm::sys::path::append(m_inputs_folder, "Inputs");
+    llvm::sys::fs::make_absolute(m_inputs_folder);
+  }
+
+  void TearDown() override {
+    SymbolVendorELF::Terminate();
+    ObjectFileELF::Terminate();
+    HostInfo::Terminate();
+  }
+
+protected:
+  llvm::SmallString<128> m_inputs_folder;
+};
+
+#define ASSERT_NO_ERROR(x)                                                     \
+  if (std::error_code ASSERT_NO_ERROR_ec = x) {                                \
+    llvm::SmallString<128> MessageStorage;                                     \
+    llvm::raw_svector_ostream Message(MessageStorage);                         \
+    Message << #x ": did not return errc::success.\n"                          \
+            << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"          \
+            << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";      \
+    GTEST_FATAL_FAILURE_(MessageStorage.c_str());                              \
+  } else {                                                                     \
+  }
+
+TEST_F(ObjectFileELFTest, SectionsResolveConsistently) {
+  llvm::SmallString<128> yaml = m_inputs_folder;
+  llvm::sys::path::append(yaml, "sections-resolve-consistently.yaml");
+  llvm::SmallString<128> obj = m_inputs_folder;
+  ASSERT_NO_ERROR(llvm::sys::fs::createTemporaryFile(
+      "sections-resolve-consistently-%%%%%%", "obj", obj));
+
+  llvm::FileRemover remover(obj);
+  const char *args[] = {YAML2OBJ, yaml.c_str(), nullptr};
+  llvm::StringRef obj_ref = obj;
+  const llvm::StringRef *redirects[] = {nullptr, &obj_ref, nullptr};
+  ASSERT_EQ(0, llvm::sys::ExecuteAndWait(YAML2OBJ, args, nullptr, redirects));
+  uint64_t size;
+  ASSERT_NO_ERROR(llvm::sys::fs::file_size(obj, size));
+  ASSERT_GT(size, 0u);
+
+  ModuleSpec spec{FileSpec(obj, false)};
+  spec.GetSymbolFileSpec().SetFile(obj, false);
+  auto module_sp = std::make_shared<Module>(spec);
+  SectionList *list = module_sp->GetSectionList();
+  ASSERT_NE(nullptr, list);
+
+  auto bss_sp = list->FindSectionByName(ConstString(".bss"));
+  ASSERT_NE(nullptr, bss_sp);
+  auto data_sp = list->FindSectionByName(ConstString(".data"));
+  ASSERT_NE(nullptr, data_sp);
+  auto text_sp = list->FindSectionByName(ConstString(".text"));
+  ASSERT_NE(nullptr, text_sp);
+
+  const Symbol *X = module_sp->FindFirstSymbolWithNameAndType(ConstString("X"),
+                                                              eSymbolTypeAny);
+  ASSERT_NE(nullptr, X);
+  EXPECT_EQ(bss_sp, X->GetAddress().GetSection());
+
+  const Symbol *Y = module_sp->FindFirstSymbolWithNameAndType(ConstString("Y"),
+                                                              eSymbolTypeAny);
+  ASSERT_NE(nullptr, Y);
+  EXPECT_EQ(data_sp, Y->GetAddress().GetSection());
+
+  const Symbol *start = module_sp->FindFirstSymbolWithNameAndType(
+      ConstString("_start"), eSymbolTypeAny);
+  ASSERT_NE(nullptr, start);
+  EXPECT_EQ(text_sp, start->GetAddress().GetSection());
+}