Page MenuHomePhabricator

[Object/llvm-readelf/llvm-readobj] - Improve error reporting when e_shstrndx is broken.
ClosedPublic

Authored by grimar on Jul 15 2019, 3:45 AM.

Details

Summary

When e_shstrndx is broken, it is impossible to get a section name.
In this patch I improved the error message we show and
added tests for Object and for llvm-readelf/llvm-readobj

Message was changed in two places:

  • llvm-readelf/llvm-readobj previously used a code from Object/ELF.h,

now they have a modified version of it (it has less checks and allows
dumping broken things).

  • Code in Object/ELF.h is still used for generic cases.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jul 15 2019, 3:45 AM

I wonder if it would make sense if the file name was included in the error message, at least in the llvm-readobj/llvm-readelf case? In that case, there can be more than one file, or indeed it could be an archive member that is broken, so reporting the file name would make sense.

Aside from that, do we have a way yet of testing the e_shstrndx == SHN_XINDEX case yet for this error? I think that deserves its own test case once we do, so probably should have a TODO note somewhere around the relevant bits of code.

include/llvm/Object/ELF.h
471 ↗(On Diff #209788)

I'm not sure "SHN_XINDEX" in this context makes sense, since that isn't the thing that's doing the referring. How about simply "section header string table index <N> does not exist"?

tools/llvm-readobj/ELFDumper.cpp
3021 ↗(On Diff #209788)

Same comment applies here.

I wonder if it would make sense if the file name was included in the error message, at least in the llvm-readobj/llvm-readelf case? In that case, there can be more than one file, or indeed it could be an archive member that is broken, so reporting the file name would make sense.

We shouldn't do this in ELF.h probably, since tools are adding it on their side. e.g. in Invalid.test I am actually testing the file name printed by obj2yaml.
And for example LLD expects that adds a error location (including file names) on it's side too.

Speaking about llvm-readobj/llvm-readelf, I do not mind, going to update the code in ELFDumper.cpp to add a file name.

Aside from that, do we have a way yet of testing the e_shstrndx == SHN_XINDEX case yet for this error? I think that deserves its own test case once we do, so probably should have a TODO note somewhere around the relevant bits of code.

I believe obj2yaml/yaml2obj tool does not support the SHN_XINDEX (https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFYAML.cpp#L901) yet. I'll add such comments.

I wonder if it would make sense if the file name was included in the error message, at least in the llvm-readobj/llvm-readelf case? In that case, there can be more than one file, or indeed it could be an archive member that is broken, so reporting the file name would make sense.

We shouldn't do this in ELF.h probably, since tools are adding it on their side. e.g. in Invalid.test I am actually testing the file name printed by obj2yaml.
And for example LLD expects that adds a error location (including file names) on it's side too.

Speaking about llvm-readobj/llvm-readelf, I do not mind, going to update the code in ELFDumper.cpp to add a file name.

Sounds good to me.

grimar updated this revision to Diff 209846.Jul 15 2019, 6:35 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
jhenderson added inline comments.Jul 15 2019, 7:12 AM
include/llvm/Object/ELF.h
469 ↗(On Diff #209846)

It's not SHN_XINDEX that's broken (it can't be - "SHN_XINDEX" is just an enum value), it's the sh_link of the section with index 0.

grimar updated this revision to Diff 210042.Jul 16 2019, 1:26 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
include/llvm/Object/ELF.h
469 ↗(On Diff #209846)

Right. Fixed.

This revision is now accepted and ready to land.Jul 16 2019, 2:19 AM
MaskRay accepted this revision.Jul 16 2019, 2:49 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 4:07 AM