From 033230f756215ac99df28d82dca7dd351b08157f Mon Sep 17 00:00:00 2001 From: Kai Nacke Date: Fri, 26 Apr 2024 16:32:36 -0400 Subject: [PATCH 1/4] [obj2yaml] Add ability to dump GOFF header records Add support for the GOFF file format to obj2yaml, beginning with dumping header records. The GOFF file format does not fit well into the ELF model, so an iterator over the logical records and a data extractor is used to retrieve the information. --- llvm/include/llvm/Object/GOFFObjectFile.h | 75 ++++++++- llvm/include/llvm/ObjectYAML/GOFFYAML.h | 4 +- llvm/lib/Object/GOFFObjectFile.cpp | 148 ++++++++++++------ llvm/lib/Object/ObjectFile.cpp | 3 +- llvm/lib/ObjectYAML/GOFFEmitter.cpp | 3 +- llvm/test/ObjectYAML/GOFF/header-default.yaml | 13 ++ llvm/test/ObjectYAML/GOFF/header-filled.yaml | 21 +++ llvm/tools/obj2yaml/CMakeLists.txt | 1 + llvm/tools/obj2yaml/goff2yaml.cpp | 148 ++++++++++++++++++ llvm/tools/obj2yaml/obj2yaml.cpp | 3 + llvm/tools/obj2yaml/obj2yaml.h | 3 + 11 files changed, 366 insertions(+), 56 deletions(-) create mode 100644 llvm/test/ObjectYAML/GOFF/header-default.yaml create mode 100644 llvm/test/ObjectYAML/GOFF/header-filled.yaml create mode 100644 llvm/tools/obj2yaml/goff2yaml.cpp diff --git a/llvm/include/llvm/Object/GOFFObjectFile.h b/llvm/include/llvm/Object/GOFFObjectFile.h index 6871641e97ec8..1d05bad9e83dd 100644 --- a/llvm/include/llvm/Object/GOFFObjectFile.h +++ b/llvm/include/llvm/Object/GOFFObjectFile.h @@ -20,6 +20,7 @@ #include "llvm/Object/ObjectFile.h" #include "llvm/Support/ConvertEBCDIC.h" #include "llvm/Support/Debug.h" +#include "llvm/Support/Error.h" #include "llvm/Support/raw_ostream.h" #include "llvm/TargetParser/SubtargetFeature.h" #include "llvm/TargetParser/Triple.h" @@ -28,6 +29,66 @@ namespace llvm { namespace object { +// The GOFF file format consists of 2 layers. The data is organized in logical +// records, which are divided into physical records. A RecordRef object +// represents a logical record, with the data still divided in physical record. +class RecordRef { + const ObjectFile *OwningObject = nullptr; + const uint8_t *Data = nullptr; + uint64_t Size = 0U; + Error *Err = nullptr; + + const uint8_t *base() const { + return reinterpret_cast(OwningObject->getData().data()); + } + + // Create a string error, and clear the position. + void createError(std::error_code EC, const Twine &S) { + if (Err) + *Err = createStringError(EC, S); + Data = nullptr; + Size = 0; + } + + // Determine the size of the logical record. Also makes sure that the physical + // records are correctly linked. + void determineSize(); + +public: + RecordRef() = default; + RecordRef(const ObjectFile *Owner) : OwningObject(Owner) {} + RecordRef(const ObjectFile *Owner, Error *Err) + : OwningObject(Owner), Data(base()), Err(Err) { + determineSize(); + }; + RecordRef(const RecordRef &Other) { + OwningObject = Other.OwningObject; + Data = Other.Data; + Size = Other.Size; + Err = Other.Err; + }; + + bool operator==(const RecordRef &Other) const; + void moveNext(); + + /// Returns the type of the record. + llvm::GOFF::RecordType getRecordType() const; + + /// Returns the size of the logical record. This is a multiply of the size of + /// a physical record. + uint64_t getSize() const; + + /// Data of the logical record, still divided in physical records. + const ArrayRef getContents() const; +}; + +inline bool RecordRef::operator==(const RecordRef &Other) const { + return OwningObject == Other.OwningObject && Data == Other.Data && + Size == Other.Size; +} + +using record_iterator = content_iterator; + class GOFFObjectFile : public ObjectFile { friend class GOFFSymbolRef; @@ -44,6 +105,12 @@ class GOFFObjectFile : public ObjectFile { mutable DenseMap> SectionDataCache; public: + record_iterator record_begin(Error *Err) const; + record_iterator record_end() const; + iterator_range records(Error *Err) const { + return make_range(record_begin(Err), record_end()); + } + Expected getSymbolName(SymbolRef Symbol) const; GOFFObjectFile(MemoryBufferRef Object, Error &Err); @@ -57,7 +124,9 @@ class GOFFObjectFile : public ObjectFile { Triple::ArchType getArch() const override { return Triple::systemz; } - Expected getFeatures() const override { return SubtargetFeatures(); } + Expected getFeatures() const override { + return SubtargetFeatures(); + } bool isRelocatableObject() const override { return true; } @@ -65,9 +134,7 @@ class GOFFObjectFile : public ObjectFile { basic_symbol_iterator symbol_begin() const override; basic_symbol_iterator symbol_end() const override; - bool is64Bit() const override { - return true; - } + bool is64Bit() const override { return true; } bool isSectionNoLoad(DataRefImpl Sec) const; bool isSectionReadOnlyData(DataRefImpl Sec) const; diff --git a/llvm/include/llvm/ObjectYAML/GOFFYAML.h b/llvm/include/llvm/ObjectYAML/GOFFYAML.h index f9bf45e95bd3a..7715053e19ff9 100644 --- a/llvm/include/llvm/ObjectYAML/GOFFYAML.h +++ b/llvm/include/llvm/ObjectYAML/GOFFYAML.h @@ -29,8 +29,8 @@ struct FileHeader { uint32_t TargetEnvironment = 0; uint32_t TargetOperatingSystem = 0; uint16_t CCSID = 0; - StringRef CharacterSetName; - StringRef LanguageProductIdentifier; + std::string CharacterSetName; + std::string LanguageProductIdentifier; uint32_t ArchitectureLevel = 0; std::optional InternalCCSID; std::optional TargetSoftwareEnvironment; diff --git a/llvm/lib/Object/GOFFObjectFile.cpp b/llvm/lib/Object/GOFFObjectFile.cpp index 3b8704f28fdbb..20be66c758fb9 100644 --- a/llvm/lib/Object/GOFFObjectFile.cpp +++ b/llvm/lib/Object/GOFFObjectFile.cpp @@ -16,6 +16,7 @@ #include "llvm/Support/Debug.h" #include "llvm/Support/Errc.h" #include "llvm/Support/raw_ostream.h" +#include #ifndef DEBUG_TYPE #define DEBUG_TYPE "goff" @@ -24,6 +25,94 @@ using namespace llvm::object; using namespace llvm; +namespace { +// Return the type of the record. +GOFF::RecordType getRecordType(const uint8_t *PhysicalRecord) { + return GOFF::RecordType((PhysicalRecord[1] & 0xF0) >> 4); +} + +// Return true if the record is a continuation record. +bool isContinuation(const uint8_t *PhysicalRecord) { + return PhysicalRecord[1] & 0x02; +} + +// Return true if the record has a continuation. +bool isContinued(const uint8_t *PhysicalRecord) { + return PhysicalRecord[1] & 0x01; +} +} // namespace + +void RecordRef::determineSize() { + GOFF::RecordType CurrRecordType = ::getRecordType(Data); + bool PrevWasContinued = isContinued(Data); + const uint8_t *It = Data + GOFF::RecordLength; + const uint8_t *End = reinterpret_cast( + base() + OwningObject->getData().size()); + for (; It < End; + PrevWasContinued = isContinued(It), It += GOFF::RecordLength) { + GOFF::RecordType RecordType = ::getRecordType(It); + bool IsContinuation = isContinuation(It); + size_t RecordNum = (It - base()) / GOFF::RecordLength; + // If the previous record was continued, the current record should be a + // continuation. + if (PrevWasContinued && !IsContinuation) { + createError(object_error::parse_failed, + "record " + std::to_string(RecordNum) + + " is not a continuation record but the " + "preceding record is continued"); + return; + } + // If the current record is a continuation, then the previous record should + // be continued, and have the same record type. + if (IsContinuation) { + if (RecordType != CurrRecordType) { + createError(object_error::parse_failed, + "record " + std::to_string(RecordNum) + + " is a continuation record that does not " + "match the type of the previous record"); + return; + } + if (!PrevWasContinued) { + createError(object_error::parse_failed, + "record " + std::to_string(RecordNum) + + " is a continuation record that is not " + "preceded by a continued record"); + return; + } + } + + // Break out of loop when we reached a new record. + if (!IsContinuation) + break; + } + Size = It - Data; +} + +void RecordRef::moveNext() { + if (Data == nullptr) + return; + const uint8_t *Base = base(); + std::ptrdiff_t Offset = Data - Base; + uint64_t NewOffset = Offset + Size; + if (NewOffset > OwningObject->getData().size()) { + Data = nullptr; + Size = 0; + } else { + Data = &Base[NewOffset]; + determineSize(); + } +} + +GOFF::RecordType RecordRef::getRecordType() const { + return ::getRecordType(Data); +} + +uint64_t RecordRef::getSize() const { return Size; } + +const ArrayRef RecordRef::getContents() const { + return ArrayRef(Data, Size); +} + Expected> ObjectFile::createGOFFObjectFile(MemoryBufferRef Object) { Error Err = Error::success(); @@ -64,52 +153,9 @@ GOFFObjectFile::GOFFObjectFile(MemoryBufferRef Object, Error &Err) SectionEntryImpl DummySection; SectionList.emplace_back(DummySection); // Dummy entry at index 0. - uint8_t PrevRecordType = 0; - uint8_t PrevContinuationBits = 0; - const uint8_t *End = reinterpret_cast(Data.getBufferEnd()); - for (const uint8_t *I = base(); I < End; I += GOFF::RecordLength) { - uint8_t RecordType = (I[1] & 0xF0) >> 4; - bool IsContinuation = I[1] & 0x02; - bool PrevWasContinued = PrevContinuationBits & 0x01; - size_t RecordNum = (I - base()) / GOFF::RecordLength; - - // If the previous record was continued, the current record should be a - // continuation. - if (PrevWasContinued && !IsContinuation) { - if (PrevRecordType == RecordType) { - Err = createStringError(object_error::parse_failed, - "record " + std::to_string(RecordNum) + - " is not a continuation record but the " - "preceding record is continued"); - return; - } - } - // Don't parse continuations records, only parse initial record. - if (IsContinuation) { - if (RecordType != PrevRecordType) { - Err = createStringError(object_error::parse_failed, - "record " + std::to_string(RecordNum) + - " is a continuation record that does not " - "match the type of the previous record"); - return; - } - if (!PrevWasContinued) { - Err = createStringError(object_error::parse_failed, - "record " + std::to_string(RecordNum) + - " is a continuation record that is not " - "preceded by a continued record"); - return; - } - PrevRecordType = RecordType; - PrevContinuationBits = I[1] & 0x03; - continue; - } - LLVM_DEBUG(for (size_t J = 0; J < GOFF::RecordLength; ++J) { - const uint8_t *P = I + J; - if (J % 8 == 0) - dbgs() << " "; - dbgs() << format("%02hhX", *P); - }); + for (auto &Rec : records(&Err)) { + uint8_t RecordType = Rec.getRecordType(); + const uint8_t *I = Rec.getContents().data(); switch (RecordType) { case GOFF::RT_ESD: { @@ -179,11 +225,17 @@ GOFFObjectFile::GOFFObjectFile(MemoryBufferRef Object, Error &Err) default: llvm_unreachable("Unknown record type"); } - PrevRecordType = RecordType; - PrevContinuationBits = I[1] & 0x03; } } +record_iterator GOFFObjectFile::record_begin(Error *Err) const { + return record_iterator(RecordRef(this, Err)); +} + +record_iterator GOFFObjectFile::record_end() const { + return record_iterator(RecordRef(this)); +} + const uint8_t *GOFFObjectFile::getSymbolEsdRecord(DataRefImpl Symb) const { const uint8_t *EsdRecord = EsdPtrs[Symb.d.a]; return EsdRecord; diff --git a/llvm/lib/Object/ObjectFile.cpp b/llvm/lib/Object/ObjectFile.cpp index 6a226a3bbdbca..831d3abed0954 100644 --- a/llvm/lib/Object/ObjectFile.cpp +++ b/llvm/lib/Object/ObjectFile.cpp @@ -162,7 +162,6 @@ ObjectFile::createObjectFile(MemoryBufferRef Object, file_magic Type, case file_magic::windows_resource: case file_magic::pdb: case file_magic::minidump: - case file_magic::goff_object: case file_magic::cuda_fatbinary: case file_magic::offload_binary: case file_magic::dxcontainer_object: @@ -178,6 +177,8 @@ ObjectFile::createObjectFile(MemoryBufferRef Object, file_magic Type, case file_magic::elf_shared_object: case file_magic::elf_core: return createELFObjectFile(Object, InitContent); + case file_magic::goff_object: + return createGOFFObjectFile(Object); case file_magic::macho_object: case file_magic::macho_executable: case file_magic::macho_fixed_virtual_memory_shared_lib: diff --git a/llvm/lib/ObjectYAML/GOFFEmitter.cpp b/llvm/lib/ObjectYAML/GOFFEmitter.cpp index 345904407e1d2..47550ac971980 100644 --- a/llvm/lib/ObjectYAML/GOFFEmitter.cpp +++ b/llvm/lib/ObjectYAML/GOFFEmitter.cpp @@ -219,7 +219,8 @@ void GOFFState::writeHeader(GOFFYAML::FileHeader &FileHdr) { } GW.makeNewRecord(GOFF::RT_HDR, GOFF::PayloadLength); - GW << binaryBe(FileHdr.TargetEnvironment) // TargetEnvironment + GW << zeros(1) // Reserved + << binaryBe(FileHdr.TargetEnvironment) // TargetEnvironment << binaryBe(FileHdr.TargetOperatingSystem) // TargetOperatingSystem << zeros(2) // Reserved << binaryBe(FileHdr.CCSID) // CCSID diff --git a/llvm/test/ObjectYAML/GOFF/header-default.yaml b/llvm/test/ObjectYAML/GOFF/header-default.yaml new file mode 100644 index 0000000000000..ed85aeb67840b --- /dev/null +++ b/llvm/test/ObjectYAML/GOFF/header-default.yaml @@ -0,0 +1,13 @@ +# RUN: yaml2obj %s | obj2yaml | FileCheck %s + +# CHECK: FileHeader: {} + +--- !GOFF +FileHeader: + TargetEnvironment: 0 + TargetOperatingSystem: 0 + CCSID: 0 + CharacterSetName: "\0" + LanguageProductIdentifier: "\0" + ArchitectureLevel: 1 +... diff --git a/llvm/test/ObjectYAML/GOFF/header-filled.yaml b/llvm/test/ObjectYAML/GOFF/header-filled.yaml new file mode 100644 index 0000000000000..090b8e1c1b5cc --- /dev/null +++ b/llvm/test/ObjectYAML/GOFF/header-filled.yaml @@ -0,0 +1,21 @@ +# RUN: yaml2obj %s | obj2yaml | FileCheck %s + +# CHECK: --- !GOFF +# CHECK-NEXT: FileHeader: +# CHECK-NEXT: TargetEnvironment: 1 +# CHECK-NEXT: TargetOperatingSystem: 2 +# CHECK-NEXT: CCSID: 1047 +# CHECK-NEXT: CharacterSetName: EBCDIC-1047 +# CHECK-NEXT: LanguageProductIdentifier: Foo +# CHECK-NEXT: ArchitectureLevel: 2 +# CHECK: ... + +--- !GOFF +FileHeader: + TargetEnvironment: 1 + TargetOperatingSystem: 2 + CCSID: 1047 + CharacterSetName: EBCDIC-1047 + LanguageProductIdentifier: Foo + ArchitectureLevel: 2 +... diff --git a/llvm/tools/obj2yaml/CMakeLists.txt b/llvm/tools/obj2yaml/CMakeLists.txt index ac8ff8c85dd9c..ab0a04e617ca6 100644 --- a/llvm/tools/obj2yaml/CMakeLists.txt +++ b/llvm/tools/obj2yaml/CMakeLists.txt @@ -14,6 +14,7 @@ add_llvm_utility(obj2yaml dwarf2yaml.cpp dxcontainer2yaml.cpp elf2yaml.cpp + goff2yaml.cpp macho2yaml.cpp minidump2yaml.cpp offload2yaml.cpp diff --git a/llvm/tools/obj2yaml/goff2yaml.cpp b/llvm/tools/obj2yaml/goff2yaml.cpp new file mode 100644 index 0000000000000..35bfe993c1de1 --- /dev/null +++ b/llvm/tools/obj2yaml/goff2yaml.cpp @@ -0,0 +1,148 @@ +//===------ goff2yaml.cpp - obj2yaml conversion tool ------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "obj2yaml.h" +#include "llvm/ADT/SmallString.h" +#include "llvm/Object/GOFFObjectFile.h" +#include "llvm/ObjectYAML/ObjectYAML.h" +#include "llvm/Support/ConvertEBCDIC.h" +#include "llvm/Support/DataExtractor.h" +#include "llvm/Support/Error.h" +#include "llvm/Support/ErrorHandling.h" + +using namespace llvm; + +namespace { +std::string getFixedLengthEBCDICString(DataExtractor &Data, + DataExtractor::Cursor &C, + uint64_t Length, + StringRef TrimChars = {"\0", 1}) { + StringRef FixedLenStr = Data.getBytes(C, Length); + SmallString<16> Str; + ConverterEBCDIC::convertToUTF8(FixedLenStr, Str); + return Str.str().trim(TrimChars).str(); +} +} // namespace + +class GOFFDumper { + const object::GOFFObjectFile &Obj; + GOFFYAML::Object YAMLObj; + + Error dumpHeader(ArrayRef Records); + Error dumpExternalSymbol(ArrayRef Records); + Error dumpText(ArrayRef Records); + Error dumpRelocationDirectory(ArrayRef Records); + Error dumpDeferredLength(ArrayRef Records); + Error dumpEnd(ArrayRef Records); + +public: + GOFFDumper(const object::GOFFObjectFile &Obj); + Error dump(); + GOFFYAML::Object &getYAMLObj(); +}; + +GOFFDumper::GOFFDumper(const object::GOFFObjectFile &Obj) : Obj(Obj) {} + +Error GOFFDumper::dumpHeader(ArrayRef Records) { + DataExtractor Data(Records, false, 0); + DataExtractor::Cursor C(4); + YAMLObj.Header.TargetEnvironment = Data.getU32(C); + YAMLObj.Header.TargetOperatingSystem = Data.getU32(C); + Data.skip(C, 2); + YAMLObj.Header.CCSID = Data.getU16(C); + YAMLObj.Header.CharacterSetName = + getFixedLengthEBCDICString(Data, C, 16); + YAMLObj.Header.LanguageProductIdentifier = + getFixedLengthEBCDICString(Data, C, 16); + YAMLObj.Header.ArchitectureLevel = Data.getU32(C); + uint16_t PropertiesLength = Data.getU16(C); + Data.skip(C, 6); + if (PropertiesLength) { + YAMLObj.Header.InternalCCSID = Data.getU16(C); + PropertiesLength -= 2; + } + if (PropertiesLength) { + YAMLObj.Header.TargetSoftwareEnvironment = Data.getU8(C); + PropertiesLength -= 1; + } + return C.takeError(); +} + +Error GOFFDumper::dumpExternalSymbol(ArrayRef Records) { + // TODO Implement. + return Error::success(); +} + +Error GOFFDumper::dumpText(ArrayRef Records) { + // TODO Implement. + return Error::success(); +} + +Error GOFFDumper::dumpRelocationDirectory(ArrayRef Records) { + // TODO Implement. + return Error::success(); +} + +Error GOFFDumper::dumpDeferredLength(ArrayRef Records) { + // TODO Implement. + return Error::success(); +} + +Error GOFFDumper::dumpEnd(ArrayRef Records) { + // TODO Implement. + return Error::success(); +} + +Error GOFFDumper::dump() { + Error Err = Error::success(); + for (auto &Rec : Obj.records(&Err)) { + if (Err) + return Err; + switch (Rec.getRecordType()) { + case GOFF::RT_ESD: + if (auto Err = dumpExternalSymbol(Rec.getContents())) + return Err; + break; + case GOFF::RT_TXT: + if (auto Err = dumpText(Rec.getContents())) + return Err; + break; + case GOFF::RT_RLD: + if (auto Err = dumpRelocationDirectory(Rec.getContents())) + return Err; + break; + case GOFF::RT_LEN: + if (auto Err = dumpDeferredLength(Rec.getContents())) + return Err; + break; + case GOFF::RT_END: + if (auto Err = dumpEnd(Rec.getContents())) + return Err; + break; + case GOFF::RT_HDR: + if (auto Err = dumpHeader(Rec.getContents())) + return Err; + break; + } + } + return Err; +} + +GOFFYAML::Object &GOFFDumper::getYAMLObj() { return YAMLObj; } + +Error goff2yaml(raw_ostream &Out, const llvm::object::GOFFObjectFile &Obj) { + GOFFDumper Dumper(Obj); + + if (auto Err = Dumper.dump()) + return Err; + + yaml::Output Yout(Out); + Yout << Dumper.getYAMLObj(); + + return Error::success(); +} diff --git a/llvm/tools/obj2yaml/obj2yaml.cpp b/llvm/tools/obj2yaml/obj2yaml.cpp index 63c8cc55ee2d4..1b6badeb48faf 100644 --- a/llvm/tools/obj2yaml/obj2yaml.cpp +++ b/llvm/tools/obj2yaml/obj2yaml.cpp @@ -45,6 +45,9 @@ static Error dumpObject(const ObjectFile &Obj, raw_ostream &OS) { if (Obj.isELF()) return elf2yaml(OS, Obj); + if (Obj.isGOFF()) + return goff2yaml(OS, cast(Obj)); + if (Obj.isWasm()) return errorCodeToError(wasm2yaml(OS, cast(Obj))); diff --git a/llvm/tools/obj2yaml/obj2yaml.h b/llvm/tools/obj2yaml/obj2yaml.h index 03d7db5317acd..f2247655f6009 100644 --- a/llvm/tools/obj2yaml/obj2yaml.h +++ b/llvm/tools/obj2yaml/obj2yaml.h @@ -13,6 +13,7 @@ #define LLVM_TOOLS_OBJ2YAML_OBJ2YAML_H #include "llvm/Object/COFF.h" +#include "llvm/Object/GOFFObjectFile.h" #include "llvm/Object/Minidump.h" #include "llvm/Object/Wasm.h" #include "llvm/Object/XCOFFObjectFile.h" @@ -25,6 +26,8 @@ std::error_code coff2yaml(llvm::raw_ostream &Out, const llvm::object::COFFObjectFile &Obj); llvm::Error elf2yaml(llvm::raw_ostream &Out, const llvm::object::ObjectFile &Obj); +llvm::Error goff2yaml(llvm::raw_ostream &Out, + const llvm::object::GOFFObjectFile &Obj); llvm::Error macho2yaml(llvm::raw_ostream &Out, const llvm::object::Binary &Obj, unsigned RawSegments); llvm::Error minidump2yaml(llvm::raw_ostream &Out, From 595d8b9211777bc464a25caf688fd81a021e9e90 Mon Sep 17 00:00:00 2001 From: Kai Nacke Date: Thu, 2 May 2024 12:13:23 -0400 Subject: [PATCH 2/4] Accidently removed an error check. --- llvm/lib/Object/GOFFObjectFile.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/llvm/lib/Object/GOFFObjectFile.cpp b/llvm/lib/Object/GOFFObjectFile.cpp index 20be66c758fb9..4c9cea436342b 100644 --- a/llvm/lib/Object/GOFFObjectFile.cpp +++ b/llvm/lib/Object/GOFFObjectFile.cpp @@ -154,6 +154,8 @@ GOFFObjectFile::GOFFObjectFile(MemoryBufferRef Object, Error &Err) SectionList.emplace_back(DummySection); // Dummy entry at index 0. for (auto &Rec : records(&Err)) { + if (Err) + return; uint8_t RecordType = Rec.getRecordType(); const uint8_t *I = Rec.getContents().data(); From e72feef97b419158bfa5adaf945540493ee07180 Mon Sep 17 00:00:00 2001 From: Kai Nacke Date: Mon, 6 May 2024 12:48:24 -0400 Subject: [PATCH 3/4] Address reviewer comments and more - Use static instead of anonymous namespace for functions - Use Twine instead of string concatenation - Fix clang-format error - Move test cases to test/obj2yaml since they are for the tool --- llvm/lib/Object/GOFFObjectFile.cpp | 29 ++++++++++--------- .../obj2yaml}/GOFF/header-default.yaml | 0 .../obj2yaml}/GOFF/header-filled.yaml | 0 llvm/tools/obj2yaml/goff2yaml.cpp | 13 ++++----- 4 files changed, 20 insertions(+), 22 deletions(-) rename llvm/test/{ObjectYAML => tools/obj2yaml}/GOFF/header-default.yaml (100%) rename llvm/test/{ObjectYAML => tools/obj2yaml}/GOFF/header-filled.yaml (100%) diff --git a/llvm/lib/Object/GOFFObjectFile.cpp b/llvm/lib/Object/GOFFObjectFile.cpp index 4c9cea436342b..77648c4bd6b8b 100644 --- a/llvm/lib/Object/GOFFObjectFile.cpp +++ b/llvm/lib/Object/GOFFObjectFile.cpp @@ -25,22 +25,20 @@ using namespace llvm::object; using namespace llvm; -namespace { // Return the type of the record. -GOFF::RecordType getRecordType(const uint8_t *PhysicalRecord) { +static GOFF::RecordType getRecordType(const uint8_t *PhysicalRecord) { return GOFF::RecordType((PhysicalRecord[1] & 0xF0) >> 4); } // Return true if the record is a continuation record. -bool isContinuation(const uint8_t *PhysicalRecord) { +static bool isContinuation(const uint8_t *PhysicalRecord) { return PhysicalRecord[1] & 0x02; } // Return true if the record has a continuation. -bool isContinued(const uint8_t *PhysicalRecord) { +static bool isContinued(const uint8_t *PhysicalRecord) { return PhysicalRecord[1] & 0x01; } -} // namespace void RecordRef::determineSize() { GOFF::RecordType CurrRecordType = ::getRecordType(Data); @@ -57,9 +55,10 @@ void RecordRef::determineSize() { // continuation. if (PrevWasContinued && !IsContinuation) { createError(object_error::parse_failed, - "record " + std::to_string(RecordNum) + - " is not a continuation record but the " - "preceding record is continued"); + Twine("Record ") + .concat(std::to_string(RecordNum)) + .concat(" is not a continuation record but the " + "preceding record is continued")); return; } // If the current record is a continuation, then the previous record should @@ -67,16 +66,18 @@ void RecordRef::determineSize() { if (IsContinuation) { if (RecordType != CurrRecordType) { createError(object_error::parse_failed, - "record " + std::to_string(RecordNum) + - " is a continuation record that does not " - "match the type of the previous record"); + Twine("Record ") + .concat(std::to_string(RecordNum)) + .concat(" is a continuation record that does not " + "match the type of the previous record")); return; } if (!PrevWasContinued) { createError(object_error::parse_failed, - "record " + std::to_string(RecordNum) + - " is a continuation record that is not " - "preceded by a continued record"); + Twine("Record ") + .concat(std::to_string(RecordNum)) + .concat(" is a continuation record that is not " + "preceded by a continued record")); return; } } diff --git a/llvm/test/ObjectYAML/GOFF/header-default.yaml b/llvm/test/tools/obj2yaml/GOFF/header-default.yaml similarity index 100% rename from llvm/test/ObjectYAML/GOFF/header-default.yaml rename to llvm/test/tools/obj2yaml/GOFF/header-default.yaml diff --git a/llvm/test/ObjectYAML/GOFF/header-filled.yaml b/llvm/test/tools/obj2yaml/GOFF/header-filled.yaml similarity index 100% rename from llvm/test/ObjectYAML/GOFF/header-filled.yaml rename to llvm/test/tools/obj2yaml/GOFF/header-filled.yaml diff --git a/llvm/tools/obj2yaml/goff2yaml.cpp b/llvm/tools/obj2yaml/goff2yaml.cpp index 35bfe993c1de1..61bbd8301e446 100644 --- a/llvm/tools/obj2yaml/goff2yaml.cpp +++ b/llvm/tools/obj2yaml/goff2yaml.cpp @@ -17,17 +17,15 @@ using namespace llvm; -namespace { -std::string getFixedLengthEBCDICString(DataExtractor &Data, - DataExtractor::Cursor &C, - uint64_t Length, - StringRef TrimChars = {"\0", 1}) { +static std::string getFixedLengthEBCDICString(DataExtractor &Data, + DataExtractor::Cursor &C, + uint64_t Length, + StringRef TrimChars = {"\0", 1}) { StringRef FixedLenStr = Data.getBytes(C, Length); SmallString<16> Str; ConverterEBCDIC::convertToUTF8(FixedLenStr, Str); return Str.str().trim(TrimChars).str(); } -} // namespace class GOFFDumper { const object::GOFFObjectFile &Obj; @@ -55,8 +53,7 @@ Error GOFFDumper::dumpHeader(ArrayRef Records) { YAMLObj.Header.TargetOperatingSystem = Data.getU32(C); Data.skip(C, 2); YAMLObj.Header.CCSID = Data.getU16(C); - YAMLObj.Header.CharacterSetName = - getFixedLengthEBCDICString(Data, C, 16); + YAMLObj.Header.CharacterSetName = getFixedLengthEBCDICString(Data, C, 16); YAMLObj.Header.LanguageProductIdentifier = getFixedLengthEBCDICString(Data, C, 16); YAMLObj.Header.ArchitectureLevel = Data.getU32(C); From daa36cbb0db266d5552fc8086994df42aca7f552 Mon Sep 17 00:00:00 2001 From: Kai Nacke Date: Mon, 6 May 2024 15:01:12 -0400 Subject: [PATCH 4/4] Replace std::to_string with Twine --- llvm/lib/Object/GOFFObjectFile.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/llvm/lib/Object/GOFFObjectFile.cpp b/llvm/lib/Object/GOFFObjectFile.cpp index 77648c4bd6b8b..404bd963caefb 100644 --- a/llvm/lib/Object/GOFFObjectFile.cpp +++ b/llvm/lib/Object/GOFFObjectFile.cpp @@ -56,7 +56,7 @@ void RecordRef::determineSize() { if (PrevWasContinued && !IsContinuation) { createError(object_error::parse_failed, Twine("Record ") - .concat(std::to_string(RecordNum)) + .concat(Twine(RecordNum)) .concat(" is not a continuation record but the " "preceding record is continued")); return; @@ -67,7 +67,7 @@ void RecordRef::determineSize() { if (RecordType != CurrRecordType) { createError(object_error::parse_failed, Twine("Record ") - .concat(std::to_string(RecordNum)) + .concat(Twine(RecordNum)) .concat(" is a continuation record that does not " "match the type of the previous record")); return; @@ -75,7 +75,7 @@ void RecordRef::determineSize() { if (!PrevWasContinued) { createError(object_error::parse_failed, Twine("Record ") - .concat(std::to_string(RecordNum)) + .concat(Twine(RecordNum)) .concat(" is a continuation record that is not " "preceded by a continued record")); return;