Page MenuHomePhabricator

[Object][XCOFF] Add support for 64-bit file header and section header dumping.
ClosedPublic

Authored by sfertile on Jun 26 2019, 2:04 PM.

Details

Summary

Adds a readobj dumper for 32-bit and 64-bit section header tables, and extend support for the file-header dumping to include 64-bit object files.

Diff Detail

Repository
rL LLVM

Event Timeline

sfertile created this revision.Jun 26 2019, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2019, 2:04 PM
DiggerLin added inline comments.Jun 27 2019, 2:15 PM
llvm/include/llvm/Object/XCOFFObjectFile.h
240 ↗(On Diff #206741)

I prefer to keep as uint16_t getFlags() const { return FileHdrPtr->Flags; };

llvm/lib/Object/XCOFFObjectFile.cpp
125 ↗(On Diff #206741)

there are assert(!is64Bit() && "Symbol table support not implemented for 64-bit."); in the toSymbolEntry() , I do not think we need assert(!is64Bit() && "Symbol table support not implemented for 64-bit."); here.

134 ↗(On Diff #206741)

ditto

186 ↗(On Diff #206741)

ditto

467 ↗(On Diff #206741)

it looks the flag as

support::big32_t Flags

in the XCOFFObjectFile.h

uint32_t XCOFFObjectFile::getSectionFlags(DataRefImpl Sec) const
should be int32_t XCOFFObjectFile::getSectionFlags(DataRefImpl Sec) const

501 ↗(On Diff #206741)

if is64Bit(). it should return here.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
142 ↗(On Diff #206741)

do you want to try to use W.printEnum to output Sec.Flags, it will be more readable

145 ↗(On Diff #206741)

do we need to print relocation here ?

148 ↗(On Diff #206741)

ditto

151 ↗(On Diff #206741)

ditto

MaskRay added inline comments.Jun 27 2019, 9:38 PM
llvm/lib/Object/XCOFFObjectFile.cpp
49 ↗(On Diff #206741)

getWithOffset does not seem useful: it just does a plus operation.

67 ↗(On Diff #206741)

You may use Offset >= getSectionHeaderSize() * getNumberOfSections() instead.

83 ↗(On Diff #206741)

I think inline reinterpret_cast may be clearer, then the viewAs helper can be deleted.

130 ↗(On Diff #206741)

return; is redundant.

364 ↗(On Diff #206741)

Consider making XCOFFObjectFile a class template.

See ELFObjectFile<ELFT> as an example, then you may avoid various is64Bit() dispatching in this file.

478 ↗(On Diff #206741)

warning: using the result of an assignment as a condition without parentheses [-Wparentheses]

I remember I said in the initial check-in of this file: lib/Object code is moving away from std::error_code, ErrorOr and errorCodeToError. Error Expected etc should be used instead.

sfertile marked 9 inline comments as done.Jun 28 2019, 5:49 AM
sfertile added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
240 ↗(On Diff #206741)

We can't because of the underlying type depends on if its a 64-bit object or a 32-bit object, do you mean you would prefer if

return is64Bit() ? fileHeader64()->Flags : fileHeader32()->Flags;

was inline here?

llvm/lib/Object/XCOFFObjectFile.cpp
125 ↗(On Diff #206741)

In the spirit of trying to keep this patch to a minimum I only implemented section headers and file-header support. I put defensive assert to indicate that I hadn't evaluated any of the symbol related interfaces for 64-bit. While this would technically work for 64-bit, that is only because the 64-bit symbol table entrey happens to also be 18 bytes of data. Casting a 64-bit symbol table entry to an XCOFFSymbolEntry is still wrong as they have different layouts.

134 ↗(On Diff #206741)

Again, the 64-bit and 32-bit symbol table entries have different layouts, this function AFAICT would not work for a 64-bit Symbol table entry. (Its String table offset member is at offset 8 instead of offset 4).

467 ↗(On Diff #206741)

Good catch, updated.

501 ↗(On Diff #206741)

is64Bit() has already returned on line 495.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
142 ↗(On Diff #206741)

Updated, thats a good suggestion.

145 ↗(On Diff #206741)

When that is supported, yes.

148 ↗(On Diff #206741)

Yep, once dumping symbols from objdump is supported.

151 ↗(On Diff #206741)

same.

sfertile updated this revision to Diff 207046.Jun 28 2019, 5:55 AM

Addressed first round of review comments.

  • Fix getSectionFlags return value to be signed.
  • Print the section flags as its symbolic value.
llvm/lib/Object/XCOFFObjectFile.cpp
49 ↗(On Diff #206741)

The function helps the code be more self-documenting about the intended semantics and is much more greppable than raw arithmetic. This allows pointer arithmetic to be properly performed in pointer space. Although not all such arithmetic is done in pointer space currently, that does not mean that we shouldn't keep the instances where they are.

83 ↗(On Diff #206741)

The viewAs helper is not new to this patch. Additionally, it saves on verbosity and noise that is distracting. Familiarity with viewAs is not difficult to attain.

DiggerLin marked an inline comment as done.Jun 28 2019, 7:07 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
501 ↗(On Diff #206741)

thanks.

sfertile updated this revision to Diff 207069.Jun 28 2019, 8:15 AM

Address further comments:

  • Simplified the address checking in 'checkSectionAddress'
  • Removed unneeded asserts. As Digger pointed out, an symbol related function that casts a DataRefImpl.p to a symbol table entry already asserts in toSymbolEntry.

Still outstanding: cleaning up of the error handling in the constructor.

sfertile marked 8 inline comments as done and an inline comment as not done.Jun 28 2019, 8:20 AM
sfertile added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
125 ↗(On Diff #206741)

Sorry Digger you are correct, I don't need to add an assert in any of the interfaces that cast a DataRefImpl to a SymbolEntry as the helper fucntion asserts. Updated.

478 ↗(On Diff #206741)

Attempting to clean this up next.

sfertile updated this revision to Diff 207142.Jun 28 2019, 2:08 PM

Converted the way parsing the binary is done to not need an error_code out argument in the constructor. Creates a bare bones object file, then attempts to parse the expected structures out of the memory buffer. Trying to match the style ELF used.

llvm/include/llvm/Object/XCOFFObjectFile.h
162 ↗(On Diff #207142)

Typo: "Constuctor [sic]".

Suggestion:
Constructor and "create" factory function. The constructor is only a thin wrapper around the base constructor. The "create" function fills out the XCOFF-specific information and performs the error checking along the way.

168 ↗(On Diff #207142)

I would suggest adding an empty line before this comment block. Also, break the sentence just before "returns" (where the comma is).

173 ↗(On Diff #207142)

s/freind/friend/;

223 ↗(On Diff #207142)

If the extra empty line is meant to signify that the interface functions below are not inherited from a base class, then perhaps a comment would work better.

227 ↗(On Diff #207142)

This is a public function, so there should be an assertion regarding use when is64Bit() is true.

248 ↗(On Diff #207142)

Add an empty line before this. Also, missing "as" before "an index".

llvm/lib/Object/XCOFFObjectFile.cpp
187 ↗(On Diff #207142)

Remove the space before the return type.

386 ↗(On Diff #207142)

What does SI mean?

475 ↗(On Diff #207142)

Use either "string table's size" or "string-table size".
Typo: s/as string/a string/;

This patch seems to have suffered from scope creep. The replacement of error_code has led to a considerable shifting around of the code that makes it more difficult to review the other part of the patch. Since it has already happened, I guess we will push through; however, I believe this sort of situation should be avoided.

llvm/lib/Object/XCOFFObjectFile.cpp
476 ↗(On Diff #207142)

Just practising my theoretical-overflow avoidance:

assert(Obj->Data.getBufferSize() >= Offset);
if (Obj->Data.getBufferSize() - Offset < 4)

Or use a helper function:

if (auto EC = Binary::checkOffset(Obj->Data, Offset, 4))
488 ↗(On Diff #207142)

Use getObject.

492 ↗(On Diff #207142)

s/StingTablePtr/StringTablePtr/g;

llvm/lib/Object/XCOFFObjectFile.cpp
507 ↗(On Diff #207142)

const auto *Base.

508 ↗(On Diff #207142)

MemoryBufferRef Data.

522 ↗(On Diff #207142)

s/Pase/Parse/;

Suggestion:
Parse the section header table if it is present.

564 ↗(On Diff #207142)

The XCOFFObjectFile constructor will assert as appropriate without this assert here.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
154 ↗(On Diff #207142)

STYP_OVRFLO/_OVERFLOW_MARKER handling TODO here.

160 ↗(On Diff #207142)

Please test STYP_DWARF. It looks like the handling of DWARF section subtypes is missing. At the least, Sec.Flags & 0xffU might help.

164 ↗(On Diff #207142)

These should be report_fatal_error.

llvm/tools/obj2yaml/xcoff2yaml.cpp
44 ↗(On Diff #207142)

s/suported/supported/;

sfertile updated this revision to Diff 208078.Jul 4 2019, 1:47 PM
  • Lots of tweaks to comments and whitespace.
    • Added assertion to 'getPointerToSymbolTable`
    • Changed name of local variable in getSymbolSectionName to be more descriptive.
    • Use checkOffset and getObject'in parseStringTable.
    • Changed unreachables to report_fatal_error in 'printSectionHeaders`.
    • Added TODO where overflow checking eventually needs to be done.
    • Added comment about DWARF subsection types, & limited section flag printing to the section type flags.
sfertile marked 22 inline comments as done.Jul 4 2019, 1:53 PM
sfertile added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
162 ↗(On Diff #207142)

Updated with suggested comment.

llvm/lib/Object/XCOFFObjectFile.cpp
386 ↗(On Diff #207142)

No idea :S. Updated to a more appropriate name.

llvm/lib/Object/XCOFFObjectFile.cpp
477 ↗(On Diff #208078)

I would much prefer to perform the addition before the cast.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
155 ↗(On Diff #208078)

s/STYP_OVRFLO/_OVERFLOW_MARKER/;

There's no _OVERFLOW_MARKER in the code currently, so we might want to add an appropriate named constant.

sfertile updated this revision to Diff 208179.Jul 5 2019, 7:29 AM
sfertile marked 2 inline comments as done.
  • Perform address arithmetic on const char* before casting to a uintptr_t.
    • Fixup comment. NumberOfX == STYP_OVRFLO doesn't make sense as STYP_OVRFLO is a section type. _OVERFLOW_MARKER is the name of the constant representing the overflow marker value.
sfertile marked 4 inline comments as done.Jul 5 2019, 7:33 AM
sfertile added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
477 ↗(On Diff #208078)

Good catch, updated.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
155 ↗(On Diff #208078)

Updated the comment.

I didn't add the named constant yet because its not used in the code anywhere and I though expressing how there are 32-bit and 64-bit values for _OVERFLOW_MARKER with the 64-bit value (IIUC) not actually representing overflow difficult to do without the overflow handling code.

This revision is now accepted and ready to land.Jul 5 2019, 7:54 AM
This revision was automatically updated to reflect the committed changes.
sfertile marked 2 inline comments as done.