Page MenuHomePhabricator

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

Authored by grimar on Apr 7 2020, 8:30 AM.

Details

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.

Diff Detail

Event Timeline

grimar created this revision.Apr 7 2020, 8:30 AM
jhenderson added inline comments.Apr 8 2020, 12:30 AM
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
316

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.

317

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

319

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

319–321

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 updated this revision to Diff 255931.Apr 8 2020, 2:30 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.
llvm/tools/obj2yaml/elf2yaml.cpp
319–321

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 added inline comments.Apr 9 2020, 12:18 AM
llvm/tools/obj2yaml/elf2yaml.cpp
318

the segment

319–321

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

grimar marked an inline comment as done.Apr 9 2020, 1:45 AM
grimar added inline comments.
llvm/tools/obj2yaml/elf2yaml.cpp
319–321

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 updated this revision to Diff 256297.Apr 9 2020, 7:19 AM
MaskRay added inline comments.Apr 9 2020, 1:06 PM
llvm/test/tools/obj2yaml/program-headers.yaml
430

This line can use -NEXT:

llvm/tools/obj2yaml/elf2yaml.cpp
319–321

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 marked an inline comment as done.Apr 10 2020, 1:00 AM
grimar added inline comments.
llvm/tools/obj2yaml/elf2yaml.cpp
319–321

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

jhenderson added inline comments.Apr 16 2020, 1:24 AM
llvm/tools/obj2yaml/elf2yaml.cpp
319–321

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 planned changes to this revision.Apr 17 2020, 4:18 AM
grimar updated this revision to Diff 258727.Apr 20 2020, 7:06 AM
grimar marked 2 inline comments as done.
  • Rebased.
  • Addressed comments.
llvm/tools/obj2yaml/elf2yaml.cpp
319–321

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 added inline comments.Apr 21 2020, 1:00 AM
llvm/tools/obj2yaml/elf2yaml.cpp
319–321

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.

319–321

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 updated this revision to Diff 258969.Apr 21 2020, 5:20 AM
grimar marked an inline comment as done.
  • Addressed review comments.
llvm/tools/obj2yaml/elf2yaml.cpp
319–321

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 added inline comments.Apr 21 2020, 5:26 AM
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 updated this revision to Diff 258993.Apr 21 2020, 7:23 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.
This revision is now accepted and ready to land.Apr 22 2020, 12:34 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2020, 2:40 AM