This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj/llvm-objdump] - Improve how tool locate the dynamic table and report warnings about that.
ClosedPublic

Authored by grimar on Sep 13 2019, 6:17 AM.

Details

Summary

Before this patch we gave a priority to a dynamic table found
from the section header.

It was discussed (here: https://reviews.llvm.org/D67078?id=218356#inline-602082)
that probably preferring the table from PT_DYNAMIC is better,
because it is what runtime loader sees.

This patch makes the table from PT_DYNAMIC be chosen at first place if it is available.
But also it adds logic to fall back to SHT_DYNAMIC if the table from the dynamic segment is
broken or fall back to use no table if both are broken.

It adds a few more diagnostic warnings for the logic above.

Diff Detail

Event Timeline

grimar created this revision.Sep 13 2019, 6:17 AM
grimar updated this revision to Diff 220088.Sep 13 2019, 6:21 AM
  • A cosmetic cleanup.
jhenderson added inline comments.Sep 16 2019, 2:27 AM
tools/llvm-readobj/ELFDumper.cpp
1492

dump the dynamic sections -> dump dynamic sections

1503–1504

When we only have information from one of the SHT_DYNAMIC section header or PT_DYNAMIC program header, just use that.

1522–1523

I'd make this error message say "PT_DYNAMIC program header" for consistency, since we call the section header the "SHT_DYNAMIC section header".

1532

header -> program header

1538

"invalid," -> "invalid:"

1541

Delete this blank line

1545

"invalid," -> "invalid:"

grimar updated this revision to Diff 220304.Sep 16 2019, 3:38 AM
grimar marked 7 inline comments as done.
  • Addressed review comments.
jhenderson accepted this revision.Sep 16 2019, 3:59 AM

LGTM. Probably check with @MaskRay though before committing.

This revision is now accepted and ready to land.Sep 16 2019, 3:59 AM
MaskRay added inline comments.Sep 16 2019, 4:16 AM
tools/llvm-readobj/ELFDumper.cpp
1500

The logic is:

findDynamic
  find PT_DYNAMIC
  find .dynamic
  if p_offset is out of bounds
    reportWarning
  if both PT_DYNAMIC and .dynamic exist
    check if the information matches

loadDynamicTable
  findDynamic()
  if PT_DYNAMIC exists
    IsPhdrTableValid = PT_DYNAMIC's DynRegionInfo is not empty
  if .dynamic exists
    IsSecTableValid = .dynamic's DynRegionInfo is not empty
  if either does not exist
    if (IsPhdrTableValid && IsSecTableValid)
      parseDynamicTable()
    else
      warn
    return

  validate information matches
  ...

Shall we check whether p_offset is out of bounds and whether DynRegionInfo of PT_DYNAMIC is empty together?

findDynamic
  find PT_DYNAMIC
  IsPhdrTableValid = whether PT_DYNAMIC exists
  if p_offset is out of bounds or DynRegionInfo not empty
    reportWarning
    IsPhdrTableValid = false

  find .dynamic
  validate .dynamic

  if either does not exist
    if ((DynamicPhdr && IsPhdrTableValid) || (DynamicSec && IsSecTableValid))
      parseDynamicTable()
    else
      warn
    return

  ...
  validate information matches
  parseDynamicTable()
grimar marked an inline comment as done.Sep 16 2019, 5:19 AM
grimar added inline comments.
tools/llvm-readobj/ELFDumper.cpp
1500

I am not sure I understand what you suggest, sorry, Do you want to merge 2 existing methods into one large one?
(I do not understand why your findDynamic might call arseDynamicTable() for example).

MaskRay added inline comments.Sep 16 2019, 5:41 AM
tools/llvm-readobj/ELFDumper.cpp
1500

Do you want to merge 2 existing methods into one large one?

Yes. Locate PT_DYNAMIC, and do all PT_DYNAMIC specific checks. Then, locate .dynamic, and do all .dynamic related checks. Last, check whether PT_DYNAMIC and .dynamic mismatch.

grimar marked an inline comment as done.Sep 16 2019, 5:50 AM
grimar added inline comments.
tools/llvm-readobj/ELFDumper.cpp
1500

Ok. Now things became clear :) I spent some time experimenting with this code while worked on a patch and one of things I tried to achieve was to reduce the amount of the code in each method. I.e. I did splitting intentionally (and was very happy about that), because it looks simpler to me generally to work with a smaller pieces of code.
Even now, looking at loadDynamicTable it seems a bit too large to me.

Are you strong it this suggestion? I.e. I am not sure it will look better, but I do not mind to try to do it again if you really insist.

MaskRay accepted this revision.Sep 17 2019, 3:15 AM
MaskRay added inline comments.
tools/llvm-readobj/ELFDumper.cpp
1500

I am not strong about the suggestion. I think the functions are still long after the split (the current patch). I was trying to figure out why we added so many lines for the extra warnings.

p_offset is out of bounds or DynRegionInfo not empty

I was hoping regrouping the diagnostics can simplify the code, and then there may be other ways to split the function, but maybe I am wrong.

1564

Delete return;

This revision was automatically updated to reflect the committed changes.
grimar marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2019, 6:57 AM