[PATCH][lld][PECOFF] Add Support for entry point symbol name

[Rui Ueyama]

Hi Jesús,

Thank you for doing this. This mostly looks good, but here are some nits,
mainly about the LLVM coding style.

> diff --git include/lld/ReaderWriter/PECOFFTargetInfo.h
include/lld/ReaderWriter/PECOFFTargetInfo.h
> index 10337b6..7d39dc2 100644
> --- include/lld/ReaderWriter/PECOFFTargetInfo.h
> +++ include/lld/ReaderWriter/PECOFFTargetInfo.h
> @@ -29,7 +29,7 @@ public:
>          _heapReserve(1024 * 1024), _heapCommit(4096),
>          _subsystem(llvm::COFF::IMAGE_SUBSYSTEM_UNKNOWN),
_minOSVersion(6, 0),
>          _nxCompat(true), _largeAddressAware(false),
_baseRelocationEnabled(true),
> -        _terminalServerAware(true) {}
> +        _terminalServerAware(true), _imageType(ImageType::imageExe) {}
>
>    struct OSVersion {
>      OSVersion(int v1, int v2) : majorVersion(v1), minorVersion(v2) {}
> @@ -37,6 +37,11 @@ public:
>      int minorVersion;
>    };
>
> +  enum ImageType {
> +    imageExe,
> +    imageDll
> +  };

Should be IMAGE_EXE and IMAGE_DLL.

> diff --git lib/ReaderWriter/PECOFF/PECOFFTargetInfo.cpp
lib/ReaderWriter/PECOFF/PECOFFTargetInfo.cpp
> index b1f599c..ef5de77 100644
> --- lib/ReaderWriter/PECOFF/PECOFFTargetInfo.cpp
> +++ lib/ReaderWriter/PECOFF/PECOFFTargetInfo.cpp
> @@ -76,6 +76,10 @@ bool PECOFFTargetInfo::validateImpl(raw_ostream
&diagnostics) {
>      return true;
>    }
>
> +  if (_entrySymbolName.empty() && _imageType == ImageType::imageExe) {
> +    _entrySymbolName = "_main";
> +  }

Looks like the entry function name should be _mainCRTStartup if subsystem
is "console",
_WinMainCRTStartup if subsystem is "windows", and "__DllMainCRTStartup" for
DLL.
See http://msdn.microsoft.com/en-us/library/f9t8842e(v=vs.80).aspx.

> diff --git lib/ReaderWriter/PECOFF/WriterPECOFF.cpp
lib/ReaderWriter/PECOFF/WriterPECOFF.cpp
> index 34dc1fb..4984f09 100644
> --- lib/ReaderWriter/PECOFF/WriterPECOFF.cpp
> +++ lib/ReaderWriter/PECOFF/WriterPECOFF.cpp
> @@ -245,6 +245,8 @@ public:
>
>    virtual void setSizeOfImage(uint32_t size) { _peHeader.SizeOfImage =
size; }
>
> +  virtual void setAddressOfEntryPoint(uint32_t address) {
_peHeader.AddressOfEntryPoint = address; }

80 char.

> +
>  private:
>    llvm::object::coff_file_header _coffHeader;
>    llvm::object::pe32_header _peHeader;
> @@ -351,6 +353,17 @@ public:
>        layout->_virtualAddr += rva;
>    }
>
> +  virtual bool getAtomVirtualAddress(StringRef name, uint64_t& address)
> +  {

Make it const and move "{" at the end of the previous line. Put a space
after '&'.

Why is this a virtual function?

> +    for (auto atomLayout : _atomLayouts) {
> +      if (atomLayout->_atom->name() == name) {
> +        address = atomLayout->_virtualAddr;
> +        return true;
> +      }
> +    }
> +    return false;
> +  }

What about returning the address when success and return 0 when fail? Zero
can never
be a valid virtual address so that should be safe.

>    static bool classof(const Chunk *c) {
>      Kind kind = c->getKind();
>      return kind == kindSection || kind == kindDataDirectory;
> @@ -747,6 +760,18 @@ public:
>      peHeader->setSizeOfInitializedData(rdata->size() + data->size());
>      peHeader->setNumberOfSections(_numSections);
>      peHeader->setSizeOfImage(_imageSizeInMemory);
> +
> +    // Find the virtual address of the entry point symbol if any
> +    // PeCOFF spec says that entry point for dll images is optional

- Period at the end of each sentence.
- s/PeCOFF/PECOFF/

> +    if (_PECOFFTargetInfo.entrySymbolName().empty() &&
_PECOFFTargetInfo.getImageType() == PECOFFTargetInfo::imageDll) {
> +      peHeader->setAddressOfEntryPoint(0);
> +    } else {
> +      uint64_t entryPointAddress = 0;
> +      if
(text->getAtomVirtualAddress(_PECOFFTargetInfo.entrySymbolName(),
entryPointAddress)) {
> +        // TODO: AddressOfEntryPoint in pe header is 4 byte, have to
check if entry point virtual address is within 32 bit space
> +        peHeader->setAddressOfEntryPoint(entryPointAddress);
> +      }
> +    }

80 char.

I think this is not really a TODO thing. We might want to check if we
are not creating binary larger than 4GB (or 3GB?), but that should not
be checked here but at the place we compute the atom's location. I'd
remove the TODO.

On Tue, Jul 30, 2013 at 12:59 PM, Jesús Serrano García <dagonoweda at gmail.com
> wrote:

> Currently the PECOFF writer does not take into account the entry point
> symbol name, and the resulting image starts with the firts symbol in .text
> section. Currently the AddressOfEntryPoint is assumed to be in 0x1000, wich
> is not correct according to PECOFF spec. In fact, this address may be 0 in
> DLL images, this is also handled in the patch allowing to set the image
> output type.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130730/13078212/attachment.html>