[lld] r205163 - [core] support .gnu.linkonce sections

[Rui Ueyama]

On Mon, Mar 31, 2014 at 12:02 PM, Shankar Easwaran
<shankare at codeaurora.org>wrote:

>  On 3/31/2014 1:40 PM, Rui Ueyama wrote:
>
>  On Mon, Mar 31, 2014 at 11:31 AM, Rui Ueyama <ruiu at google.com> wrote:
>
>>   On Mon, Mar 31, 2014 at 11:21 AM, Shankar Easwaran <
>> shankare at codeaurora.org> wrote:
>>
>>>  On 3/31/2014 1:02 PM, Rui Ueyama wrote:
>>>
>>>
>>> I did not simply push back this patch but also pointed out some issues
>>> that were in your patch with a suggestion.
>>>
>>>  My suggestion was to merge it with COMDAT group section if they share
>>> the same semantics. If COMDAT groups and .gnu.linkonce sections need to be
>>> distinguished only to detect collisions of different types, i'd be great if
>>> we can compartment that detail in a small piece of code in Resolver, rather
>>> than spreading typeGroupComdat||typeGnuLinkOnce at every if's. Can't
>>> you use name prefix ".gnu.linkonce" to detect one section is a
>>> .gnu.linkonce section? If not, can't you add a new attribute for it?
>>>
>>>    I dont want to make a comparison on section names in the resolver.
>>>
>>
>>  Why? It might not feel very clean, but thinking that .gnu.linkonce is a
>> kind of reserved name, and this is for compatibility with old tools that
>> still produces .gnu.linkonce instead of COMDAT group sections, that may be
>> good enough.
>>
>
>  Or, alternatively, I'd add isComdatGroup (non-virtual) function to
> defined atom to check if it's a COMDAT group or .gnu.linkonce and use it in
> if's.
>
>
> In my opinion, Attributes are only added to Atoms, if there is no clean
> way to support functionality without that addition.
>
> Also, there is no need to make this a separate attribute of an Atom, as
> there is only one flavor that uses linkonce ie, .gnu.linkonce.
>

I'm not sure if you get my suggestion. This suggestion is basically to keep
your code as is, except that replacing conditions inside if's with
atom->isComdatGroup(). That would look better than writing the same
conditions in many places. We already have a precedence of a utility method
that is to be used inside a if condition, which is
DefinedAtom::occupiesDiskSpace().


> For Gnu linkonce, setting the contentType is one of the solutions that I
> can think of, here.
>
>
> Thanks
>
> Shankar Easwaran
>
> --
> 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/20140331/e2279985/attachment.html>