[PATCH] D101332: [llvm-objcopy] Exclude empty sections in IHexWriter output

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D101332#2775675 <https://reviews.llvm.org/D101332#2775675>, @mciantyre wrote:

> I skipped the "does not share offset or address with any section" test case. There's a similar section in the existing `ihex-elf-sections2.yaml` test input, part of `ihex-writer.test`.

Would it make sense to move that test case into the new test? That way all empty section beahviour is localised to a single test.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-empty-sections.yaml:1
+# Evaluates the hex writer behavior with empty sections and segments.
+#
----------------
As this input is only used for a single test, I think it makes more sense to inline this directly in that test.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-empty-sections.yaml:38
+# An empty section that's placed at the end of .data0. This won't
+# be in the output
+  - Name:             .empty_behind_data0
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-empty-sections.yaml:70
+
+# The sections below are placed into segments of varying configurations
+  - Name:             .data2
----------------
Perhaps also explain which cases each section is testing with comments, like you do above.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-empty-sections.yaml:107
+ProgramHeaders:
+# .data0 sections, with empty bookends
+  - Type:             PT_LOAD
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-empty-sections.yaml:114
+    LastSec:          .empty_behind_data0
+# .data1 sections, with empty bookends
+  - Type:             PT_LOAD
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-empty-sections.yaml:123
+# Segments below include a single empty segment, and are positioned
+# around .data2 in various ways
+
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-empty-sections.yaml:125
+
+# Declared before, placed behind .data2 segment
+  - Type:             PT_LOAD
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-empty-sections.yaml:132
+    LastSec:          .empty0
+# Declared before, placed at .data2 segment
+  - Type:             PT_LOAD
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-empty-sections.yaml:139
+    LastSec:          .empty1
+# Segment for .data2
+  - Type:             PT_LOAD
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-empty-sections.yaml:146
+    LastSec:          .data2
+# Declared after, placed at .data2 segment
+  - Type:             PT_LOAD
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-empty-sections.yaml:153
+    LastSec:          .empty2
+# Declared after, placed behind .data2 segment
+  - Type:             PT_LOAD
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/ihex-writer-empty-sections.test:2
+# RUN: yaml2obj %p/Inputs/ihex-elf-empty-sections.yaml -o %t
+# RUN: llvm-objcopy -O ihex %t - | FileCheck %s
+
----------------
You probably want something like `--implicit-check-not={{.}}` in your FileCheck command, so that no other lines can appear in the output before are after the bits you are actually checking. If you do that, you can omit the trailing `CHECK-EMPTY`. At the moment, it looks like you could have output before your first checked line.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/ihex-writer-empty-sections.test:4
+
+# .data0 address
+# CHECK:         :02000004333394
----------------
Nit: In newer llvm-objcopy tests, we use `##` for comments to distinguish them from FileCheck and lit directives.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101332/new/

https://reviews.llvm.org/D101332