[PATCH] D40849: CodeGen: support an extension to pass linker options on ELF

[Rui Ueyama via llvm-commits]

On Fri, Dec 15, 2017 at 2:48 PM, Saleem Abdulrasool <compnerd at compnerd.org>
wrote:

> On Fri, Dec 15, 2017 at 11:09 AM, Rui Ueyama <ruiu at google.com> wrote:
>
>> On Fri, Dec 15, 2017 at 11:07 AM, Saleem Abdulrasool via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>>> compnerd added inline comments.
>>>
>>>
>>> ================
>>> Comment at: test/Feature/elf-linker-options.ll:6
>>> +
>>> +!0 = !{!"spaced", !"option"}
>>> +!1 = !{!"nospace"}
>>> ----------------
>>> compnerd wrote:
>>> > jhenderson wrote:
>>> > > ruiu wrote:
>>> > > > Imagine that these two strings are pathnames.  Pathnames can
>>> contain any character other than \0. That mean you can't distinguish
>>> "spaced options" from "spaced" and "options, which is bad. You should allow
>>> only one string.
>>> > > The way I see it, this is intended to be interpreted as a single
>>> option. However, I think this demonstrates why a key-value pair is
>>> important.
>>> > Yes, this is a single value.  " spaced option".  I don't remember what
>>> the issue was with trying to merge the entire spaced option into a single
>>> string.  But, that would simplify the logic here.  I think that we should
>>> go ahead and expect that the frontend will generate a single entry for
>>> that.  The key/value wouldn't help with this if the spaces were removed in
>>> the MDNode itself.
>>> Wait, I forgot that I modelled this after the existing MachO and COFF
>>> behaviors.  I think that changing the behavior of this documented interface
>>> is less desirable.  If we want to have a new way to pass along the options,
>>> that is fine, but should be a separate change that unifies the behavior
>>> across all three object file formats.
>>
>>
>> I don't think we should mimic an interface that could produce ambiguous
>> string. If you think these interfaces need to be coherent all the time,
>> please fix MachO and COFF models first
>>
>
> I understand that you are concerned about the parsing of the options in
> the linker.  I think that the issue that you are raising is better
> addressed in the frontend rather than in the backend.  This is purely the
> passthru from the frontend to the backend, and I don't see why the LLVM IR
> needs to be modified for that as it can support either approach.
>

I mean we shouldn't define an interface that is too easy to use in a wrong
way. I think there's no valid use of passing two separate string, expecting
that these strings are concatenated with a space. So I don't see a need to
provide that feature in the first place.


>
>>
>>> Repository:
>>>   rL LLVM
>>>
>>> https://reviews.llvm.org/D40849
>>>
>>>
>>>
>>>
> --
> Saleem Abdulrasool
> compnerd (at) compnerd (dot) org
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171215/9879ba52/attachment.html>