[PATCH] D60784: [Object][XCOFF] Add intial support for parsing/dumping section header table.
Hubert Tong via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 17 21:30:20 PDT 2019
hubert.reinterpretcast added inline comments.
================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:125
+ Expected<uint64_t> getStartAddress() const override;
}; // XCOFFObjectFile
----------------
In keeping with the ordering of overriden functions in this file, this declaration should occur between `getFeatures` and `isRelocatableObject` as that is the ordering present in the definition of `class ObjectFile`.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:40
+
+
// Sets Obj unless any bytes in [addr, addr + size) fall outsize of m.
----------------
There is an extra blank line here in comparison to the number of blank lines used between the next few functions below.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:54
+template <typename T> const T *viewAs(uintptr_t in) {
+ return reinterpret_cast<const T *>(in);
----------------
This should be `static` or otherwise made to have internal linkage to avoid ODR violations.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:139
+ ? SH->Name
+ : StringRef(SH->Name, XCOFF::SectionNameSize);
return std::error_code();
----------------
Do we want to use the version that avoids embedded NUL characters in this patch?
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:181
+ unsigned Flags = toSection(Sec)->Flags;
+ return (Flags & XCOFF::STYP_DATA) || (Flags & XCOFF::STYP_TDATA);
}
----------------
It seems `(Flags & (XCOFF::STYP_DATA | XCOFF::STYP_TDATA))` is a more common pattern in the codebase.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:186
+ unsigned Flags = toSection(Sec)->Flags;
+ return (Flags & XCOFF::STYP_BSS) || (Flags & XCOFF::STYP_TBSS);
}
----------------
Same comment as above.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:310
+ if (getNumberOfSections() != 0) {
+ if (!checkSize(Data, EC, CurPtr,
+ getNumberOfSections() * getSectionHeaderSize()))
----------------
`getObject` (called below) takes a `Size` parameter. It can be used to check for the right number of sections without this call to `checkSize`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60784/new/
https://reviews.llvm.org/D60784
More information about the llvm-commits
mailing list