On Tue, Jul 31, 2012 at 2:21 AM, Stephen Checkoway <span dir="ltr"><<a href="mailto:s@pahtak.org" target="_blank" class="cremed">s@pahtak.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On Jul 31, 2012, at 4:10 AM, Chandler Carruth wrote:<br>
<br>
> Cool.<br>
<br>
Thanks for taking a look at this so quickly! I apologize for the length of this email.<br>
<div class="im"><br>
> First, this needs a test in the 'test/Driver/...' tree. Likely with other BSD 'ld' invocation tests. There are lots of examples to crib from, in particular the Linux ones.<br>
<br>
</div>I'm actually not sure what the right way to test this is.</blockquote><div><br></div><div>The tests shouldn't have anything to do with the actual system. Look at the linux-ld.c tests. We use fake system root stub trees checked into the test suite to simulate a sysroot, and then inspect the command line which would be used to invoke ld without over invoking it. This is what you should extend.</div>
<div><br></div><div>There are piles of tests for different OSes here, you should be able to copy one to get a test for this feature.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The base FreeBSD does not have gold so any test of gold shouldn't run. In fact, it seems like LTO support should be tested at configure time. Currently, Darwin and Linux (and FreeBSD with my patch) just report that they support LTO, even though the rest of the tool chain may not. This causes errors later:<br>
</blockquote><div><br></div><div>Bleh, this is gross.</div><div><br></div><div>First, it is an explicit goal to avoid having the ./configure used to build the clang binary really change much of its behavior. That makes it harder to test and debug.</div>
<div><br></div><div>The driver tries to inspect the system in a very efficient way (filesystem probes mostly) to determine the correct way to invoke 'ld'. I think it is fine for the driver to look for 'ld.gold' and use that instead of 'ld' on OSes where that is a plausible thing to do (it sounds like FreeBSD and Debian, but I would want one of the FreeBSD toolchain maintainers to chime in on FreeBSD there).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><br>
> Second, can we factor out the logic to add the actual plugin flag? It should already exist somewhere for Linux?<br>
<br>
</div>The freebsd::Link and linux::Link classes don't have much in the way of shared ancestry. They're both subclasses of Tool which seems like the wrong place to put such common code. Maybe they, and others that use GNU binutils should inherit from something more specific than Tool that could handle both adding adding the -plugin LLVMgold.so arguments as well as the appropriate selection of linker.<br>

<br>
Otherwise, it seems like factoring this out wouldn't save much. Here are the two implementations currently (I clearly copied from the Linux implementation, comment and all).<br></blockquote><div><br></div><div>I'm really just looking for sharing the fiddly path logic, nothing really comprehensive.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Linux:<br>
  if (D.IsUsingLTO(Args) || Args.hasArg(options::OPT_use_gold_plugin)) {<br>
    CmdArgs.push_back("-plugin");<br>
    std::string Plugin = ToolChain.getDriver().Dir + "/../lib/LLVMgold.so";<br>
    CmdArgs.push_back(Args.MakeArgString(Plugin));<br>
  }<br>
<br>
FreeBSD:<br>
  const bool UseGold = D.IsUsingLTO(Args) || Args.hasArg(options::OPT_use_gold_plugin);<br>
  if (UseGold) {<br>
    CmdArgs.push_back("-plugin");<br>
    std::string Plugin = getToolChain().getDriver().Dir + "/../lib/LLVMgold.so";<br>
    CmdArgs.push_back(Args.MakeArgString(Plugin));<br>
  }<br>
<br>
Something like<br>
<br>
static void addGoldPlugin(ArgStringList &CmdArgs, StringRef Plugin) {<br>
  CmdArgs.push_back("-plugin");<br>
  CmdArgs.push_back(Args.MakeArgString(Plugin.str()));<br>
}<br>
<br>
// ...<br>
  if (UseGold)<br>
    addGoldPlugin(CmdArgs, getToolChain().getDriver().Dir + "/../lib/LLVMgold.so");<br></blockquote><div><br></div><div>How about sinking the relative path and file name into the static helper?</div><div><br></div>
<div>  addGoldPlugin(CmdArgs, getToolChain().getDriver().Dir);</div><div><br></div></div></div>