Arent you possibly thinking of Layouting the output file ? Is that not relevant in context of COFF ?
Isnt there a need for a LayoutPass with COFF ?
Do atoms have the same behaviour in terms of resolution in COFF ?
Need more testcases for this patch.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:242
@@ +241,3 @@
+
+ std::vector<SectionChunk*> _sections;
+};
----------------
Why is this public ?
================
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 {
----------------
You need to change this to \brief to be consistent with doxygen comments
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:42
@@ +41,3 @@
+
+// A Chunk is an abstrace contiguous range in an output file.
+class Chunk {
----------------
Doxygen.
================
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.
+//
----------------
Does the PE/COFF file have 32/64bit types ? If so whats the current Reader/Writer supporting ?
================
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:
> Does the PE/COFF file have 32/64bit types ? If so whats the current Reader/Writer supporting ?
Empty ?
================
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.
----------------
Is this a binutils requirement, is llvm-objdump able to print ?
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:71
@@ +70,3 @@
+ // parse such irregular header.
+ _size = 128;
+
----------------
Why size is 128 ?
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:96
@@ +95,3 @@
+ PEHeaderChunk(const PECOFFTargetInfo &targetInfo) : Chunk() {
+ using namespace llvm::COFF;
+
----------------
not sure if this is proper llvm convention.
================
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;
----------------
The comment is misleading. All these should come from TargetInfo.
================
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;
+
----------------
all of these should possibly come from WinTargetInfo.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:100
@@ +99,3 @@
+ _size = sizeof(_peHeader);
+ std::memset(&_peHeader, 0, sizeof(_peHeader));
+
----------------
Why do you want to memset if you are setting all members ?
================
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;
+
----------------
Same here, it should possibly come from TargetInfo.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:114-115
@@ +113,4 @@
+
+ // 0x10b indicates a normal executable. For PE32+ it should be 0x20b.
+ _peHeader.Magic = 0x10b;
+
----------------
The comment is misleading too.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:168
@@ +167,3 @@
+
+ llvm::COFF::PE32Header _peHeader;
+};
----------------
Why public ?
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:187
@@ +186,3 @@
+
+ llvm::COFF::DataDirectory _dirs[16];
+};
----------------
public ?
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:211
@@ +210,3 @@
+
+ llvm::COFF::section _sectionHeader;
+
----------------
public ?
================
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;
+ }
+
----------------
size should return size and not align. you are trying to put more functionality in size.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:309
@@ +308,3 @@
+ // Round up to the nearest alignment boundary.
+ offset = (offset + chunk->align() - 1) & -chunk->align();
+ chunk->setFileOffset(offset);
----------------
chunk should have align functionality.
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:29
@@ -15,3 +28,3 @@
namespace lld {
namespace pecoff {
----------------
should this be in coff namespace instead ?
================
Comment at: lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:80
@@ +79,3 @@
+
+ _dosHeader.Magic = 0x5a4d; // PE starts with the magic "MZ"
+ _dosHeader.AddressOfNewExeHeader = _size;
----------------
SHould come from TargetInfo.
http://llvm-reviews.chandlerc.com/D892