Page MenuHomePhabricator

[llvm-readelf] - Allow dumping of the .dynamic section even if there is no PT_DYNAMIC header.
ClosedPublic

Authored by grimar on May 21 2019, 2:48 AM.

Details

Summary

It is now possible after D61937 was landed and was discussed
in it's review comments. It is not consistent with GNU, which
does not output .dynamic section content in this case for
no visible reason.

Should it be only done for LLVM style output, i.e. for llvm-readobj,
but not for GNU (llvm-readelf)?

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.May 21 2019, 2:48 AM

This is an odd situation anyway (unless you know of a concrete example), so I don't think any users are likely to hit it either way. I don't think we need to follow GNU's behaviour here as a result. We should just do what makes the most sense (and GNU's behaviour doesn't make much sense to me). It would be worth getting somebody else's opinion on this too.

tools/llvm-readobj/ELFDumper.cpp
1363–1364 ↗(On Diff #200439)

might want -> will
the dynamic section found -> the found dynamic section

I don't think you need the last sentence.

grimar updated this revision to Diff 200667.May 22 2019, 2:08 AM
  • Updated comments.
jhenderson accepted this revision.May 22 2019, 3:39 AM

LGTM, but you may want to wait for others to comment too.

This revision is now accepted and ready to land.May 22 2019, 3:39 AM

LGTM, but you may want to wait for others to comment too.

Yep, I'll hold it for a while.

rupprecht accepted this revision.May 22 2019, 10:47 AM

This is an odd situation anyway (unless you know of a concrete example), so I don't think any users are likely to hit it either way. I don't think we need to follow GNU's behaviour here as a result. We should just do what makes the most sense (and GNU's behaviour doesn't make much sense to me). It would be worth getting somebody else's opinion on this too.

Same here... this patch seems fine, someone would need a use case for:

  • Having dynamic sections
  • Not having PT_HEADERs
  • Needing llvm-readelf to be quiet about it

Which seems unlikely, but maybe wait a day or two.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2019, 4:11 AM
grimar reopened this revision.May 24 2019, 5:27 AM
This revision is now accepted and ready to land.May 24 2019, 5:27 AM
grimar abandoned this revision.May 24 2019, 5:45 AM

I had to revert this one.
It broke the following tests in a different projects:
(My apologies that I did not notice that before posting on review)

lld :: ELF/linkerscript/empty-tls.test
lld :: ELF/linkerscript/pt-interp.test
LLVM :: Object/invalid.test
LLVM :: tools/yaml2obj/dynamic-section-raw-content.yaml

The error is the same, it is: "Virtual address is not in any segment"
and is coming from `here: https://github.com/llvm-mirror/llvm/blob/master/lib/Object/ELF.cpp#L559

which is called from a place where dynamic tags are parsed:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L1497

I looked on the empty-tls.test and the problem is that it contains only TLS prorgam header,
and the following call fails:

case ELF::DT_SYMTAB:
   DynSymRegion.Addr = toMappedAddr(Dyn.getPtr());
..

I.e. since there is no segment, covering the address of dynamic symbol table,
it fails to get its data properly.

We probably can make this error not critical, i.e. skip the dynamic entries that
requires other segnemts, but I am not sure it really worth the efford and
additional code complication.

I am abandodning this patch.

I had to revert this one.
It broke the following tests in a different projects:
(My apologies that I did not notice that before posting on review)

lld :: ELF/linkerscript/empty-tls.test
lld :: ELF/linkerscript/pt-interp.test
LLVM :: Object/invalid.test
LLVM :: tools/yaml2obj/dynamic-section-raw-content.yaml

The error is the same, it is: "Virtual address is not in any segment"
and is coming from `here: https://github.com/llvm-mirror/llvm/blob/master/lib/Object/ELF.cpp#L559

which is called from a place where dynamic tags are parsed:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L1497
...
We probably can make this error not critical, i.e. skip the dynamic entries that
requires other segnemts, but I am not sure it really worth the efford and
additional code complication.

I am abandodning this patch.

I've seen this same error internally when switching from GNU readelf to llvm-readelf and reading core files with it, and I'm interested in fixing that. GNU readelf does what you mention -- prints a warning and moves on. I think that's useful; we should be able to use tools to read partial information from a corrupt/incomplete file.

grimar reclaimed this revision.May 27 2019, 1:53 AM

I had to revert this one.
It broke the following tests in a different projects:
(My apologies that I did not notice that before posting on review)

lld :: ELF/linkerscript/empty-tls.test
lld :: ELF/linkerscript/pt-interp.test
LLVM :: Object/invalid.test
LLVM :: tools/yaml2obj/dynamic-section-raw-content.yaml

The error is the same, it is: "Virtual address is not in any segment"
and is coming from `here: https://github.com/llvm-mirror/llvm/blob/master/lib/Object/ELF.cpp#L559

which is called from a place where dynamic tags are parsed:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L1497
...
We probably can make this error not critical, i.e. skip the dynamic entries that
requires other segnemts, but I am not sure it really worth the efford and
additional code complication.

I am abandodning this patch.

I've seen this same error internally when switching from GNU readelf to llvm-readelf and reading core files with it, and I'm interested in fixing that. GNU readelf does what you mention -- prints a warning and moves on. I think that's useful; we should be able to use tools to read partial information from a corrupt/incomplete file.

OK. I'll resurrect this one and take a look.

This revision is now accepted and ready to land.May 27 2019, 1:53 AM
grimar planned changes to this revision.May 27 2019, 1:53 AM
grimar updated this revision to Diff 201530.May 27 2019, 7:14 AM
  • Updated implementation and test cases in according to latest discussion comments.

(now llvm-readelf bypass the "Virtual address is not in any segment" error and will try to continue parsing the object).

This revision is now accepted and ready to land.May 27 2019, 7:14 AM
grimar requested review of this revision.May 27 2019, 7:14 AM
rupprecht accepted this revision.May 28 2019, 1:06 PM

Thanks for following up with this! Just one optional suggestion:

tools/llvm-readobj/ELFDumper.cpp
1486 ↗(On Diff #201530)

Instead of constants like "DT_HASH" for all these changes, can you pass in Dyn.d_tag instead and use getTypeString() within toMappedAddr to print the string? (Passing Obj->getHeader()->e_machine by lambda capture). That will keep all these calls identical.

This revision is now accepted and ready to land.May 28 2019, 1:06 PM
grimar marked an inline comment as done.May 29 2019, 3:28 AM
grimar added inline comments.
tools/llvm-readobj/ELFDumper.cpp
1486 ↗(On Diff #201530)

Oh, cool, I did not know we have something like getTypeString() here (or perhaps forgot about that) , thanks!

This revision was automatically updated to reflect the committed changes.

There is a regression - after this patch - at least after its later commit:

commit 8ac7b2d07bd6042afe0e8618ca8682d7663f4be8
Author: George Rimar <grimar@accesssoftek.com>
Date:   Wed May 29 10:31:46 2019 +0000
    [llvm-readelf] - Allow dumping of the .dynamic section even if there is no PT_DYNAMIC header.
    Differential revision: https://reviews.llvm.org/D62179
    llvm-svn: 361943

llvm-readobj cannot print .dynsym of executable without Program Headers. GNU readelf can print it fine. We have found it with @kwk because of some unrelated bug in obj2yaml/`yaml2obj' dropping Program Headers.
On 64-bit Linux one can create a test binary with:

cp -p /bin/bash /tmp/bashtest;dd if=/dev/zero of=/tmp/bashtest bs=1 count=2 seek=$[0x38] conv=notrunc

llvm-readobj -dyn-symbols formerly printed for example:

Symbol {
  Name: tilde_expand_word (11424)
  Value: 0xDBE60

while now it prints only:

warning: Unable to parse DT_GNU_HASH: Virtual address is not in any segment
warning: Unable to parse DT_STRTAB: Virtual address is not in any segment
warning: Unable to parse DT_SYMTAB: Virtual address is not in any segment
warning: Unable to parse DT_JMPREL: Virtual address is not in any segment
warning: Unable to parse DT_RELA: Virtual address is not in any segment
File: /tmp/bashtest
Format: ELF64-x86-64
Arch: x86_64
AddressSize: 64bit 
LoadName:
DynamicSymbols [
]
grimar added a comment.Sep 2 2019, 1:18 AM

There is a regression - after this patch - at least after its later commit

I'll investigate.

grimar added a comment.Sep 2 2019, 6:44 AM

There is a regression - after this patch - at least after its later commit:

commit 8ac7b2d07bd6042afe0e8618ca8682d7663f4be8
Author: George Rimar <grimar@accesssoftek.com>
Date:   Wed May 29 10:31:46 2019 +0000
    [llvm-readelf] - Allow dumping of the .dynamic section even if there is no PT_DYNAMIC header.
    Differential revision: https://reviews.llvm.org/D62179
    llvm-svn: 361943

llvm-readobj cannot print .dynsym of executable without Program Headers. GNU readelf can print it fine. We have found it with @kwk because of some unrelated bug in obj2yaml/`yaml2obj' dropping Program Headers.
On 64-bit Linux one can create a test binary with:

cp -p /bin/bash /tmp/bashtest;dd if=/dev/zero of=/tmp/bashtest bs=1 count=2 seek=$[0x38] conv=notrunc

llvm-readobj -dyn-symbols formerly printed for example:

Symbol {
  Name: tilde_expand_word (11424)
  Value: 0xDBE60

while now it prints only:

warning: Unable to parse DT_GNU_HASH: Virtual address is not in any segment
warning: Unable to parse DT_STRTAB: Virtual address is not in any segment
warning: Unable to parse DT_SYMTAB: Virtual address is not in any segment
warning: Unable to parse DT_JMPREL: Virtual address is not in any segment
warning: Unable to parse DT_RELA: Virtual address is not in any segment
File: /tmp/bashtest
Format: ELF64-x86-64
Arch: x86_64
AddressSize: 64bit 
LoadName:
DynamicSymbols [
]

I posted a patch to fix this: https://reviews.llvm.org/D67078. Thanks for reporting!