[vm/aot] Move ELF section manipulation out of CreateAppAOTSnapshotAsElf.

Instead, we just always add a BSS section and we add the ROData
sections for ELF snapshots in BlobImageWriter::WriteText() (similar
to how AssemblyImageWriter::WriteText() handles it for assembly).

Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-mac-release-simarm64-try,vm-kernel-precomp-win-release-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-release-simarm_x64-try
Change-Id: I87ce64037821ce10f2344964e7f379376c4538df
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/150937
Reviewed-by: Martin Kustermann <kustermann@google.com>

runtime/vm/dart_api_impl.cc

@@ -6443,9 +6443,10 @@ Dart_CreateAppAOTSnapshotAsAssembly(Dart_StreamingWriteCallback callback,
   StreamingWriteStream debug_stream(generate_debug ? kInitialDebugSize : 0,
                                     callback, debug_callback_data);
 
-  auto const elf = generate_debug ? new (Z)
-                                        Elf(Z, &debug_stream, new (Z) Dwarf(Z))
-                                  : nullptr;
+  auto const elf = generate_debug
+                       ? new (Z) Elf(Z, &debug_stream, Elf::Type::DebugInfo,
+                                     new (Z) Dwarf(Z))
+                       : nullptr;
 
   AssemblyImageWriter image_writer(T, callback, callback_data, strip, elf);
   uint8_t* vm_snapshot_data_buffer = NULL;
@@ -6517,49 +6518,23 @@ Dart_CreateAppAOTSnapshotAsElf(Dart_StreamingWriteCallback callback,
                                     callback, debug_callback_data);
 
   auto const dwarf = strip ? nullptr : new (Z) Dwarf(Z);
-  auto const elf = new (Z) Elf(Z, &elf_stream, dwarf);
-  // Re-use the same DWARF object if unstripped.
+  auto const elf = new (Z) Elf(Z, &elf_stream, Elf::Type::Snapshot, dwarf);
+  // Re-use the same DWARF object if the snapshot is unstripped.
   auto const debug_elf =
-      generate_debug
-          ? new (Z) Elf(Z, &debug_stream, strip ? new (Z) Dwarf(Z) : dwarf)
-          : nullptr;
-
-  // Here, both VM and isolate will be compiled into a single snapshot.
-  // In assembly generation, each serialized text section gets a separate
-  // pointer into the BSS segment and BSS slots are created for each, since
-  // we may not serialize both VM and isolate. Here, we always serialize both,
-  // so make a BSS segment large enough for both, with the VM entries coming
-  // first.
-  auto const isolate_offset = BSS::kVmEntryCount * compiler::target::kWordSize;
-  auto const bss_size =
-      isolate_offset + BSS::kIsolateEntryCount * compiler::target::kWordSize;
-  // Note that the BSS section must come first because it cannot be placed in
-  // between any two non-writable segments, due to a bug in Jelly Bean's ELF
-  // loader. See also Elf::WriteProgramTable().
-  const intptr_t vm_bss_base = elf->AddBSSData("_kDartBSSData", bss_size);
-  const intptr_t isolate_bss_base = vm_bss_base + isolate_offset;
-  // Add the BSS section to the separately saved debugging information, even
-  // though there will be no code in it to relocate, since it precedes the
-  // .text sections and thus affects their virtual addresses.
-  if (debug_elf != nullptr) {
-    debug_elf->AddBSSData("_kDartBSSData", bss_size);
-  }
+      generate_debug ? new (Z) Elf(Z, &debug_stream, Elf::Type::DebugInfo,
+                                   strip ? new (Z) Dwarf(Z) : dwarf)
+                     : nullptr;
 
   BlobImageWriter vm_image_writer(T, &vm_snapshot_instructions_buffer,
-                                  ApiReallocate, kInitialSize, debug_elf,
-                                  vm_bss_base, elf);
+                                  ApiReallocate, kInitialSize, debug_elf, elf);
   BlobImageWriter isolate_image_writer(T, &isolate_snapshot_instructions_buffer,
                                        ApiReallocate, kInitialSize, debug_elf,
-                                       isolate_bss_base, elf);
+                                       elf);
   FullSnapshotWriter writer(Snapshot::kFullAOT, &vm_snapshot_data_buffer,
                             &isolate_snapshot_data_buffer, ApiReallocate,
                             &vm_image_writer, &isolate_image_writer);
 
   writer.WriteFullSnapshot();
-  elf->AddROData(kVmSnapshotDataAsmSymbol, vm_snapshot_data_buffer,
-                 writer.VmIsolateSnapshotSize());
-  elf->AddROData(kIsolateSnapshotDataAsmSymbol, isolate_snapshot_data_buffer,
-                 writer.IsolateSnapshotSize());
 
   elf->Finalize();
   if (debug_elf != nullptr) {

runtime/vm/elf.cc

@@ -678,14 +678,29 @@ class DynamicSegment : public BlobSection {
 
 static const intptr_t kProgramTableSegmentSize = Elf::kPageSize;
 
-Elf::Elf(Zone* zone, StreamingWriteStream* stream, Dwarf* dwarf)
+// Here, both VM and isolate will be compiled into a single snapshot.
+// In assembly generation, each serialized text section gets a separate
+// pointer into the BSS segment and BSS slots are created for each, since
+// we may not serialize both VM and isolate. Here, we always serialize both,
+// so make a BSS segment large enough for both, with the VM entries coming
+// first.
+static const intptr_t kBssIsolateOffset =
+    BSS::kVmEntryCount * compiler::target::kWordSize;
+static const intptr_t kBssSize =
+    kBssIsolateOffset + BSS::kIsolateEntryCount * compiler::target::kWordSize;
+
+Elf::Elf(Zone* zone, StreamingWriteStream* stream, Type type, Dwarf* dwarf)
     : zone_(zone),
       unwrapped_stream_(stream),
+      type_(type),
       dwarf_(dwarf),
+      bss_(CreateBSS(zone, type, kBssSize)),
       shstrtab_(new (zone) StringTable(/*allocate=*/false)),
       dynstrtab_(new (zone) StringTable(/*allocate=*/true)),
       dynsym_(new (zone) SymbolTable(/*dynamic=*/true)),
       memory_offset_(kProgramTableSegmentSize) {
+  // Separate debugging information should always have a Dwarf object.
+  ASSERT(type_ == Type::Snapshot || dwarf_ != nullptr);
   // Assumed by various offset logic in this file.
   ASSERT_EQUAL(unwrapped_stream_->position(), 0);
   // The first section in the section header table is always a reserved
@@ -702,6 +717,19 @@ Elf::Elf(Zone* zone, StreamingWriteStream* stream, Dwarf* dwarf)
     AddSection(symtab_, ".symtab");
     symtab_->section_link = strtab_->section_index();
   }
+  // Note that the BSS segment must be the first user-defined segment because
+  // it cannot be placed in between any two non-writable segments, due to a bug
+  // in Jelly Bean's ELF loader. See also Elf::WriteProgramTable().
+  //
+  // We add it in all cases, even to the separate debugging information ELF,
+  // to ensure that relocated addresses are consistent between ELF snapshots
+  // and ELF separate debugging information.
+  AddSection(bss_, ".bss");
+  AddSegmentSymbol(bss_, "_kDartBSSData");
+}
+
+uword Elf::BssStart(bool vm) const {
+  return bss_->memory_offset() + (vm ? 0 : kBssIsolateOffset);
 }
 
 void Elf::AddSection(Section* section, const char* name) {
@@ -734,11 +762,14 @@ intptr_t Elf::AddSegmentSymbol(const Section* section, const char* name) {
 }
 
 intptr_t Elf::AddText(const char* name, const uint8_t* bytes, intptr_t size) {
-  Section* image = nullptr;
-  if (bytes != nullptr) {
-    image = new (zone_) ProgramBits(true, true, false, bytes, size);
-  } else {
+  // When making a separate debugging info file for assembly, we don't have
+  // the binary text segment contents.
+  ASSERT(type_ == Type::DebugInfo || bytes != nullptr);
+  Section* image;
+  if (type_ == Type::DebugInfo) {
     image = new (zone_) NoBits(true, true, false, size);
+  } else {
+    image = new (zone_) ProgramBits(true, true, false, bytes, size);
   }
   AddSection(image, ".text");
 
@@ -776,28 +807,34 @@ bool Elf::FindStaticSymbol(const char* name,
   return FindSymbol(strtab_, symtab_, name, offset, size);
 }
 
-intptr_t Elf::AddBSSData(const char* name, intptr_t size) {
-  // Ideally the BSS segment would take no space in the object, but Android's
-  // "strip" utility truncates the memory-size of our segments to their
-  // file-size.
-  //
-  // Therefore we must insert zero-filled pages for the BSS.
-  uint8_t* const bytes = zone_->Alloc<uint8_t>(size);
-  memset(bytes, 0, size);
-
-  ProgramBits* const image =
-      new (zone_) ProgramBits(true, false, true, bytes, size);
+Section* Elf::CreateBSS(Zone* zone, Type type, intptr_t size) {
+  Section* image;
+  if (type == Type::DebugInfo) {
+    image = new (zone) NoBits(true, false, true, size);
+  } else {
+    // Ideally the BSS segment would take no space in the object, but Android's
+    // "strip" utility truncates the memory-size of our segments to their
+    // file-size.
+    //
+    // Therefore we must insert zero-filled pages for the BSS.
+    uint8_t* const bytes = zone->Alloc<uint8_t>(size);
+    memset(bytes, 0, size);
+    image = new (zone) ProgramBits(true, false, true, bytes, size);
+  }
   static_assert(Image::kBssAlignment <= kPageSize,
                 "ELF .bss section is not aligned as expected by Image class");
   ASSERT_EQUAL(image->alignment, kPageSize);
-  AddSection(image, ".bss");
-
-  return AddSegmentSymbol(image, name);
+  return image;
 }
 
 intptr_t Elf::AddROData(const char* name, const uint8_t* bytes, intptr_t size) {
   ASSERT(bytes != nullptr);
-  ProgramBits* image = new (zone_) ProgramBits(true, false, false, bytes, size);
+  Section* image;
+  if (type_ == Type::DebugInfo) {
+    image = new (zone_) NoBits(true, false, false, size);
+  } else {
+    image = new (zone_) ProgramBits(true, false, false, bytes, size);
+  }
   AddSection(image, ".rodata");
 
   return AddSegmentSymbol(image, name);

runtime/vm/elf.h

@@ -24,7 +24,18 @@ class SymbolTable;
 
 class Elf : public ZoneAllocated {
  public:
-  Elf(Zone* zone, StreamingWriteStream* stream, Dwarf* dwarf = nullptr);
+  enum class Type {
+    // A snapshot that should include segment contents.
+    Snapshot,
+    // Separately compiled debugging information that should not include
+    // most segment contents.
+    DebugInfo,
+  };
+
+  Elf(Zone* zone,
+      StreamingWriteStream* stream,
+      Type type,
+      Dwarf* dwarf = nullptr);
 
   static const intptr_t kPageSize = 4096;
 
@@ -32,11 +43,12 @@ class Elf : public ZoneAllocated {
   const Dwarf* dwarf() const { return dwarf_; }
   Dwarf* dwarf() { return dwarf_; }
 
+  uword BssStart(bool vm) const;
+
   intptr_t NextMemoryOffset() const { return memory_offset_; }
   intptr_t NextSectionIndex() const { return sections_.length(); }
   intptr_t AddText(const char* name, const uint8_t* bytes, intptr_t size);
   intptr_t AddROData(const char* name, const uint8_t* bytes, intptr_t size);
-  intptr_t AddBSSData(const char* name, intptr_t size);
   void AddDebug(const char* name, const uint8_t* bytes, intptr_t size);
 
   // Returns whether the symbol was found. If found, sets the contents of
@@ -66,6 +78,8 @@ class Elf : public ZoneAllocated {
                         intptr_t address,
                         intptr_t size);
 
+  static Section* CreateBSS(Zone* zone, Type type, intptr_t size);
+
   const Section* FindSegmentForAddress(intptr_t address) const;
 
   void FinalizeDwarfSections();
@@ -79,10 +93,15 @@ class Elf : public ZoneAllocated {
 
   Zone* const zone_;
   StreamingWriteStream* const unwrapped_stream_;
+  const Type type_;
   // If nullptr, then the ELF file should be stripped of static information like
   // the static symbol table (and its corresponding string table).
   Dwarf* const dwarf_;
 
+  // We always create a BSS section for all Elf files, though it may be NOBITS
+  // if this is separate debugging information.
+  Section* const bss_;
+
   // All our strings would fit in a single page. However, we use separate
   // .shstrtab and .dynstr to work around a bug in Android's strip utility.
   StringTable* const shstrtab_;

runtime/vm/image_snapshot.cc

@@ -418,6 +418,8 @@ void ImageWriter::Write(WriteStream* clustered_stream, bool vm) {
 
   offset_space_ = vm ? V8SnapshotProfileWriter::kVmText
                      : V8SnapshotProfileWriter::kIsolateText;
+  // Needs to happen after WriteROData, because all image writers currently
+  // add the clustered data information to their output in WriteText().
   WriteText(clustered_stream, vm);
 }
 
@@ -1190,12 +1192,10 @@ BlobImageWriter::BlobImageWriter(Thread* thread,
                                  ReAlloc alloc,
                                  intptr_t initial_size,
                                  Elf* debug_elf,
-                                 intptr_t bss_base,
                                  Elf* elf)
     : ImageWriter(thread),
       instructions_blob_stream_(instructions_blob_buffer, alloc, initial_size),
       elf_(elf),
-      bss_base_(bss_base),
       debug_elf_(debug_elf) {
 #if defined(DART_PRECOMPILER)
   ASSERT(debug_elf_ == nullptr || debug_elf_->dwarf() != nullptr);
@@ -1248,7 +1248,8 @@ void BlobImageWriter::WriteText(WriteStream* clustered_stream, bool vm) {
 #if defined(DART_PRECOMPILER)
   // Store the offset of the BSS section from the instructions section here.
   // If not compiling to ELF (and thus no BSS segment), write 0.
-  const word bss_offset = elf_ != nullptr ? bss_base_ - segment_base : 0;
+  const word bss_offset =
+      elf_ != nullptr ? elf_->BssStart(vm) - segment_base : 0;
   ASSERT_EQUAL(Utils::RoundDown(bss_offset, Image::kBssAlignment), bss_offset);
   // Set the lowest bit if we are compiling to ELF.
   const word compiled_to_elf = elf_ != nullptr ? 0x1 : 0x0;
@@ -1479,17 +1480,26 @@ void BlobImageWriter::WriteText(WriteStream* clustered_stream, bool vm) {
   ASSERT_EQUAL(text_offset, image_size);
 
 #ifdef DART_PRECOMPILER
+  auto const data_symbol =
+      vm ? kVmSnapshotDataAsmSymbol : kIsolateSnapshotDataAsmSymbol;
   if (elf_ != nullptr) {
     auto const segment_base2 =
         elf_->AddText(instructions_symbol, instructions_blob_stream_.buffer(),
                       instructions_blob_stream_.bytes_written());
     ASSERT(segment_base == segment_base2);
+    // Write the .rodata section here like the AssemblyImageWriter.
+    elf_->AddROData(data_symbol, clustered_stream->buffer(),
+                    clustered_stream->bytes_written());
   }
   if (debug_elf_ != nullptr) {
-    auto const debug_segment_base2 =
-        debug_elf_->AddText(instructions_symbol, nullptr,
-                            instructions_blob_stream_.bytes_written());
+    // To keep memory addresses consistent, we need to add corresponding
+    // sections (though these will be NOBITS sections).
+    auto const debug_segment_base2 = debug_elf_->AddText(
+        instructions_symbol, instructions_blob_stream_.buffer(),
+        instructions_blob_stream_.bytes_written());
     ASSERT(debug_segment_base == debug_segment_base2);
+    debug_elf_->AddROData(data_symbol, clustered_stream->buffer(),
+                          clustered_stream->bytes_written());
   }
 #endif
 }

runtime/vm/image_snapshot.h

@@ -429,7 +429,6 @@ class BlobImageWriter : public ImageWriter {
                   ReAlloc alloc,
                   intptr_t initial_size,
                   Elf* debug_elf = nullptr,
-                  intptr_t bss_base = 0,
                   Elf* elf = nullptr);
 
   virtual void WriteText(WriteStream* clustered_stream, bool vm);
@@ -443,7 +442,6 @@ class BlobImageWriter : public ImageWriter {
 
   WriteStream instructions_blob_stream_;
   Elf* const elf_;
-  const intptr_t bss_base_;
   Elf* const debug_elf_;
 
   DISALLOW_COPY_AND_ASSIGN(BlobImageWriter);

tests/standalone_2/dwarf_stack_trace_test.dart

@@ -71,9 +71,12 @@ Future<void> checkStackTrace(String rawStack, Dwarf dwarf,
   final absolutes = absoluteAddresses(rawLines);
   final relocatedAddresses = absolutes.map((a) => a - dsoBase);
   final explicits = explicitVirtualAddresses(rawLines);
-  Expect.deepEquals(relocatedAddresses, virtualAddresses);
-  // Explicits will be empty if not generating ELF snapshots directly.
+
+  // Explicits will be empty if not generating ELF snapshots directly, which
+  // means we can't depend on virtual addresses in the snapshot lining up with
+  // those in the separate debugging information.
   if (explicits.isNotEmpty) {
+    Expect.deepEquals(relocatedAddresses, virtualAddresses);
     Expect.deepEquals(explicits, virtualAddresses);
   }