[PATCH] [PECOFF][Writer] Implement the writer that can emit text section.
Rui Ueyama
ruiu at google.com
Thu May 30 11:49:29 PDT 2013
Thank you for reviewing!
There's a need to add LayoutPass for PE/COFF, and I will do that in another patch. The aim of this patch is to emit a smallest valid Windows executable. There are still a lot to-dos in this file.
What tests could be added? I'd think the most important thing of the test is the fact that the code can write a .exe file that llvm-readobj can parse.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:29
@@ -15,3 +28,3 @@
namespace lld {
namespace pecoff {
----------------
Shankar Kalpathi Easwaran wrote:
> should this be in coff namespace instead ?
makes sense. will do.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:31
@@ -17,1 +30,3 @@
+//
+// PE/COFF file consists of DOS Header, PE Header, COFF Header and Section
----------------
Shankar Kalpathi Easwaran wrote:
> Shankar Kalpathi Easwaran wrote:
> > Does the PE/COFF file have 32/64bit types ? If so whats the current Reader/Writer supporting ?
> Empty ?
removed
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:31-33
@@ -17,1 +30,5 @@
+//
+// PE/COFF file consists of DOS Header, PE Header, COFF Header and Section
+// Tables followed by raw section data.
+//
----------------
Rui Ueyama wrote:
> Shankar Kalpathi Easwaran wrote:
> > Shankar Kalpathi Easwaran wrote:
> > > Does the PE/COFF file have 32/64bit types ? If so whats the current Reader/Writer supporting ?
> > Empty ?
> removed
PE/COFF has 32/64bit types, and this writer currently supports 32 bit only. Will add a comment about that.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:42
@@ +41,3 @@
+
+// A Chunk is an abstrace contiguous range in an output file.
+class Chunk {
----------------
Shankar Kalpathi Easwaran wrote:
> Doxygen.
k
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:63-64
@@ +62,4 @@
+
+// A DOSStubChunk represents the DOS compatible header at the beginning of
+// PE/COFF files.
+class DOSStubChunk : public Chunk {
----------------
Shankar Kalpathi Easwaran wrote:
> You need to change this to \brief to be consistent with doxygen comments
k
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:69
@@ +68,3 @@
+ // Make the DOS stub occupy the first 128 bytes of an exe. Technically
+ // this can be as small as 64 bytes, but GNU binutil's objdump cannot
+ // parse such irregular header.
----------------
Shankar Kalpathi Easwaran wrote:
> Is this a binutils requirement, is llvm-objdump able to print ?
I think it's binutil's bug. llmv-objdump can parse such file.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:71
@@ +70,3 @@
+ // parse such irregular header.
+ _size = 128;
+
----------------
Shankar Kalpathi Easwaran wrote:
> Why size is 128 ?
Because its' a regular size of the DOS stub.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:80
@@ +79,3 @@
+
+ _dosHeader.Magic = 0x5a4d; // PE starts with the magic "MZ"
+ _dosHeader.AddressOfNewExeHeader = _size;
----------------
Shankar Kalpathi Easwaran wrote:
> SHould come from TargetInfo.
I don't think so. This is the file magic and will never change. We may have a constant for this magic in COFF.h though.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:96
@@ +95,3 @@
+ PEHeaderChunk(const PECOFFTargetInfo &targetInfo) : Chunk() {
+ using namespace llvm::COFF;
+
----------------
Shankar Kalpathi Easwaran wrote:
> not sure if this is proper llvm convention.
removed
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:100
@@ +99,3 @@
+ _size = sizeof(_peHeader);
+ std::memset(&_peHeader, 0, sizeof(_peHeader));
+
----------------
Shankar Kalpathi Easwaran wrote:
> Why do you want to memset if you are setting all members ?
I'm not setting all the members. Some members are reserved or not interesting to me, and they needs to be zero.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:103-115
@@ +102,15 @@
+ // Set PE/COFF header fields
+ _peHeader.Signature = 0x4550; // 0x50 = 'P', 0x45 = 'E'
+ _peHeader.COFFHeader.Machine = IMAGE_FILE_MACHINE_I386;
+
+ _peHeader.COFFHeader.NumberOfSections = 1; // [FIXME]
+ _peHeader.COFFHeader.TimeDateStamp = time(NULL);
+
+ // The size of PE header including optional data directory is always 224.
+ _peHeader.COFFHeader.SizeOfOptionalHeader = 224;
+ _peHeader.COFFHeader.Characteristics = IMAGE_FILE_32BIT_MACHINE
+ | IMAGE_FILE_EXECUTABLE_IMAGE;
+
+ // 0x10b indicates a normal executable. For PE32+ it should be 0x20b.
+ _peHeader.Magic = 0x10b;
+
----------------
Shankar Kalpathi Easwaran wrote:
> all of these should possibly come from WinTargetInfo.
How misleading is the comment?
Magic and Characteristics should come from TargetInfo. This is a to-do.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:117-119
@@ +116,5 @@
+
+ // The address of entry point relative to ImageBase. Windows executable
+ // usually starts at address 0x401000.
+ _peHeader.AddressOfEntryPoint = 0x1000;
+ _peHeader.BaseOfCode = 0x1000;
----------------
Shankar Kalpathi Easwaran wrote:
> The comment is misleading. All these should come from TargetInfo.
Right. It should be set by command line option. It's also to-do.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:122-140
@@ +121,21 @@
+
+ // [FIXME] The address of data section relative to ImageBase.
+ _peHeader.BaseOfData = 0x2000;
+
+ // The address of the executable when loaded into memory. The default for
+ // DLLs is 0x10000000. The default for executables is 0x400000.
+ _peHeader.ImageBase = 0x400000;
+
+ // Sections should be page-aligned when loaded into memory, which is 4KB on
+ // x86.
+ _peHeader.SectionAlignment = 4096;
+
+ // Sections in an executable file on disk should be sector-aligned (512 byte).
+ _peHeader.FileAlignment = 512;
+
+ // [FIXME] Windows 5.1 is Windows XP.
+ _peHeader.MajorOperatingSystemVersion = 5;
+ _peHeader.MinorOperatingSystemVersion = 1;
+ _peHeader.MajorSubsystemVersion = 5;
+ _peHeader.MinorSubsystemVersion = 1;
+
----------------
Shankar Kalpathi Easwaran wrote:
> Same here, it should possibly come from TargetInfo.
Ditto.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:168
@@ +167,3 @@
+
+ llvm::COFF::PE32Header _peHeader;
+};
----------------
Shankar Kalpathi Easwaran wrote:
> Why public ?
Because some fields of the PE header needs to be adjusted after sections are built. I'd think making this private rather complicates this file.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:187
@@ +186,3 @@
+
+ llvm::COFF::DataDirectory _dirs[16];
+};
----------------
Shankar Kalpathi Easwaran wrote:
> public ?
Ditto, although they are not in use yet.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:211
@@ +210,3 @@
+
+ llvm::COFF::section _sectionHeader;
+
----------------
Shankar Kalpathi Easwaran wrote:
> public ?
Ditto
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:242
@@ +241,3 @@
+
+ std::vector<SectionChunk*> _sections;
+};
----------------
Shankar Kalpathi Easwaran wrote:
> Why is this public ?
k
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:287-291
@@ +286,7 @@
+
+ virtual uint64_t size() const {
+ // Round up to the nearest alignment border, so that the text segment ends
+ // at a border.
+ return (_size + _align - 1) & -_align;
+ }
+
----------------
Shankar Kalpathi Easwaran wrote:
> size should return size and not align. you are trying to put more functionality in size.
That's correct in general, but it's different for PE/COFF text section. IIUC, text section always needs to occupy a whole disk sector in PE/COFF regardless of its content size, so it's natural to think that the section size is multiple of the alignment.
http://llvm-reviews.chandlerc.com/D892
More information about the llvm-commits
mailing list