[lld] r227784 - [ELF] Determine default search directories from Context.
Rui Ueyama
ruiu at google.com
Sun Feb 1 22:45:25 PST 2015
On Sun, Feb 1, 2015 at 10:31 PM, Shankar Easwaran <shankare at codeaurora.org>
wrote:
> On 2/2/2015 12:24 AM, Rui Ueyama wrote:
>
>> On Sun, Feb 1, 2015 at 10:18 PM, Shankar Easwaran <
>> shankare at codeaurora.org>
>> wrote:
>>
>> The reason was more that the Context should own what is the default
>>> search
>>> directory. The ELFLinkingContext is using the baseTriple here not the
>>> real
>>> triple.
>>>
>>> Could you please elaborate why you think it should?
>>
>> My point was that this patch scattered the logic that used to be in one
>> function and also added a new virtual public function that we used to not
>> need. It doesn't seem like a good tradeoff.
>>
> Are you thinking that the default search path for all the ELF
> targets(aarch64, armv7, etc) be housed in the Driver ? I would disagree.
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.
>
>>
>> IMO, Adding target specific search directory in the Gnu driver does not
>>> make the code clean.
>>>
>>> Shankar Easwaran
>>>
>>>
>>> On 2/2/2015 12:11 AM, Rui Ueyama wrote:
>>>
>>> Does this actually improve the code quality? You added a new virtual
>>>> function to the linking context, but the linking context are still using
>>>> the triple to make a decision. This patch split a private function in
>>>> the
>>>> driver into two, and added them to the linking context as public member
>>>> functions. Seems it's slightly worse than before.
>>>>
>>>> On Sun, Feb 1, 2015 at 10:00 PM, Shankar Easwaran <
>>>> shankare at codeaurora.org>
>>>> wrote:
>>>>
>>>> Author: shankare
>>>>
>>>>> Date: Mon Feb 2 00:00:04 2015
>>>>> New Revision: 227784
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=227784&view=rev
>>>>> Log:
>>>>> [ELF] Determine default search directories from Context.
>>>>>
>>>>> Target specific LinkingContext's determine the default search
>>>>> directory.
>>>>>
>>>>> No change in functionality.
>>>>>
>>>>> Modified:
>>>>> lld/trunk/include/lld/Driver/Driver.h
>>>>> lld/trunk/include/lld/ReaderWriter/ELFLinkingContext.h
>>>>> lld/trunk/lib/Driver/GnuLdDriver.cpp
>>>>> lld/trunk/lib/ReaderWriter/ELF/X86/X86LinkingContext.h
>>>>>
>>>>> Modified: lld/trunk/include/lld/Driver/Driver.h
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/
>>>>> Driver/Driver.h?rev=227784&r1=227783&r2=227784&view=diff
>>>>>
>>>>> ============================================================
>>>>> ==================
>>>>> --- lld/trunk/include/lld/Driver/Driver.h (original)
>>>>> +++ lld/trunk/include/lld/Driver/Driver.h Mon Feb 2 00:00:04 2015
>>>>> @@ -87,10 +87,6 @@ private:
>>>>> static bool applyEmulation(llvm::Triple &triple,
>>>>> llvm::opt::InputArgList &args,
>>>>> raw_ostream &diag);
>>>>> - static void addPlatformSearchDirs(ELFLinkingContext &ctx,
>>>>> - llvm::Triple &triple,
>>>>> - llvm::Triple &baseTriple);
>>>>> -
>>>>> GnuLdDriver() LLVM_DELETED_FUNCTION;
>>>>> };
>>>>>
>>>>>
>>>>> Modified: lld/trunk/include/lld/ReaderWriter/ELFLinkingContext.h
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/
>>>>> ReaderWriter/
>>>>> ELFLinkingContext.h?rev=227784&r1=227783&r2=227784&view=diff
>>>>>
>>>>> ============================================================
>>>>> ==================
>>>>> --- lld/trunk/include/lld/ReaderWriter/ELFLinkingContext.h (original)
>>>>> +++ lld/trunk/include/lld/ReaderWriter/ELFLinkingContext.h Mon Feb 2
>>>>> 00:00:04 2015
>>>>> @@ -291,6 +291,11 @@ public:
>>>>> bool alignSegments() const { return _alignSegments; }
>>>>> void setAlignSegments(bool align) { _alignSegments = align; }
>>>>>
>>>>> + /// \brief add platform specific search directories.
>>>>> + virtual void addDefaultSearchDirs(llvm::Triple & /*triple*/) {
>>>>> + addSearchPath("=/usr/lib");
>>>>> + }
>>>>> +
>>>>> private:
>>>>> ELFLinkingContext() LLVM_DELETED_FUNCTION;
>>>>>
>>>>>
>>>>> Modified: lld/trunk/lib/Driver/GnuLdDriver.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Driver/
>>>>> GnuLdDriver.cpp?rev=227784&r1=227783&r2=227784&view=diff
>>>>>
>>>>> ============================================================
>>>>> ==================
>>>>> --- lld/trunk/lib/Driver/GnuLdDriver.cpp (original)
>>>>> +++ lld/trunk/lib/Driver/GnuLdDriver.cpp Mon Feb 2 00:00:04 2015
>>>>> @@ -311,18 +311,6 @@ bool GnuLdDriver::applyEmulation(llvm::T
>>>>> return true;
>>>>> }
>>>>>
>>>>> -void GnuLdDriver::addPlatformSearchDirs(ELFLinkingContext &ctx,
>>>>> - llvm::Triple &triple,
>>>>> - llvm::Triple &baseTriple) {
>>>>> - if (triple.getOS() == llvm::Triple::NetBSD &&
>>>>> - triple.getArch() == llvm::Triple::x86 &&
>>>>> - baseTriple.getArch() == llvm::Triple::x86_64) {
>>>>> - ctx.addSearchPath("=/usr/lib/i386");
>>>>> - return;
>>>>> - }
>>>>> - ctx.addSearchPath("=/usr/lib");
>>>>> -}
>>>>> -
>>>>> #define LLVM_TARGET(targetName) \
>>>>> if ((p = elf::targetName##LinkingContext::create(triple)))
>>>>> return p;
>>>>>
>>>>> @@ -403,8 +391,9 @@ bool GnuLdDriver::parse(int argc, const
>>>>> for (auto libDir : parsedArgs->filtered(OPT_L))
>>>>> ctx->addSearchPath(libDir->getValue());
>>>>>
>>>>> + // Add the default search directory specific to the target.
>>>>> if (!parsedArgs->hasArg(OPT_nostdlib))
>>>>> - addPlatformSearchDirs(*ctx, triple, baseTriple);
>>>>> + ctx->addDefaultSearchDirs(baseTriple);
>>>>>
>>>>> // Handle --demangle option(For compatibility)
>>>>> if (parsedArgs->getLastArg(OPT_demangle))
>>>>>
>>>>> Modified: lld/trunk/lib/ReaderWriter/ELF/X86/X86LinkingContext.h
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/
>>>>> ReaderWriter/ELF/X86/
>>>>> X86LinkingContext.h?rev=227784&r1=227783&r2=227784&view=diff
>>>>>
>>>>> ============================================================
>>>>> ==================
>>>>> --- lld/trunk/lib/ReaderWriter/ELF/X86/X86LinkingContext.h (original)
>>>>> +++ lld/trunk/lib/ReaderWriter/ELF/X86/X86LinkingContext.h Mon Feb 2
>>>>> 00:00:04 2015
>>>>> @@ -36,6 +36,15 @@ public:
>>>>> return false;
>>>>> }
>>>>> }
>>>>> +
>>>>> + void addDefaultSearchDirs(llvm::Triple &baseTriple) override {
>>>>> + if (_triple.getOS() == llvm::Triple::NetBSD &&
>>>>> + baseTriple.getArch() == llvm::Triple::x86_64) {
>>>>> + addSearchPath("=/usr/lib/i386");
>>>>> + return;
>>>>> + }
>>>>> + ELFLinkingContext::addDefaultSearchDirs(baseTriple);
>>>>> + }
>>>>> };
>>>>> } // end namespace elf
>>>>> } // end namespace lld
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>>>>
>>>>> --
>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
>>> by the Linux Foundation
>>>
>>>
>>>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
> by the Linux Foundation
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150201/2a903e4b/attachment.html>
More information about the llvm-commits
mailing list