[obj2yaml] - Fix the issue with dumping empty sections when dumping program headers.

[grimar]

Summary

Imagine we have:

ProgramHeaders:
  - Type:  PT_LOAD
    Flags: [ PF_W, PF_R ]
    Sections:
      - Section: .bar
    VAddr: 0x2000
Sections:
  - Name:    .foo
    Type:    SHT_PROGBITS
    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
    Address: 0x1000
  - Name:    .bar
    Type:    SHT_PROGBITS
    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
    Address: 0x2000

Both .foo and .bar share the same starting file offset,
but VA(.foo) < VA(PT_LOAD), we should not include it into segment.

This patch fixes the issue.

[jhenderson]

llvm/test/tools/obj2yaml/program-headers.yaml

423

has a VA
include it in it

426

outside of segment's -> outside the segment's

llvm/tools/obj2yaml/elf2yaml.cpp

311

I'm not sure this first part of the comment is relevant: it's not about whether two sections share the same offset, it's about what to do with empty sections on the edges of a program header. There could be no other sections present at the same offset, but we should still include/not include the section in that case, based on its address.

312

such section -> such a section
outside of -> outside

314

I'd find Shdr.sh_size == 0 to be easier to read.

314–316

I think this will change the behaviour for a (malformed) input with an empty section in the middle of a segment, where that section has a mismatching address. I'd be inclined to say that the section should be listed in the segment, although I'm not sure if it actually matters either way.

[grimar]

• Addressed review comments.

llvm/tools/obj2yaml/elf2yaml.cpp

314–316

I think the right way would be to implement the missing functionality first (e.g. I'll post a patch for SHT_NOBITS soon, this diff is the preparation),
and then we can add support for tricky mailformed cases (if we want to support them) when we'll see the whole code.

[jhenderson]

llvm/tools/obj2yaml/elf2yaml.cpp

313

the segment

314–316

I'm not sure I follow what you mean by "missing functionality" here?

[grimar]

llvm/tools/obj2yaml/elf2yaml.cpp

314–316

isInSegment is incomplete yet for valid cases: it doesn't work with SHT_NOBITS correctly and these particular lines will probably be reused because we need to check virtual addresses and not file offsets. So the code will change more. My point was that we need to implement everything we know we want and then can switch to supporting broken cases like you've mentioned.

"a (malformed) input with an empty section in the middle of a segment, where that section has a mismatching address" just doesn't sound like a important real case that we need to handle before the changes I've mentioned above or am I missing something?

[grimar]

• Addressed review comment.
• Rebased after rG56a8150428d45e6831d60e8d547626780634f511.

[MaskRay]

llvm/test/tools/obj2yaml/program-headers.yaml

430

This line can use -NEXT:

llvm/tools/obj2yaml/elf2yaml.cpp

314–316

My feeling is that we should handle SHT_NOBITS first (straightforward), then think about the non-SHF_NOBITS sh_size=0 case (which may involve several alternative designs).

[grimar]

llvm/tools/obj2yaml/elf2yaml.cpp

314–316

This patch fixes the issue we have (see Object/obj2yaml.test).

[jhenderson]

llvm/tools/obj2yaml/elf2yaml.cpp

314–316

Maybe I'm missing something, but can't we just change the if to something along the lines of if (shdr.sh_size == 0 && (shdr.sh_type == SHT_NOBITS || shdr.sh_offset == phdr.p_offset || shdr.sh_offset == phdr.p_offset + phdr.p_filesz))? It's not like it's that complicated and it would avoid changing the behaviour for these sections.

[grimar]

• Rebased.
• Addressed comments.

llvm/tools/obj2yaml/elf2yaml.cpp

314–316

but can't we just change the if to something along the lines of if (shdr.sh_size == 0 && (shdr.sh_type == SHT_NOBITS || shdr.sh_offset == phdr.p_offset || shdr.sh_offset == phdr.p_offset + phdr.p_filesz))?

This doesn't work for obj2yaml.test fixed by this patch I think.
It uses a valid precompiled object (trivial-object-test.elf-avr) which has the following layout:

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            00000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        00000000 000074 000004 00  AX  0   0  2
  [ 2] .data             PROGBITS        00800060 000078 000000 00  WA  0   0  1
  [ 3] .shstrtab         STRTAB          00000000 000078 000027 00      0   0  1
  [ 4] .symtab           SYMTAB          00000000 0000a0 000110 10      5   5  4
  [ 5] .strtab           STRTAB          00000000 0001b0 0000a6 00      0   0  1

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000074 0x00000000 0x00000000 0x00004 0x00004 R E 0x2
  LOAD           0x000078 0x00800060 0x00000004 0x00000 0x00000 RW  0x1

The .data should not be included to the first PT_LOAD.

(I think first of all we want to focus on valid cases and seems we have to check VAs for that?).

[jhenderson]

llvm/tools/obj2yaml/elf2yaml.cpp

314–316

Maybe we're misunderstanding each other. I'm suggesting the following:

if (FileOffsetsMatch) {
  if (SHdr.sh_size == 0 && (shdr.sh_offset == phdr.p_offset ||
                            shdr.sh_offset == phdr.p_offset + phdr.p_filesz))
    return VirtualAddressesMatch;
}

Looking at the layout in your comment, that should work for .data, I believe?

I think it's important, so that we don't have a divergence in behaviour here between empty and non-empty segments.

314–316

I think it's important, so that we don't have a divergence in behaviour here between empty and non-empty segments.

segments -> sections

[grimar]

• Addressed review comments.

llvm/tools/obj2yaml/elf2yaml.cpp

314–316

Maybe we're misunderstanding each other. I'm suggesting the following.

Yes, I've not got your idea right at first, sorry.
It looks good I think. Thanks!

(I've added the test case for the new condition)

[jhenderson]

llvm/test/tools/obj2yaml/program-headers.yaml

589

to a segment -> in a segment

I think you can delete "usually" here too.

590

it means -> it may mean

614

I missed that this segment had an address initially. I'd put it before the Sections list.

[grimar]

• Addressed review comments.

[jhenderson]

LGTM.

llvm/test/Object/obj2yaml.test

 # ELF-AVR-NEXT:   Type:            ET_EXEC
 # ELF-AVR-NEXT:   Machine:         EM_AVR
 # ELF-AVR-NEXT:   Flags:           [ EF_AVR_ARCH_AVR2 ]
 # ELF-AVR-NEXT: ProgramHeaders:
 # ELF-AVR-NEXT:  - Type:  PT_LOAD
 # ELF-AVR-NEXT:    Flags: [ PF_X, PF_R ]
 # ELF-AVR-NEXT:    Sections:
 # ELF-AVR-NEXT:     - Section: .text
-# ELF-AVR-NEXT:     - Section: .data
 # ELF-AVR-NEXT:    Align: 0x0000000000000002
 # ELF-AVR-NEXT:  - Type:  PT_LOAD
 # ELF-AVR-NEXT:    Flags: [ PF_W, PF_R ]
 # ELF-AVR-NEXT:    Sections:
 # ELF-AVR-NEXT:     - Section: .data
 # ELF-AVR-NEXT:    VAddr: 0x0000000000800060
 # ELF-AVR-NEXT:    PAddr: 0x0000000000000004
 # ELF-AVR-NEXT: Sections:

llvm/test/tools/obj2yaml/program-headers.yaml

- Name:    .alloc.2
     Flags:   [ SHF_ALLOC ]
     Size:    0x1
 
 ## Check how we dump segments which contain SHT_NOBITS sections.
 # RUN: yaml2obj --docnum=5 %s -o %t5
 # RUN: obj2yaml %t5 | FileCheck %s --check-prefix=NOBITS
 
 # NOBITS:      ProgramHeaders:
 # NOBITS-NEXT:   - Type:  PT_LOAD
 # NOBITS-NEXT:     Flags: [ PF_W, PF_R ]
 # NOBITS-NEXT:     Sections:
 # NOBITS-NEXT:       - Section: .bss
 # NOBITS-NEXT:   - Type:  PT_LOAD
 # NOBITS-NEXT:     Flags: [ PF_W, PF_R ]
 # NOBITS-NEXT:     Sections:
 # NOBITS-NEXT:       - Section: .data.1
 # NOBITS-NEXT:       - Section: .bss
 # NOBITS-NEXT:   - Type:  PT_LOAD
 # NOBITS-NEXT:     Flags: [ PF_W, PF_R ]
 # NOBITS-NEXT:     Sections:
 # NOBITS-NEXT:       - Section: .data.1
 # NOBITS-NEXT:       - Section: .bss
 # NOBITS-NEXT:       - Section: .data.2
 # NOBITS-NEXT:   - Type:  PT_LOAD
- Name:    .bar
     Type:    SHT_PROGBITS
     Flags:   [ SHF_ALLOC ]
     Size:    0x1
   - Name:     .zed
     Type:     SHT_PROGBITS
     Flags:    [ SHF_ALLOC ]
     Size:     0x1
     ShOffset: 0x0
+
+## Check how we dump segments which contain empty sections.
+# RUN: yaml2obj --docnum=7 %s -o %t7
+
+## Show the layout of the object before we dump it using obj2yaml.
+## Notes: 1) '.empty.foo', '.empty.bar1' and '.bar' have the same file offset, but '.empty.foo'
+##           has a VA that is outside of the segment, hence we should not include it in it.
+##        2) '.bar1' ends at 0x79, which is the starting file offset of both '.empty.bar2'
+##           and '.empty.zed'. We should only include '.empty.bar2', because the VA of the
+##           '.empty.zed' section is outside the segment's virtual space.
+# RUN: llvm-readelf -sections %t7 | FileCheck %s --check-prefix=ZERO-SIZE-MAPPING
+
+# ZERO-SIZE-MAPPING:      Section Headers:
+# ZERO-SIZE-MAPPING-NEXT:   [Nr] Name        Type     Address          Off    Size
+# ZERO-SIZE-MAPPING:        [ 1] .empty.foo  PROGBITS 0000000000001000 000078 000000
+# ZERO-SIZE-MAPPING-NEXT:   [ 2] .empty.bar1 PROGBITS 0000000000002000 000078 000000
+# ZERO-SIZE-MAPPING-NEXT:   [ 3] .bar        PROGBITS 0000000000002000 000078 000001
+# ZERO-SIZE-MAPPING-NEXT:   [ 4] .empty.bar2 PROGBITS 0000000000002001 000079 000000
+# ZERO-SIZE-MAPPING-NEXT:   [ 5] .empty.zed  PROGBITS 0000000000003000 000079 000000
+
+# RUN: obj2yaml %t7 | FileCheck %s --check-prefix=ZERO-SIZE
+
+# ZERO-SIZE:      ProgramHeaders:
+# ZERO-SIZE-NEXT:   - Type:  PT_LOAD
+# ZERO-SIZE-NEXT:     Flags: [ PF_W, PF_R ]
+# ZERO-SIZE-NEXT:     Sections:
+# ZERO-SIZE-NEXT:       - Section: .empty.bar1
+# ZERO-SIZE-NEXT:       - Section: .bar
+# ZERO-SIZE-NEXT:       - Section: .empty.bar2
+# ZERO-SIZE-NEXT:     VAddr: 0x0000000000002000
+# ZERO-SIZE-NEXT: Sections:
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+ProgramHeaders:
+  - Type:  PT_LOAD
+    Flags: [ PF_W, PF_R ]
+    Sections:
+      - Section: .bar
+    VAddr: 0x2000
+Sections:
+  - Name:    .empty.foo
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address: 0x1000
+  - Name:    .empty.bar1
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address: 0x2000
+  - Name:    .bar
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_WRITE, SHF_ALLOC ]
+    Address: 0x2000
+    Size:    0x1
+  - Name:    .empty.bar2
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address: 0x2001
+  - Name:    .empty.zed
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address: 0x3000
+
+## Check how we dump a segment when we have sections that are outside of the virtual
+## address space of a segment, but inside its file space. We do not include such sections
+## in a segment when they are at the edges of a segment, because this is a normal case and
+## it may mean they belong to a different segment.
+# RUN: yaml2obj --docnum=8 %s -o %t8
+# RUN: obj2yaml %t8 | FileCheck %s --check-prefix=BROKEN-VA
+
+# BROKEN-VA:      ProgramHeaders:
+# BROKEN-VA-NEXT:  - Type:  PT_LOAD
+# BROKEN-VA-NEXT:    Flags: [ PF_W, PF_R ]
+# BROKEN-VA-NEXT:    Sections:
+# BROKEN-VA-NEXT:      - Section: .empty_middle
+# BROKEN-VA-NEXT:    VAddr: 0x0000000000001000
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+ProgramHeaders:
+  - Type:  PT_LOAD
+    Flags: [ PF_W, PF_R ]
+    VAddr: 0x1000
+    Sections:
+      - Section: .empty_begin
+      - Section: .empty_middle
+      - Section: .empty_end
+Sections:
+  - Name:    .empty_begin
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address: 0xFEFEFEFE
+  - Type:    Fill
+    Pattern: "00"
+    Size:    1
+    Name:    begin
+  - Name:    .empty_middle
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address: 0xFEFEFEFE
+  - Type:    Fill
+    Pattern: "00"
+    Size:    1
+  - Name:    .empty_end
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address: 0xFEFEFEFE

llvm/tools/obj2yaml/elf2yaml.cpp

if (Sec.Type == ELF::SHT_NULL)
     return false;
 
   // A section is within a segment when its location in a file is within the
   // [p_offset, p_offset + p_filesz] region.
   bool FileOffsetsMatch =
       SHdr.sh_offset >= Phdr.p_offset &&
       (SHdr.sh_offset + SHdr.sh_size <= Phdr.p_offset + Phdr.p_filesz);
 
-  if (FileOffsetsMatch)
+  bool VirtualAddressesMatch = SHdr.sh_addr >= Phdr.p_vaddr &&
+                               SHdr.sh_addr <= Phdr.p_vaddr + Phdr.p_memsz;
+
+  if (FileOffsetsMatch) {
+    // An empty section on the edges of a program header can be outside of the
+    // virtual address space of the segment. This means it is not included in
+    // the segment and we should ignore it.
+    if (SHdr.sh_size == 0 && (SHdr.sh_offset == Phdr.p_offset ||
+                              SHdr.sh_offset == Phdr.p_offset + Phdr.p_filesz))
+      return VirtualAddressesMatch;
     return true;
+  }
 
   // SHT_NOBITS sections usually occupy no physical space in a file. Such
   // sections belong to a segment when they reside in the segment's virtual
   // address space.
   if (Sec.Type != ELF::SHT_NOBITS)
     return false;
-
-  return SHdr.sh_addr >= Phdr.p_vaddr &&
-         SHdr.sh_addr <= Phdr.p_vaddr + Phdr.p_memsz;
+  return VirtualAddressesMatch;
 }
 
 template <class ELFT>
 Expected<std::vector<ELFYAML::ProgramHeader>>
 ELFDumper<ELFT>::dumpProgramHeaders(
     ArrayRef<std::unique_ptr<ELFYAML::Chunk>> Chunks) {
   std::vector<ELFYAML::ProgramHeader> Ret;
   Expected<typename ELFT::PhdrRange> PhdrsOrErr = Obj.program_headers();