<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 13/08/2013 0:02, Rui Ueyama wrote:<br>
</div>
<blockquote
cite="mid:CAJENXgv7HogG9qAwJMjQm_QY_Wf4iVU-q-MmXJ3YNZGbkFwvRg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>We have unit tests for the driver in the following file. In
the file, we have bunch of tests for all the command line
options. You could add two new tests for the cases (1) if
/entry is given or (2) if /entry is not given.</div>
<div><br>
</div>
lld/unittests/DriverTests/WinLinkDriverTest.cpp<br>
</div>
<div class="gmail_extra"><br>
</div>
</blockquote>
I've added 2 tests, both to check the default entry symbol name, one
for console subsystem and another for windows subsystem. I've not
added a test for the case the /entry is given since the Basic test
already checks it.<br>
Besides the driver tests, I'd like to add tests to check if the
AddressOfEntryPoint is correctly setted in the PE Header of the
generated image. I understand that such tests must be implemented in
Lit test system.<br>
<blockquote
cite="mid:CAJENXgv7HogG9qAwJMjQm_QY_Wf4iVU-q-MmXJ3YNZGbkFwvRg@mail.gmail.com"
type="cite">
<div class="gmail_extra"><br>
<div class="gmail_quote">On Mon, Aug 12, 2013 at 2:58 PM, Rui
Ueyama <span dir="ltr"><<a moz-do-not-send="true"
href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div>Code mostly looks good, some nits.</div>
<div><br>
</div>
<div>> +// Sets a default entry point symbol name
depending on context image type and</div>
<div>> +// subsystem. These default names are MS CRT
compliant.</div>
<div>> +void
setDefaultEntrySymbolName(PECOFFLinkingContext
&context)</div>
<div>> +{</div>
<div>> + if (context.getImageType() ==
PECOFFLinkingContext::ImageType::IMAGE_DLL)</div>
<div>> +
context.setEntrySymbolName("_DllMainCRTStartup");</div>
<div>> + else {</div>
<div><br>
</div>
<div>nit: we usually use {} if "else" has takes a block.</div>
<div><br>
</div>
<div>> + llvm::COFF::WindowsSubsystem subsystem =
context.getSubsystem();</div>
<div>> + if (subsystem ==
llvm::COFF::WindowsSubsystem::IMAGE_SUBSYSTEM_WINDOWS_GUI)</div>
<div>> +
context.setEntrySymbolName("WinMainCRTStartup");</div>
<div>> + else if (subsystem ==</div>
<div>> +
llvm::COFF::WindowsSubsystem::IMAGE_SUBSYSTEM_WINDOWS_CUI)</div>
<div>> +
context.setEntrySymbolName("mainCRTStartup");</div>
<div>> + }</div>
<div>> +}</div>
<div><br>
</div>
<div>You may want to add an underscore at the beginning of
the symbols. That symbol</div>
<div>mangling is needed only on Windows/x86, so we will
have to add "_" only when on</div>
<div>x86, but currently we are hard-coding the underscore.</div>
<div><br>
</div>
<div>> // Parses the given command line options and
returns the result. Returns NULL if</div>
<div>> // there's an error in the options.</div>
<div>> std::unique_ptr<llvm::opt::InputArgList>
parseArgs(int argc, const char *argv[],</div>
<div>> @@ -356,6 +372,8 @@ bool
WinLinkDriver::parse(int argc, const char *argv[],</div>
<div>> // handle /entry</div>
<div>> if (llvm::opt::Arg *arg =
parsedArgs->getLastArg(OPT_entry))</div>
<div>>
context.setEntrySymbolName(arg->getValue());</div>
<div>> + else</div>
<div>> + setDefaultEntrySymbolName(context);</div>
<div><br>
</div>
<div>nit: brace</div>
<div><br>
</div>
<div>> + void setAddressOfEntryPoint(TextSectionChunk
*text, PEHeaderChunk *peHeader) {</div>
<div class="im">
<div>> + // Find the virtual address of the entry
point symbol if any.</div>
<div>> + // PECOFF spec says that entry point for
dll images is optional, in which</div>
</div>
<div>> + // case it must be set to 0.</div>
<div>> + if
(_PECOFFLinkingContext.entrySymbolName().empty()
&&</div>
<div>> + _PECOFFLinkingContext.getImageType() ==
PECOFFLinkingContext::IMAGE_DLL)</div>
<div class="im">
<div>> + peHeader->setAddressOfEntryPoint(0);</div>
<div>> + else {</div>
<div><br>
</div>
</div>
<div>nit: brace</div>
<div><br>
</div>
</div>
</blockquote>
</div>
</div>
</blockquote>
<br>
Corrections made in the attached patch.<br>
<br>
</body>
</html>