[llvm-objcopy] Support SREC output format #75874
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-llvm-binary-utilities Author: None (quic-areg) ChangesAdds a new output target "srec" to write SREC files from ELF inputs. https://en.wikipedia.org/wiki/SREC_(file_format) Patch is 20.88 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/75874.diff 10 Files Affected:
diff --git a/llvm/include/llvm/ObjCopy/CommonConfig.h b/llvm/include/llvm/ObjCopy/CommonConfig.h
index 386c20aec184de..893308eff3a42c 100644
--- a/llvm/include/llvm/ObjCopy/CommonConfig.h
+++ b/llvm/include/llvm/ObjCopy/CommonConfig.h
@@ -27,12 +27,7 @@
namespace llvm {
namespace objcopy {
-enum class FileFormat {
- Unspecified,
- ELF,
- Binary,
- IHex,
-};
+enum class FileFormat { Unspecified, ELF, Binary, IHex, SREC };
// This type keeps track of the machine info for various architectures. This
// lets us map architecture names to ELF types and the e_machine value of the
diff --git a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
index daf03810fd7bff..772c19610d8c4e 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
@@ -183,6 +183,8 @@ static std::unique_ptr<Writer> createWriter(const CommonConfig &Config,
return std::make_unique<BinaryWriter>(Obj, Out, Config);
case FileFormat::IHex:
return std::make_unique<IHexWriter>(Obj, Out);
+ case FileFormat::SREC:
+ return std::make_unique<SRECWriter>(Obj, Out, Config.OutputFilename);
default:
return createELFWriter(Config, Obj, Out, OutputElfType);
}
diff --git a/llvm/lib/ObjCopy/ELF/ELFObject.cpp b/llvm/lib/ObjCopy/ELF/ELFObject.cpp
index 5352736bdcb9b8..5868168700a16a 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObject.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObject.cpp
@@ -324,6 +324,14 @@ Expected<IHexRecord> IHexRecord::parse(StringRef Line) {
return Rec;
}
+static uint64_t sectionLMA(const SectionBase *Sec) {
+ Segment *Seg = Sec->ParentSegment;
+ if (Seg && Seg->Type == PT_LOAD && Seg->VAddr <= Sec->Addr &&
+ (Seg->VAddr + Seg->MemSize > Sec->Addr))
+ return Sec->Addr - Seg->VAddr + Seg->PAddr;
+ return Sec->Addr;
+}
+
static uint64_t sectionPhysicalAddr(const SectionBase *Sec) {
Segment *Seg = Sec->ParentSegment;
if (Seg && Seg->Type != ELF::PT_LOAD)
@@ -2813,6 +2821,235 @@ Error IHexWriter::finalize() {
return Error::success();
}
+Error SRECSectionWriterBase::visit(const StringTableSection &Sec) {
+ // Check that sizer has already done its work
+ assert(Sec.Size == Sec.StrTabBuilder.getSize());
+ // we don't need to write anything here the real writer has already done it.
+ return Error::success();
+}
+
+Error SRECSectionWriterBase::visit(const Section &Sec) {
+ writeSection(Sec, Sec.Contents);
+ return Error::success();
+}
+
+Error SRECSectionWriterBase::visit(const OwnedDataSection &Sec) {
+ writeSection(Sec, Sec.Data);
+ return Error::success();
+}
+
+Error SRECSectionWriterBase::visit(const DynamicRelocationSection &Sec) {
+ writeSection(Sec, Sec.Contents);
+ return Error::success();
+}
+
+void SRECSectionWriter::writeRecord(SRecord &Record, uint64_t Off) {
+ SRecLineData Data = Record.toString();
+ memcpy(Out.getBufferStart() + Off, Data.data(), Data.size());
+}
+
+void SRECSectionWriter::writeRecords() {
+ uint64_t Off = HeaderSize;
+ for (SRecord &Record : Records) {
+ Record.Type = Type;
+ writeRecord(Record, Off);
+ Off += Record.getSize();
+ }
+ Offset = Off;
+}
+
+void SRECSectionWriterBase::writeRecords() {
+ uint64_t Off = HeaderSize;
+ for (SRecord &Record : Records) {
+ Record.Type = Type;
+ Off += Record.getSize();
+ }
+ Offset = Off;
+}
+
+void SRECSectionWriterBase::writeSection(const SectionBase &S,
+ ArrayRef<uint8_t> Data) {
+ const uint32_t ChunkSize = 16;
+ uint32_t Address = sectionLMA(&S);
+ uint32_t EndAddr = Address + S.Size - 1;
+ if (isUInt<16>(EndAddr))
+ Type = std::max(static_cast<uint8_t>(SRecord::S1), Type);
+ else if (isUInt<24>(EndAddr))
+ Type = std::max(static_cast<uint8_t>(SRecord::S2), Type);
+ else
+ Type = SRecord::S3;
+ while (!Data.empty()) {
+ uint64_t DataSize = std::min<uint64_t>(Data.size(), ChunkSize);
+ SRecord Record{Type, Address, Data.take_front(DataSize)};
+ Records.push_back(Record);
+ Data = Data.drop_front(DataSize);
+ Address += DataSize;
+ }
+}
+
+Error SRECSectionWriter::visit(const StringTableSection &Sec) {
+ assert(Sec.Size == Sec.StrTabBuilder.getSize());
+ std::vector<uint8_t> Data(Sec.Size);
+ Sec.StrTabBuilder.write(Data.data());
+ writeSection(Sec, Data);
+ return Error::success();
+}
+
+SRecLineData SRecord::toString() const {
+ SRecLineData Line(getSize());
+ auto *Iter = Line.begin();
+ *Iter++ = 'S';
+ *Iter++ = '0' + Type;
+ // write 1 byte (2 hex characters) record count
+ Iter = toHexStr(getCount(), Iter, 2);
+ // write the address field with length depending on record type
+ Iter = toHexStr(Address, Iter, getAddressSize());
+ // write data byte by byte
+ for (uint8_t X : Data)
+ Iter = toHexStr(X, Iter, 2);
+ // write the 1 byte checksum
+ Iter = toHexStr(getChecksum(), Iter, 2);
+ *Iter++ = '\r';
+ *Iter++ = '\n';
+ assert(Iter == Line.end());
+ return Line;
+}
+
+uint8_t SRecord::getChecksum() const {
+ uint32_t Sum = getCount();
+ Sum += (Address >> 24) & 0xFF;
+ Sum += (Address >> 16) & 0xFF;
+ Sum += (Address >> 8) & 0xFF;
+ Sum += Address & 0xFF;
+ for (uint8_t Byte : Data)
+ Sum += Byte;
+ return 0xFF - (Sum & 0xFF);
+}
+
+size_t SRecord::getSize() const {
+ // 2 characters for type, count, checksum, CRLF
+ return 2 + 2 + getAddressSize() + Data.size() * 2 + 2 + 2;
+}
+
+uint8_t SRecord::getAddressSize() const {
+ switch (Type) {
+ case Type::S2:
+ return 6;
+ case Type::S3:
+ return 8;
+ case Type::S7:
+ return 8;
+ case Type::S8:
+ return 6;
+ default:
+ return 4;
+ }
+}
+
+uint8_t SRecord::getCount() const {
+ uint8_t DataSize = Data.size();
+ uint8_t ChecksumSize = 1;
+ return getAddressSize() / 2 + DataSize + ChecksumSize;
+}
+
+SRecord SRecord::getHeader(StringRef FileName) {
+ // limit header to 40 characters
+ StringRef HeaderContents = FileName.slice(0, 40);
+ ArrayRef<uint8_t> Data(
+ reinterpret_cast<const uint8_t *>(HeaderContents.data()),
+ HeaderContents.size());
+ return {SRecord::S0, 0, Data};
+}
+
+size_t SRECWriter::writeHeader(uint8_t *Buf) {
+ SRecLineData Record = SRecord::getHeader(OutputFileName).toString();
+ memcpy(Buf, Record.data(), Record.size());
+ return Record.size();
+}
+
+size_t SRECWriter::writeTerminator(uint8_t *Buf, uint8_t Type) {
+ assert(Type >= 7 && Type <= 9);
+ uint32_t Entry = Obj.Entry;
+ SRecLineData Data = SRecord{Type, Entry, {}}.toString();
+ memcpy(Buf, Data.data(), Data.size());
+ return Data.size();
+}
+
+Error SRECWriter::write() {
+ uint32_t HeaderSize =
+ writeHeader(reinterpret_cast<uint8_t *>(Buf->getBufferStart()));
+ SRECSectionWriter Writer(*Buf, HeaderSize);
+ for (const SectionBase *S : Sections) {
+ if (Error E = S->accept(Writer))
+ return E;
+ }
+ Writer.writeRecords();
+ uint64_t Offset = Writer.getBufferOffset();
+ /// S1 terminates with S9, S2 with S8, S3 with S7
+ uint8_t TerminatorType = 10 - Writer.getType();
+ Offset += writeTerminator(
+ reinterpret_cast<uint8_t *>(Buf->getBufferStart() + Offset),
+ TerminatorType);
+ assert(Offset == TotalSize);
+ Out.write(Buf->getBufferStart(), Buf->getBufferSize());
+ return Error::success();
+}
+
+Error SRECWriter::checkSection(const SectionBase &S) const {
+ if (!isUInt<32>(S.Addr + S.Size - 1))
+ return createStringError(
+ errc::invalid_argument,
+ "Section '%s' address range [0x%llx, 0x%llx] is not 32 bit",
+ S.Name.c_str(), S.Addr, S.Addr + S.Size - 1);
+ return Error::success();
+}
+
+Error SRECWriter::finalize() {
+ // We can't write 64-bit addresses.
+ if (!isUInt<32>(Obj.Entry))
+ return createStringError(errc::invalid_argument,
+ "Entry point address 0x%llx overflows 32 bits",
+ Obj.Entry);
+
+ for (const SectionBase &S : Obj.sections()) {
+ if ((S.Flags & ELF::SHF_ALLOC) && S.Type != ELF::SHT_NOBITS && S.Size > 0) {
+ if (Error E = checkSection(S))
+ return E;
+ Sections.push_back(&S);
+ }
+ }
+
+ llvm::sort(Sections, [](const SectionBase *A, const SectionBase *B) {
+ return sectionLMA(A) < sectionLMA(B);
+ });
+
+ std::unique_ptr<WritableMemoryBuffer> EmptyBuffer =
+ WritableMemoryBuffer::getNewMemBuffer(0);
+ if (!EmptyBuffer)
+ return createStringError(errc::not_enough_memory,
+ "failed to allocate memory buffer of 0 bytes");
+
+ SRECSectionWriterBase LengthCalc(*EmptyBuffer, 0);
+ for (const SectionBase *Sec : Sections)
+ if (Error Err = Sec->accept(LengthCalc))
+ return Err;
+
+ LengthCalc.writeRecords();
+
+ // We need to add the size of the Header and Terminator records
+ SRecord Header = SRecord::getHeader(OutputFileName);
+ uint8_t TerminatorType = 10 - LengthCalc.getType();
+ SRecord Terminator = {TerminatorType, static_cast<uint32_t>(Obj.Entry), {}};
+ TotalSize =
+ LengthCalc.getBufferOffset() + Header.getSize() + Terminator.getSize();
+ Buf = WritableMemoryBuffer::getNewMemBuffer(TotalSize);
+ if (!Buf)
+ return createStringError(errc::not_enough_memory,
+ "failed to allocate memory buffer of 0x" +
+ Twine::utohexstr(TotalSize) + " bytes");
+ return Error::success();
+}
+
namespace llvm {
namespace objcopy {
namespace elf {
diff --git a/llvm/lib/ObjCopy/ELF/ELFObject.h b/llvm/lib/ObjCopy/ELF/ELFObject.h
index 95bea0964eaef3..c33d096e32fe35 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObject.h
+++ b/llvm/lib/ObjCopy/ELF/ELFObject.h
@@ -172,6 +172,8 @@ template <class ELFT> class ELFSectionSizer : public MutableSectionVisitor {
friend class SectionWriter; \
friend class IHexSectionWriterBase; \
friend class IHexSectionWriter; \
+ friend class SRECSectionWriterBase; \
+ friend class SRECSectionWriter; \
template <class ELFT> friend class ELFSectionWriter; \
template <class ELFT> friend class ELFSectionSizer;
@@ -390,6 +392,106 @@ class IHexWriter : public Writer {
IHexWriter(Object &Obj, raw_ostream &Out) : Writer(Obj, Out) {}
};
+using SRecLineData = SmallVector<char, 64>;
+struct SRecord {
+ uint8_t Type;
+ uint32_t Address;
+ ArrayRef<uint8_t> Data;
+ SRecLineData toString() const;
+ uint8_t getCount() const;
+ // get address size in characters
+ uint8_t getAddressSize() const;
+ uint8_t getChecksum() const;
+ size_t getSize() const;
+ static SRecord getHeader(StringRef FileName);
+ static SRecord getTerminator(uint8_t Type, uint32_t Entry);
+ enum Type : uint8_t {
+ // Vendor specific text comment
+ S0 = 0,
+ // Data that starts at a 16 bit address
+ S1 = 1,
+ // Data that starts at a 24 bit address
+ S2 = 2,
+ // Data that starts at a 32 bit address
+ S3 = 3,
+ // Reserved
+ S4 = 4,
+ // 16 bit count of S1/S2/S3 records (optional)
+ S5 = 5,
+ // 32 bit count of S1/S2/S3 records (optional)
+ S6 = 6,
+ // Terminates a series of S3 records
+ S7 = 7,
+ // Terminates a series of S2 records
+ S8 = 8,
+ // Terminates a series of S1 records
+ S9 = 9
+ };
+
+private:
+ uint16_t getCheckSum(StringRef S) const;
+};
+
+/// The real Writer
+class SRECWriter : public Writer {
+ StringRef OutputFileName;
+ size_t TotalSize = 0;
+ std::vector<const SectionBase *> Sections;
+ size_t writeHeader(uint8_t *Buf);
+ size_t writeTerminator(uint8_t *Buf, uint8_t Type);
+ Error checkSection(const SectionBase &S) const;
+
+public:
+ ~SRECWriter() = default;
+ Error finalize() override;
+ Error write() override;
+ SRECWriter(Object &Obj, raw_ostream &OS, StringRef OutputFile)
+ : Writer(Obj, OS), OutputFileName(OutputFile) {}
+};
+
+/// Base class for SRecSectionWriter
+/// This class does not actually write anything. It is only used for size
+/// calculation.
+class SRECSectionWriterBase : public BinarySectionWriter {
+protected:
+ // Offset in the output buffer
+ uint64_t Offset;
+ // Sections start after the header
+ uint64_t HeaderSize;
+ // Type of records to write
+ uint8_t Type = SRecord::S1;
+ std::vector<SRecord> Records;
+ void writeSection(const SectionBase &S, ArrayRef<uint8_t> Data);
+
+public:
+ explicit SRECSectionWriterBase(WritableMemoryBuffer &Buf,
+ uint64_t StartOffset)
+ : BinarySectionWriter(Buf), Offset(StartOffset), HeaderSize(StartOffset) {
+ }
+
+ // Once the type of all records is known, write the records
+ virtual void writeRecords();
+ uint64_t getBufferOffset() const { return Offset; }
+ Error visit(const Section &S) final;
+ Error visit(const OwnedDataSection &S) final;
+ Error visit(const StringTableSection &S) override;
+ Error visit(const DynamicRelocationSection &S) final;
+ uint8_t getType() const { return Type; };
+ void writeRecord(SRecord &Record);
+ using BinarySectionWriter::visit;
+};
+
+// Real SREC section writer
+class SRECSectionWriter : public SRECSectionWriterBase {
+ void writeRecord(SRecord &Record, uint64_t Off);
+
+public:
+ SRECSectionWriter(WritableMemoryBuffer &Buf, uint64_t Offset)
+ : SRECSectionWriterBase(Buf, Offset) {}
+ Error visit(const StringTableSection &Sec) override;
+ void writeRecords() override;
+};
+
class SectionBase {
public:
std::string Name;
diff --git a/llvm/test/tools/llvm-objcopy/ELF/Inputs/srec-elf-sections-err.yaml b/llvm/test/tools/llvm-objcopy/ELF/Inputs/srec-elf-sections-err.yaml
new file mode 100644
index 00000000000000..ecd031c1760e75
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/ELF/Inputs/srec-elf-sections-err.yaml
@@ -0,0 +1,22 @@
+!ELF
+FileHeader:
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_EXEC
+ Machine: EM_X86_64
+Sections:
+ - Name: .text1
+# Part of section data is in 32-bit address range
+# and part isn't.
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ Address: 0xFFFFFFF8
+ AddressAlign: 0x8
+ Content: "000102030405060708"
+ - Name: .text2
+ # Entire secion is outside of 32-bit range
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ Address: 0xFFFFFFFF0
+ AddressAlign: 0x8
+ Content: "0001020304"
diff --git a/llvm/test/tools/llvm-objcopy/ELF/Inputs/srec-elf-sections.yaml b/llvm/test/tools/llvm-objcopy/ELF/Inputs/srec-elf-sections.yaml
new file mode 100644
index 00000000000000..30190283b09c52
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/ELF/Inputs/srec-elf-sections.yaml
@@ -0,0 +1,46 @@
+!ELF
+FileHeader:
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_EXEC
+ Machine: EM_X86_64
+Sections:
+ - Name: .data1
+# Records for this section should come last
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC ]
+ Content: "11111111111111111111"
+ Address: 0xEFFFFF
+ AddressAlign: 0x8
+ - Name: .data2
+# This section overlaps 24-bit address boundary, so we expect
+# its record type to be S3
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC ]
+ Content: "3031323334353637383940"
+ Address: 0xFFFFF8
+ AddressAlign: 0x8
+ - Name: .text
+# This section's contents exceeds default line length of 16 bytes
+# so we expect two lines created for it. Records for this section
+# should appear before records for the previous section.
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ Address: 0x1000
+ AddressAlign: 0x8
+ Content: "000102030405060708090A0B0C0D0E0F1011121314"
+ - Name: .bss
+# NOBITS sections are not written
+ Type: SHT_NOBITS
+ Flags: [ SHF_ALLOC ]
+ Address: 0x10100
+ Size: 0x1000
+ AddressAlign: 0x8
+ - Name: .dummy
+# Non-allocatable sections are not written
+ Type: SHT_PROGBITS
+ Flags: [ ]
+ Address: 0x20FFF8
+ Size: 65536
+ AddressAlign: 0x8
+
diff --git a/llvm/test/tools/llvm-objcopy/ELF/Inputs/srec-elf-segments.yaml b/llvm/test/tools/llvm-objcopy/ELF/Inputs/srec-elf-segments.yaml
new file mode 100644
index 00000000000000..3f4b27ba3ae3ce
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/ELF/Inputs/srec-elf-segments.yaml
@@ -0,0 +1,45 @@
+# A test where LMA and VMA are different. Records should use the LMA
+!ELF
+FileHeader:
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_EXEC
+ Machine: EM_X86_64
+ Entry: 0x8000
+ProgramHeaders:
+ - Type: PT_LOAD
+ Flags: [ PF_X, PF_R ]
+ FirstSec: .text
+ LastSec: .text
+ VAddr: 0x8000
+ Align: 0x1000
+ Offset: 0x1000
+ - Type: PT_LOAD
+ Flags: [ PF_W, PF_R ]
+ FirstSec: .bss
+ LastSec: .data
+ VAddr: 0x20000
+ PAddr: 0x8005
+ Align: 0x1000
+ Offset: 0x2000
+Sections:
+ - Name: .text
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ Address: 0x8000
+ AddressAlign: 0x4
+ Offset: 0x1000
+ Content: '0020048090'
+ - Name: .bss
+ Type: SHT_NOBITS
+ Flags: [ SHF_WRITE, SHF_ALLOC ]
+ Address: 0x20000
+ AddressAlign: 0x1
+ Offset: 0x2000
+ Size: 0x4
+ - Name: .data
+ Type: SHT_PROGBITS
+ Flags: [ SHF_WRITE, SHF_ALLOC ]
+ Address: 0x20004
+ AddressAlign: 0x1
+ Content: FF00
diff --git a/llvm/test/tools/llvm-objcopy/ELF/srec-writer.test b/llvm/test/tools/llvm-objcopy/ELF/srec-writer.test
new file mode 100644
index 00000000000000..9624e33807873b
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/ELF/srec-writer.test
@@ -0,0 +1,44 @@
+# RUN: yaml2obj %p/Inputs/srec-elf-sections.yaml -o %t
+# RUN: llvm-objcopy -O srec %t - | FileCheck %s
+
+# Check that section address range overlapping 32 bit range
+# triggers an error
+# RUN: yaml2obj %p/Inputs/srec-elf-sections-err.yaml -o %t.err
+# RUN: not llvm-objcopy -O srec --only-section=.text1 %t.err - 2>&1 \
+# RUN: | FileCheck %s --check-prefix=BAD-ADDR
+# RUN: not llvm-objcopy -O srec --only-section=.text2 %t.err - 2>&1 \
+# RUN: | FileCheck %s --check-prefix=BAD-ADDR2
+
+# Check that zero length section is not written
+# RUN: llvm-objcopy -O srec --only-section=.text %t.err - \
+# RUN:| FileCheck %s --check-prefix=ZERO_SIZE_SEC
+
+# Start address which exceeds 32 bit range triggers an error
+# RUN: not llvm-objcopy -O srec --set-start=0xF00000000 %t - 2>&1 \
+# RUN: | FileCheck %s --check-prefix=BAD_START
+
+# Records should use LMA instead of VMA
+# RUN: yaml2obj %p/Inputs/srec-elf-segments.yaml -o %t.lma
+# RUN: llvm-objcopy -O srec %t.lma - | FileCheck %s --check-prefix=LMA
+
+CHECK: S00400002DCE
+CHECK-NEXT: S31500001000000102030405060708090A0B0C0D0E0F62
+CHECK-NEXT: S30A0000101010111213147B
+CHECK-NEXT: S30F00EFFFFF1111111111111111111159
+CHECK-NEXT: S31000FFFFF83031323334353637383940AC
+CHECK-NEXT: S70500000000FA
+
+LMA: S00400002DCE
+LMA-NEXT: S1088000002004809043
+LMA-NEXT: S1058009FF0072
+LMA-NEXT: S90380007C
+
+# BAD-ADDR: Section '.text1' address range [0xfffffff8, 0x100000000] is not 32 bit
+# BAD-ADDR2: Section '.text2' address range [0xffffffff0, 0xffffffff4] is not 32 bit
+
+# there should be no records besides header and terminator
+# ZERO_SIZE_SEC-NOT: {{S[1-8]}}
+# ZERO_SIZE_SEC: S00400002DCE
+# ZERO_SIZE_SEC-NEXT: S9030000FC
+
+# BAD_START: Entry point address 0xf00000000 overflows 32 bits
\ No newline at end of file
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
index f15307181fad61..3188dde8393d68 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
+++ b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
@@ -687,6 +687,7 @@ objcopy::parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
Config.OutputFormat = StringSwitch<FileFormat>(OutputFormat)
...
[truncated]
|
jh7370
reviewed
Dec 19, 2023
There was a problem hiding this comment.
Thank you for your contribution! This is a very large patch, and I won't have time to review it until the new year. In the meantime, some high-level comments:
- Please make sure to update the llvm-objcopy documentation. You can find it at llvm/docs/CommandGuide/llvm-objcopy.rst.
- I assume you have a need for this functionality in your company/team/...? We don't generally accept patches for new functionality if there is no concrete use case.
- It may be better to split this into a sequence of PRs, which incrementally add the functionality, rather than trying to add it all at once. For example, is it possible to support an empty ELF file with no sections/symbols/relocations/program headers in a first patch, and then add support for different section types etc in follow-on PRs?
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Thank you for your feedback. This functionality was mentioned in EuroLLVM 2023:
SREC is used in our team as an intermediate format while generating memory images. Adding support would help them transition to LLVM. I have updated the documentation to reflect the new output format. I also simplified this patch like you suggested. It will only write the header record and ignore any sections. |
MaskRay
reviewed
Dec 20, 2023
| @@ -0,0 +1,11 @@ | |||
| # RUN: yaml2obj %p/Inputs/srec-elf-sections.yaml -o %t | |||
There was a problem hiding this comment.
srec-elf-sections.yaml is only used by one test. We prefer inlining the data into the main test file.
There was a problem hiding this comment.
The test now uses multiple files
There was a problem hiding this comment.
The input file is only used in one single test. You can have multiple different YAML inputs within the same test file using the --docnum option. See many other tests that use this option for examples of this.
| # RUN: yaml2obj %p/Inputs/srec-elf-sections.yaml -o %t | ||
| # RUN: llvm-objcopy -O srec %t - | FileCheck %s | ||
|
|
||
| # The record type for the header should be S0 with a 2 byte address |
There was a problem hiding this comment.
## for non-RUN-non-CHECK comments
| CHECK: S00400002DCE | ||
| CHECK-EMPTY: | ||
|
|
||
| # FIXME: write the rest of the records |
llvm/lib/ObjCopy/ELF/ELFObject.h
Outdated
| }; | ||
| }; | ||
|
|
||
| /// FIXME: This is currently a contrived writer that only writes the |
There was a problem hiding this comment.
If we decide to accept srec support, I think the MVP needs to support writing data.
| @@ -0,0 +1,48 @@ | |||
| # FIXME: Currently the sections in this file are ignored and we | |||
There was a problem hiding this comment.
Tests from the original ihex patch (https://reviews.llvm.org/D60270) may be useful.
There was a problem hiding this comment.
The tests in this patch are based on the ones in the ihex patch
statham-arm
reviewed
Jan 8, 2024
statham-arm
requested changes
Jan 10, 2024
|
I'd like to take a look at this before this gets merged, please, but I'm out of time to review this today. I should be able to review this tomorrow. |
jh7370
requested changes
Jan 15, 2024
| @@ -0,0 +1,11 @@ | |||
| # RUN: yaml2obj %p/Inputs/srec-elf-sections.yaml -o %t | |||
There was a problem hiding this comment.
The input file is only used in one single test. You can have multiple different YAML inputs within the same test file using the --docnum option. See many other tests that use this option for examples of this.
jh7370
reviewed
Jan 19, 2024
There was a problem hiding this comment.
@quic-areg, please don't resolve comment threads where I've requested changes: please let me resolve them as a marker that I'm happy with the resolution.
A thought on testing: how do you show that the lines are terminated properly (including a \r)?
llvm/lib/ObjCopy/ELF/ELFObject.h
Outdated
| // Once the type of all records is known, write the records | ||
| using BinarySectionWriter::visit; | ||
|
|
||
| // Once the type of all records is known, write the records. |
There was a problem hiding this comment.
This doesn't feel like a comment that belongs in the header.
llvm/lib/ObjCopy/ELF/ELFObject.h
Outdated
| // Once the type of all records is known, write the records | ||
| using BinarySectionWriter::visit; | ||
|
|
||
| // Once the type of all records is known, write the records. | ||
| virtual void writeRecords(uint32_t Entry); |
There was a problem hiding this comment.
Should this be override rather than virtual?
There was a problem hiding this comment.
No it does not override anything. It is now a pure virtual function as part of the refactoring you suggested.
There was a problem hiding this comment.
Looks like there's no need for this to be virtual any more, as nothing overrides it?
| ## Check that zero length section is not written | ||
| # RUN: llvm-objcopy -O srec --only-section=.text %t.err - \ | ||
| # RUN:| FileCheck %s --check-prefix=ZERO_SIZE_SEC | ||
|
|
||
| ## Terminator should contain the entry point |
There was a problem hiding this comment.
Nit: comment style (missing full stop). Applies throughout.
jh7370
reviewed
Jan 26, 2024
jh7370
approved these changes
Feb 8, 2024
There was a problem hiding this comment.
Aside from one nit (unnecessary virtual I believe), I think this PR now gets an LGTM from me.
A couple of caveats:
- I haven't attempted to review this against the SREC docs: I don't have time to do this, so I'm assuming that @statham-arm has done so.
- I haven't attempted to verify that the testing covers everything. If you have access to a code coverage tool, it might be instructive to run it through your modified llvm-objcopy with the tests you've written, to see if you've covered all the new code you've added.
P.S. Sorry for the delay - a combination of illness and a large review backlog has delayed me getting back to this until now.
llvm/lib/ObjCopy/ELF/ELFObject.h
Outdated
| // Once the type of all records is known, write the records | ||
| using BinarySectionWriter::visit; | ||
|
|
||
| // Once the type of all records is known, write the records. | ||
| virtual void writeRecords(uint32_t Entry); |
There was a problem hiding this comment.
Looks like there's no need for this to be virtual any more, as nothing overrides it?
Adds a new output target "srec" to write SREC files from ELF inputs. https://en.wikipedia.org/wiki/SREC_(file_format)
quic-areg
force-pushed
the
objcopy-srec
branch
from
947ed54
to
f736947
Compare
|
Can this be merged? I have squashed my commits but I do not think I have permissions to merge. |
jh7370
approved these changes
Feb 9, 2024
|
@quic-areg, it looks like you have your email address hidden through github. Per the consensus on https://discourse.llvm.org/t/hidden-emails-on-github-should-we-do-something-about-it/74223/32 this isn't really desired. You have two options:
|
|
@jh7370 Github should show my email now |
|
Thanks, merged. Please keep an eye out for emails from the build bots: if you get any failure notifications, check to see if it is likely to do with your change. |
|
build fix for this in 5afbed1 |
|
Another fix in 2cbe5a3 |
SquallATF
pushed a commit
to SquallATF/llvm-project
that referenced
this pull request
May 21, 2024
Adds a new output target "srec" to write SREC files from ELF inputs. https://en.wikipedia.org/wiki/SREC_(file_format)
SquallATF
pushed a commit
to SquallATF/llvm-project
that referenced
this pull request
Jun 9, 2024
Adds a new output target "srec" to write SREC files from ELF inputs. https://en.wikipedia.org/wiki/SREC_(file_format)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a new output target "srec" to write SREC files from ELF inputs.
https://en.wikipedia.org/wiki/SREC_(file_format)