<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Feb 1, 2015 at 10:31 PM, Shankar Easwaran <span dir="ltr"><<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">On 2/2/2015 12:24 AM, Rui Ueyama wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
On Sun, Feb 1, 2015 at 10:18 PM, Shankar Easwaran <<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a>><br>
wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
The reason was more that the Context should own what is the default search<br>
directory. The ELFLinkingContext is using the baseTriple here not the real<br>
triple.<br>
<br>
</blockquote>
Could you please elaborate why you think it should?<br>
<br>
My point was that this patch scattered the logic that used to be in one<br>
function and also added a new virtual public function that we used to not<br>
need. It doesn't seem like a good tradeoff.<br>
</blockquote></span>
Are you thinking that the default search path for all the ELF targets(aarch64, armv7, etc) be housed in the Driver ? I would disagree.</blockquote><div><br></div><div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important;background-color:rgb(255,255,255)">It's hypothetical. I don't know if we need an if expression for each architecture. We currently don't have mounting conditions that we want to split them up at least. Please don't abstract things based on hypothesis, but please keep code reasonably simple for the current needs. This particular patch is a nit, but LLD is historically suffered from over-designing and over-abstraction, so we've got to cautious about that.</span></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
IMO, Adding target specific search directory in the Gnu driver does not<br>
make the code clean.<br>
<br>
Shankar Easwaran<br>
<br>
<br>
On 2/2/2015 12:11 AM, Rui Ueyama wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Does this actually improve the code quality? You added a new virtual<br>
function to the linking context, but the linking context are still using<br>
the triple to make a decision. This patch split a private function in the<br>
driver into two, and added them to the linking context as public member<br>
functions. Seems it's slightly worse than before.<br>
<br>
On Sun, Feb 1, 2015 at 10:00 PM, Shankar Easwaran <<br>
<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a>><br>
wrote:<br>
<br>
Author: shankare<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Date: Mon Feb 2 00:00:04 2015<br>
New Revision: 227784<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=227784&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=227784&view=rev</a><br>
Log:<br>
[ELF] Determine default search directories from Context.<br>
<br>
Target specific LinkingContext's determine the default search directory.<br>
<br>
No change in functionality.<br>
<br>
Modified:<br>
lld/trunk/include/lld/Driver/<u></u>Driver.h<br>
lld/trunk/include/lld/<u></u>ReaderWriter/<u></u>ELFLinkingContext.h<br>
lld/trunk/lib/Driver/<u></u>GnuLdDriver.cpp<br>
lld/trunk/lib/ReaderWriter/<u></u>ELF/X86/X86LinkingContext.h<br>
<br>
Modified: lld/trunk/include/lld/Driver/<u></u>Driver.h<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/lld/trunk/include/lld/</a><br>
Driver/Driver.h?rev=227784&r1=<u></u>227783&r2=227784&view=diff<br>
<br>
==============================<u></u>==============================<br>
==================<br>
--- lld/trunk/include/lld/Driver/<u></u>Driver.h (original)<br>
+++ lld/trunk/include/lld/Driver/<u></u>Driver.h Mon Feb 2 00:00:04 2015<br>
@@ -87,10 +87,6 @@ private:<br>
static bool applyEmulation(llvm::Triple &triple,<br>
llvm::opt::InputArgList &args,<br>
raw_ostream &diag);<br>
- static void addPlatformSearchDirs(<u></u>ELFLinkingContext &ctx,<br>
- llvm::Triple &triple,<br>
- llvm::Triple &baseTriple);<br>
-<br>
GnuLdDriver() LLVM_DELETED_FUNCTION;<br>
};<br>
<br>
<br>
Modified: lld/trunk/include/lld/<u></u>ReaderWriter/<u></u>ELFLinkingContext.h<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/ReaderWriter/" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/lld/trunk/include/lld/<u></u>ReaderWriter/</a><br>
ELFLinkingContext.h?rev=<u></u>227784&r1=227783&r2=227784&<u></u>view=diff<br>
<br>
==============================<u></u>==============================<br>
==================<br>
--- lld/trunk/include/lld/<u></u>ReaderWriter/<u></u>ELFLinkingContext.h (original)<br>
+++ lld/trunk/include/lld/<u></u>ReaderWriter/<u></u>ELFLinkingContext.h Mon Feb 2<br>
00:00:04 2015<br>
@@ -291,6 +291,11 @@ public:<br>
bool alignSegments() const { return _alignSegments; }<br>
void setAlignSegments(bool align) { _alignSegments = align; }<br>
<br>
+ /// \brief add platform specific search directories.<br>
+ virtual void addDefaultSearchDirs(llvm::<u></u>Triple & /*triple*/) {<br>
+ addSearchPath("=/usr/lib");<br>
+ }<br>
+<br>
private:<br>
ELFLinkingContext() LLVM_DELETED_FUNCTION;<br>
<br>
<br>
Modified: lld/trunk/lib/Driver/<u></u>GnuLdDriver.cpp<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Driver/" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/lld/trunk/lib/Driver/</a><br>
GnuLdDriver.cpp?rev=227784&r1=<u></u>227783&r2=227784&view=diff<br>
<br>
==============================<u></u>==============================<br>
==================<br>
--- lld/trunk/lib/Driver/<u></u>GnuLdDriver.cpp (original)<br>
+++ lld/trunk/lib/Driver/<u></u>GnuLdDriver.cpp Mon Feb 2 00:00:04 2015<br>
@@ -311,18 +311,6 @@ bool GnuLdDriver::applyEmulation(<u></u>llvm::T<br>
return true;<br>
}<br>
<br>
-void GnuLdDriver::<u></u>addPlatformSearchDirs(<u></u>ELFLinkingContext &ctx,<br>
- llvm::Triple &triple,<br>
- llvm::Triple &baseTriple) {<br>
- if (triple.getOS() == llvm::Triple::NetBSD &&<br>
- triple.getArch() == llvm::Triple::x86 &&<br>
- baseTriple.getArch() == llvm::Triple::x86_64) {<br>
- ctx.addSearchPath("=/usr/lib/<u></u>i386");<br>
- return;<br>
- }<br>
- ctx.addSearchPath("=/usr/lib")<u></u>;<br>
-}<br>
-<br>
#define LLVM_TARGET(targetName) \<br>
if ((p = elf::targetName##<u></u>LinkingContext::create(triple)<u></u>)) return p;<br>
<br>
@@ -403,8 +391,9 @@ bool GnuLdDriver::parse(int argc, const<br>
for (auto libDir : parsedArgs->filtered(OPT_L))<br>
ctx->addSearchPath(libDir-><u></u>getValue());<br>
<br>
+ // Add the default search directory specific to the target.<br>
if (!parsedArgs->hasArg(OPT_<u></u>nostdlib))<br>
- addPlatformSearchDirs(*ctx, triple, baseTriple);<br>
+ ctx->addDefaultSearchDirs(<u></u>baseTriple);<br>
<br>
// Handle --demangle option(For compatibility)<br>
if (parsedArgs->getLastArg(OPT_<u></u>demangle))<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/<u></u>ELF/X86/X86LinkingContext.h<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/X86/" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/lld/trunk/lib/<u></u>ReaderWriter/ELF/X86/</a><br>
X86LinkingContext.h?rev=<u></u>227784&r1=227783&r2=227784&<u></u>view=diff<br>
<br>
==============================<u></u>==============================<br>
==================<br>
--- lld/trunk/lib/ReaderWriter/<u></u>ELF/X86/X86LinkingContext.h (original)<br>
+++ lld/trunk/lib/ReaderWriter/<u></u>ELF/X86/X86LinkingContext.h Mon Feb 2<br>
00:00:04 2015<br>
@@ -36,6 +36,15 @@ public:<br>
return false;<br>
}<br>
}<br>
+<br>
+ void addDefaultSearchDirs(llvm::<u></u>Triple &baseTriple) override {<br>
+ if (_triple.getOS() == llvm::Triple::NetBSD &&<br>
+ baseTriple.getArch() == llvm::Triple::x86_64) {<br>
+ addSearchPath("=/usr/lib/i386"<u></u>);<br>
+ return;<br>
+ }<br>
+ ELFLinkingContext::<u></u>addDefaultSearchDirs(<u></u>baseTriple);<br>
+ }<br>
};<br>
} // end namespace elf<br>
} // end namespace lld<br>
<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
<br>
<br>
</blockquote></blockquote>
--<br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted<br>
by the Linux Foundation<br>
<br>
<br>
</blockquote></blockquote>
<br>
<br>
-- <br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation<br>
<br>
</div></div></blockquote></div><br></div></div>