[PATCH] D89071: [SystemZ/z/OS] Add GOFFObjectFile class and details of GOFF file format
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 11 02:14:49 PST 2020
jhenderson added a comment.
Sorry for the delay. This isn't high on my priorities, and I don't have the knowledge to properly review the file format details.
You will need some form of testing, possibly unit testing before this is ready to be put in. Also, please make sure to run clang-format on all new code, so that it conforms to LLVM style guidelines.
================
Comment at: llvm/include/llvm/BinaryFormat/GOFF.h:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
----------------
This doesn't look to me to be the current license header. Please update to match the current version.
================
Comment at: llvm/include/llvm/BinaryFormat/GOFF.h:40
+// Prefix byte on every record. This indicates GOFF format.
+enum { PTVPrefix = 0x03 };
+
----------------
If this is supposed to be a byte, shouldn't it just be `constexpr uint8_t PTVPrefix = 0x03`?
================
Comment at: llvm/include/llvm/Object/GOFF.h:3
+//
+// The LLVM Compiler Infrastructure
+//
----------------
This file is also using an outdated license.
================
Comment at: llvm/include/llvm/Object/GOFF.h:105
+
+ // Set byte of record
+ template <class T> void set(uint8_t ByteIndex, T Value) {
----------------
================
Comment at: llvm/include/llvm/Object/GOFF.h:163
+
+ for (int I = 0; I < Length; ++I)
+ set<uint8_t>(24 + I, Data[I]);
----------------
================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:3
+//
+// The LLVM Compiler Infrastructure
+//
----------------
Update license.
================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:34
+class GOFFObjectFile : public ObjectFile {
+
+ IndexedMap<const uint8_t *> EsdPtrs; // Indexed by EsdId.
----------------
Delete redundant blank line.
================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:51
+
+ // GOFF specific.
+public:
----------------
Isn't all of this "GOFF specific"? The class is `GOFFObjectFile`...
================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:53
+public:
+ std::error_code getSymbolName(SymbolRef Symbol, StringRef &Res) const;
+
----------------
Why is this returning a `std::error_code` not an `Error`?
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:47
+ if ((Object.getBufferSize() % GOFF::RecordLength) != 0) {
+ Err = errorCodeToError(object_error::unexpected_eof);
+ return;
----------------
As stated before, avoid using `error_code` if possible. That includes functions like `errorCodeToError` which just convert from it. Instead, prefer functions like `createStringError`, which allow you to give more contextual error information to the user.
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:82
+ // case (1): (ED,child PR)
+ // - where the PR must be have non-zero length.
+ // case (2a) (ED,0)
----------------
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:86-87
+ // case (2b) (ED,0)
+ // - where the ED is zero length but
+ // but contains a label (LD)
+ GOFF::ESDSymbolType SymbolType;
----------------
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:116-117
+ if (!EdLength) { // [ EDID, PRID ]
+ // LD child of a zero length parent ED
+ // Add the section ED which was previously ignored
+ Section.d.a = SymEdId;
----------------
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:131
+ case GOFF::RT_RLD:
+ // Save RLD records
+ RldPtrs.emplace_back(I);
----------------
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:136
+ case GOFF::RT_LEN:
+ LLVM_DEBUG(dbgs() << " -- LEN (GOFF record type) unhandled\n");
+ break;
----------------
This sounds like it should be an error?
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:152
+
+// SymbolRef
+const uint8_t *GOFFObjectFile::getSymbolEsdRecord(DataRefImpl Symb) const {
----------------
These sorts of comments don't add anything, in my opinion. Just delete them (the function names describe things sufficiently).
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:157
+}
+Expected<StringRef> GOFFObjectFile::getSymbolName(DataRefImpl Symb) const {
+ if (EsdNames.count(Symb.d.a))
----------------
Add a blank line between functions.
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:161
+
+ const char *Record = (const char *)getSymbolEsdRecord(Symb);
+
----------------
Why convert between the two when you could just use `uint8_t` throughout?
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:164
+ // TODO: This could probably be changed to extract the length of the symbol
+ // name and then grab only that many characters but for now this works fine
+ uint16_t Continuations = 0;
----------------
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:165
+ // name and then grab only that many characters but for now this works fine
+ uint16_t Continuations = 0;
+ while (true) {
----------------
What limits this to being specifically `uint16_t` in size?
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:167
+ while (true) {
+ // Is the record continued in the next record
+ const char *ContinuationByte =
----------------
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:170
+ Record + (Continuations * GOFF::RecordLength) + 1;
+ bool IsContinued = *ContinuationByte & 0x01;
+
----------------
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:180-181
+
+ // This assumes that we always emit 80 byte records even if the rest of the
+ // data in a record is nulls
+ for (uint16_t I = 0; I < Continuations; ++I) {
----------------
What happens if the data is truncated?
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:262
+ Expected<StringRef> Name = getSymbolName(Symb);
+ if (Name && !Name->equals(" ")) { // blank name is local
+ Flags |= SymbolRef::SF_Global;
----------------
Do you mean specifically " " means local? What about "", " " (i.e. 0, 2 spaces) etc?
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:282-287
+ assert((SymbolType == GOFF::ESD_ST_SectionDefinition ||
+ SymbolType == GOFF::ESD_ST_ElementDefinition ||
+ SymbolType == GOFF::ESD_ST_LabelDefinition ||
+ SymbolType == GOFF::ESD_ST_PartReference ||
+ SymbolType == GOFF::ESD_ST_ExternalReference) &&
+ "SymbolType must be SectionDef/ElemDef/LabelDef/PartRef/ExtRef");
----------------
Could this assertion fire if somebody wrote garbage in their object file's symbol type field? If so, it should be an error, not an assertion. (Use Error/Expected for malformed input and assertions for coder errors within LLVM).
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:296-299
+ assert((Executable == GOFF::ESD_EXE_CODE ||
+ Executable == GOFF::ESD_EXE_DATA ||
+ Executable == GOFF::ESD_EXE_Unspecified) &&
+ "Executable must be CODE/DATA/Unspecified");
----------------
Same question as above - should this be an error in case of malformed input?
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:338
+ }
+ llvm_unreachable("unable to get symbol section");
+ return section_iterator(SectionRef(Sec, this));
----------------
Is this really unreachable? What happens if there is no symbol section in the file?
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:356
+}
+const uint8_t *
+GOFFObjectFile::getSectionEdEsdRecord(uint32_t SectionIndex) const {
----------------
Blank line between functions. Same goes below.
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:376
+
+ // Calculate total length of relocation items from all records
+ uint32_t RelocationDataSize = 0;
----------------
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:379-380
+ uint16_t RldLengthField;
+ for (uint32_t I = 0; I < RldPtrs.size(); I++) {
+ const uint8_t *RldRecord = RldPtrs[I];
+ RLDRecord::getLength(RldRecord, RldLengthField);
----------------
Use range-based for loop here and below.
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:387
+
+ // Populate RelocationData with relocation items from all records
+ for (uint32_t I = 0; I < RldPtrs.size(); I++) {
----------------
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:396
+ : (GOFF::RecordLength - 6);
+ const char *ChrPtr = reinterpret_cast<const char *>(RldRecord);
+ RelocationData.append(ChrPtr + 6, AppendLength); // copy from initial record
----------------
Same comment as earlier. Why not stick to `uint8_t` everywhere?
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:397
+ const char *ChrPtr = reinterpret_cast<const char *>(RldRecord);
+ RelocationData.append(ChrPtr + 6, AppendLength); // copy from initial record
+ Remainder -= AppendLength;
----------------
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:404
+ : (GOFF::RecordLength - 3);
+ RelocationData.append(ChrPtr + 3, AppendLength); // copy from continuation
+ Remainder -= AppendLength;
----------------
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:416
+}
+GOFF::RLDAction GOFFObjectFile::getRLDAction(uint64_t RelocationType) {
+ return (GOFF::RLDAction)((RelocationType & 0x000000FE) >> 1);
----------------
Add blank line between functions here and below.
================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:452
+ ESDRecord::getSymbolType(EsdRecord, SymbolType);
+ // Skip EDs - i.e. section symbols
+ bool IgnoreSpecialGOFFSymbols = true;
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89071/new/
https://reviews.llvm.org/D89071
More information about the llvm-commits
mailing list