Re: [PATCH] Two level comdat priorities in gold

[Xinliang David Li <davidxl at google dot com>]

On Mon, Jul 20, 2015 at 11:03 AM, Ian Lance Taylor <iant@google.com> wrote:
> On Mon, Jul 13, 2015 at 4:17 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>
>>     This patch implements a two level comdat function priority.  Comdats defined
>> in those objects with a named section ".gnu.comdat.low" are treated with a
>> lower priority.  If this patch is suitable, I could add compiler option
>> -fcomdat-priority-low which creates this named section in the object file.  For
>> testing, I have used the asm directive to create this named section.  Comdat
>> priority checking is enabled and honored with new option
>> --honor-comdat-priority.
>>
>> Context:  I have created this patch to support Module Level Multiversioning.
>> Multi versioning at module granularity is done by compiling a subset of modules
>> with advanced ISA instructions, supported on later generations of the target
>> architecture, via -m<isa> options and invoking the functions defined in these
>> modules with explicit checks for the ISA support via builtin functions,
>> __builtin_cpu_supports.  This mechanism has the unfortunate side-effect that
>> generated COMDAT candidates from these modules can contain these advanced
>> instructions and potentially âviolateâ ODR assumptions.  Choosing such a COMDAT
>> candidate over a generic one from a different module can cause SIGILL on
>> platforms where the advanced ISA is not supported.
>
> There are many different layers of ISAs.  I don't understand how you
> can address this problem with two layers of comdats.
>
> You are using a single section to affect all the section groups in the
> object file.  I suppose that works if you just want to check command
> line options, but what about the __optimize__ attribute, or function
> multi-versioning within a single object?
>

If a user uses __optimize__ attribute on a function to enable an ISA
extension, but does not guard the call of the function with ISA
feature testing, it is considered a user error. We only need to
improve the tool chain here to help users where he does not have any
other control -- basically when a common library function defined in
the header gets implicitly versioned due to the use of -mxxx option.
In other words, currently for C++ programs, using -mxxx on a module is
basically broken, and this patch tries to address that.


> What should happen if people use the -r option to combine several
> objects together?

Can the linker pick the higher prirority comdat?

>
> I don't quite understand what problem you are trying to solve.

This is actually a common problem when people uses -mxxx option
selectively for some modules. If such module (say a.cc) includes a
command header (say from libstdc++), and one of the comdat function
there gets optimized using the ISA extension. Such a copy of the
standard function may be picked by the linker and gets called in
another unrelated module (say b.cc) which does not use -mxxx. THe call
in b.cc won't have ISA feature testing, thus results in SIGILL.

> Is the
> problem building programs with advanced ISAs and running them on
> machines with older ISAs?

Yes -- but the programs only have selected modules enabled with
advanced ISA -- those modules will properly guard the functions
defined there with cpu_feature testing.

>   Or is the problem that the linker complains
> that functions that it expects to be the same wind up being different
> because they are compiled differently?  That is, is there a real
> problem, or is there a linker warning problem?

It is not a warning problem -- it is a real problem that hurts our
users many times (runtime failures depending on where the binary is
run) in the past.

David

>
> Ian