[lldb][PECOFF] Exclude alignment padding when reading section data

[hjyamauchi]

Summary

There can be zero padding bytes at a section end for file alignment in PECOFF. Exclude those padding bytes when reading the section data.

[compnerd]

LGTM outside of the PE header check.

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

1035

Can we use m_binary->getPEHeader() || m_binary->getPE32Header() here instead? We are technically ensuring that this is a linked (PE) binary rather than an object file. While a DOS header is present, it is not an absolute requirement (theoretically, it is practically never going to change).

[MrTrillian]

Looking good!

[hjyamauchi]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

1035

Done.

[Michael137]

Hi, this is failing on swift-ci (runs on x86 bots) with the following error:

/Users/ec2-user/jenkins/workspace/oss-lldb-incremental-macos-cmake/build/Ninja-ReleaseAssert+stdlib-Release/lldb-macosx-x86_64/unittests/ObjectFile/PECOFF/./ObjectFilePECOFFTests --gtest_filter=SectionSizeTest.NoAlignmentPadding
--
YAML:12:5: error: unknown key 'SizeOfRawData'
    SizeOfRawData:   512
    ^~~~~~~~~~~~~
/Users/ec2-user/jenkins/workspace/oss-lldb-incremental-macos-cmake/llvm-project/lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp:49: Failure
Value of: llvm::detail::TakeExpected(ExpectedFile)
Expected: succeeded
  Actual: failed  (convertYAML() failed)

/Users/ec2-user/jenkins/workspace/oss-lldb-incremental-macos-cmake/llvm-project/lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp:49
Value of: llvm::detail::TakeExpected(ExpectedFile)
Expected: succeeded
  Actual: failed  (convertYAML() failed)

https://ci.swift.org/view/LLDB/job/oss-lldb-incremental-macos-cmake/

Could you take a look please?

[hjyamauchi]

In D157059#4572809, @Michael137 wrote:

Hi, this is failing on swift-ci (runs on x86 bots) with the following error:

/Users/ec2-user/jenkins/workspace/oss-lldb-incremental-macos-cmake/build/Ninja-ReleaseAssert+stdlib-Release/lldb-macosx-x86_64/unittests/ObjectFile/PECOFF/./ObjectFilePECOFFTests --gtest_filter=SectionSizeTest.NoAlignmentPadding
--
YAML:12:5: error: unknown key 'SizeOfRawData'
    SizeOfRawData:   512
    ^~~~~~~~~~~~~
/Users/ec2-user/jenkins/workspace/oss-lldb-incremental-macos-cmake/llvm-project/lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp:49: Failure
Value of: llvm::detail::TakeExpected(ExpectedFile)
Expected: succeeded
  Actual: failed  (convertYAML() failed)

/Users/ec2-user/jenkins/workspace/oss-lldb-incremental-macos-cmake/llvm-project/lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp:49
Value of: llvm::detail::TakeExpected(ExpectedFile)
Expected: succeeded
  Actual: failed  (convertYAML() failed)

https://ci.swift.org/view/LLDB/job/oss-lldb-incremental-macos-cmake/

Could you take a look please?

Fix: https://github.com/apple/llvm-project/pull/7218

lldb/include/lldb/Symbol/ObjectFile.h

virtual size_t ReadSectionData(Section *section,
                                  size_t dst_len);
 
   // This function will transparently decompress section data if the section if
   // compressed. Note that for compressed section the resulting data size may
   // be larger than what Section::GetFileSize reports.
   virtual size_t ReadSectionData(Section *section,
                                  DataExtractor &section_data);
 
+  // Returns the section data size. This is special-cased for PECOFF
+  // due to file alignment.
+  virtual size_t GetSectionDataSize(Section *section) {
+    return section->GetFileSize();
+  }
+
   /// Returns true if the object file exists only in memory.
   bool IsInMemory() const { return m_memory_addr != LLDB_INVALID_ADDRESS; }
 
   // Strip linker annotations (such as @@VERSION) from symbol names.
   virtual llvm::StringRef
   StripLinkerSymbolAnnotations(llvm::StringRef symbol_name) const {
     return symbol_name;
   }

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h

static void DumpOptCOFFHeader(lldb_private::Stream *s,
                                 const coff_opt_header_t &header);
   void DumpSectionHeaders(lldb_private::Stream *s);
   void DumpSectionHeader(lldb_private::Stream *s, const section_header_t &sh);
   void DumpDependentModules(lldb_private::Stream *s);
 
   llvm::StringRef GetSectionName(const section_header_t &sect);
   static lldb::SectionType GetSectionType(llvm::StringRef sect_name,
                                           const section_header_t &sect);
+  size_t GetSectionDataSize(lldb_private::Section *section) override;
 
   typedef std::vector<section_header_t> SectionHeaderColl;
   typedef SectionHeaderColl::iterator SectionHeaderCollIter;
   typedef SectionHeaderColl::const_iterator SectionHeaderCollConstIter;
 
 private:
   bool CreateBinary();
   typedef std::vector<std::pair<uint32_t, uint32_t>> rva_symbol_list_t;

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

if (sect.flags & llvm::COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA) {
     if (sect.size == 0)
       return eSectionTypeZeroFill;
     else
       return eSectionTypeData;
   }
   return eSectionTypeOther;
 }
 
+size_t ObjectFilePECOFF::GetSectionDataSize(Section *section) {
+  // For executables, SizeOfRawData (getFileSize()) is aligned by
+  // FileAlignment and the actual section size is in VirtualSize
+  // (getByteSize()). See the comment on
+  // llvm::object::COFFObjectFile::getSectionSize().
+  if (m_binary->getPE32Header() || m_binary->getPE32PlusHeader())
+    return std::min(section->GetByteSize(), section->GetFileSize());
+  return section->GetFileSize();
+}
+
 void ObjectFilePECOFF::CreateSections(SectionList &unified_section_list) {
   if (m_sections_up)
     return;
   m_sections_up = std::make_unique<SectionList>();
   ModuleSP module_sp(GetModule());
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
 

lldb/source/Symbol/ObjectFile.cpp

if (process_sp) {
           return section_data.GetByteSize();
         }
       }
     }
   }
 
   // The object file now contains a full mmap'ed copy of the object file
   // data, so just use this
-  return GetData(section->GetFileOffset(), section->GetFileSize(),
-                  section_data);
+  return GetData(section->GetFileOffset(), GetSectionDataSize(section),
 }
 
 bool ObjectFile::SplitArchivePathWithObject(llvm::StringRef path_with_object,
                                             FileSpec &archive_file,
                                             ConstString &archive_object,
                                             bool must_exist) {
   size_t len = path_with_object.size();
   if (len < 2 || path_with_object.back() != ')')

lldb/unittests/ObjectFile/PECOFF/CMakeLists.txt

 add_lldb_unittest(ObjectFilePECOFFTests
   TestPECallFrameInfo.cpp
+  TestSectionSize.cpp
 
   LINK_LIBS
     lldbUtilityHelpers
     lldbPluginObjectFilePECOFF
     LLVMTestingSupport
   )

lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp

+//===-- TestSectionFileSize.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 "gtest/gtest.h"
+
+#include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
+#include "Plugins/Process/Utility/lldb-x86-register-enums.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "TestingSupport/TestUtilities.h"
+
+#include "lldb/Core/Module.h"
+#include "lldb/Symbol/CallFrameInfo.h"
+#include "lldb/Symbol/UnwindPlan.h"
+#include "llvm/Testing/Support/Error.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+class SectionSizeTest : public testing::Test {
+  SubsystemRAII<FileSystem, ObjectFilePECOFF> subsystems;
+};
+
+TEST_F(SectionSizeTest, NoAlignmentPadding) {
+  llvm::Expected<TestFile> ExpectedFile = TestFile::fromYaml(
+      R"(
+--- !COFF
+OptionalHeader:
+  SectionAlignment: 4096
+  FileAlignment:   512
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE ]
+sections:
+  - Name:            swiftast
+    VirtualSize:     496
+    SizeOfRawData:   512
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+    SectionData:     11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111110000
+
+symbols:         []
+...
+)");
+  ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded());
+
+  ModuleSP module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec());
+  ObjectFile *object_file = module_sp->GetObjectFile();
+  ASSERT_NE(object_file, nullptr);
+
+  SectionList *section_list = object_file->GetSectionList();
+  ASSERT_NE(section_list, nullptr);
+
+  SectionSP swiftast_section;
+  size_t section_count = section_list->GetNumSections(0);
+  for (size_t i = 0; i < section_count; ++i) {
+    SectionSP section_sp = section_list->GetSectionAtIndex(i);
+    if (section_sp->GetName() == "swiftast") {
+      swiftast_section = section_sp;
+      break;
+    }
+  }
+  ASSERT_NE(swiftast_section.get(), nullptr);
+
+  DataExtractor section_data;
+  ASSERT_NE(object_file->ReadSectionData(swiftast_section.get(),
+                                         section_data),
+            0);
+
+  // Check that the section data size is equal to VirtualSize (496)
+  // without the zero padding, instead of SizeOfRawData (512).
+  EXPECT_EQ(section_data.GetByteSize(), 496);
+}
+

llvm/lib/ObjectYAML/COFFYAML.cpp

void MappingTraits<COFFYAML::Section>::mapping(IO &IO, COFFYAML::Section &Sec) {
 
   IO.mapOptional("StructuredData", Sec.StructuredData);
 
   if (!Sec.StructuredData.empty() && Sec.SectionData.binary_size()) {
     IO.setError("StructuredData and SectionData can't be used together");
     return;
   }
 
-  // Uninitialized sections, such as .bss, typically have no data, but the size
-  // is carried in SizeOfRawData, even though PointerToRawData is zero.
-  if (Sec.SectionData.binary_size() == 0 && Sec.StructuredData.empty() &&
-      NC->Characteristics & COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA)
-    IO.mapOptional("SizeOfRawData", Sec.Header.SizeOfRawData);
+  IO.mapOptional("SizeOfRawData", Sec.Header.SizeOfRawData, 0U);
+
+  if (!Sec.StructuredData.empty() && Sec.Header.SizeOfRawData) {
+    IO.setError("StructuredData and SizeOfRawData can't be used together");
+    return;
+  }
 
   IO.mapOptional("Relocations", Sec.Relocations);
 }
 
 void MappingTraits<COFFYAML::Object>::mapping(IO &IO, COFFYAML::Object &Obj) {
   IO.mapTag("!COFF", true);
   IO.mapOptional("OptionalHeader", Obj.OptionalHeader);
   IO.mapRequired("header", Obj.Header);
   IO.mapRequired("sections", Obj.Sections);
   IO.mapRequired("symbols", Obj.Symbols);
 }
 
 } // end namespace yaml
 
 } // end namespace llvm

llvm/test/tools/yaml2obj/COFF/invalid-raw-data.yaml

 # RUN: not yaml2obj %s -o %t 2>&1 | FileCheck %s
-# CHECK: YAML:18:5: error: unknown key 'SizeOfRawData'
+# CHECK: YAML:14:5: error: StructuredData and SizeOfRawData can't be used together
 
 --- !COFF
 OptionalHeader:
   ImageBase:        0x180000000
   SectionAlignment: 4096
   FileAlignment:    512
   DLLCharacteristics: [ ]
 header:

llvm/test/tools/yaml2obj/COFF/xrelocs.yaml

 # CHECK-OBJ-NEXT:   }
 # CHECK-OBJ-NEXT: ]
 
 # CHECK-YAML:      sections:
 # CHECK-YAML-NEXT:   - Name:            .data
 # CHECK-YAML-NEXT:     Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_LNK_NRELOC_OVFL, IMAGE_SCN_MEM_READ ]
 # CHECK-YAML-NEXT:     Alignment:       16
 # CHECK-YAML-NEXT:     SectionData:     '00000000000000000000000000000000'
+# CHECK-YAML-NEXT:     SizeOfRawData:   16
 # CHECK-YAML-NEXT:     Relocations:
 # CHECK-YAML-NEXT:       - VirtualAddress:  0
 # CHECK-YAML-NEXT:         SymbolName:      foo
 # CHECK-YAML-NEXT:         Type:            IMAGE_REL_AMD64_ADDR64
 # CHECK-YAML-NEXT:       - VirtualAddress:  8
 # CHECK-YAML-NEXT:         SymbolName:      bar
 # CHECK-YAML-NEXT:         Type:            IMAGE_REL_AMD64_ADDR64
 # CHECK-YAML-NEXT: symbols: