[U-Boot] [PATCH 2/2] RFC: Let linker create phy array

[Albert ARIBAUD]

Le 10/02/2012 22:41, Mike Frysinger a écrit :
> On Friday 10 February 2012 15:57:50 Albert ARIBAUD wrote:
>> Le 10/02/2012 21:32, Mike Frysinger a écrit :
>>> On Friday 10 February 2012 14:39:12 Albert ARIBAUD wrote:
>>>> Le 07/02/2012 16:20, Mike Frysinger a écrit :
>>>>> On Monday 06 February 2012 16:01:56 Albert ARIBAUD wrote:
>>>>>> Le 06/02/2012 21:57, Mike Frysinger a écrit :
>>>>>>>> Is there a keep attribute like the linker has for sections?
>>>>>>>
>>>>>>> yes, __attribute__((used))
>>>>>>
>>>>>> What is the point in adding a 'static' qualifier and a ((used))
>>>>>> attribute, when not adding them in the first place gives the same
>>>>>> result?
>>>>>
>>>>> to control the visibility
>>>>
>>>> I don't understand what you mean with this. Can you please elaborate?
>>>
>>> no static means it has global elf visibility (other .c files can "extern"
>>> it, and you have to worry about symbol clashes):
>>> $ gcc -x c -c - -o test.o<<<'int foo;'&&   readelf -s test.o | grep foo
>>>
>>>        7: 0000000000000004     4 OBJECT  GLOBAL DEFAULT  COM foo
>>>
>>> static means it has local elf visibility (other files don't get access,
>>> and you don't have to worry about symbol clashes):
>>> $ gcc -x c -c - -o test.o<<<'static int foo;'&&   readelf -s test.o | grep
>>> foo
>>>
>>>        5: 0000000000000000     4 OBJECT  LOCAL  DEFAULT    3 foo
>>>
>>> imo, anything that should not be externally accessed should have
>>> "static". this is just good programming practice.
>>
>> I would agree 100% if the symbol was truly local, i.e. declared *and
>> used* locally. Here, however, it is used globally, by being gathered in
>> a global section to serve as an entry in a global array.
>
> except access is now explicitly gated, and symbol collisions are still
> prevented
>
>> The only interest of making the symbol static would indeed be to allow
>> reusing the symbol name elsewhere, which I think is quite improbable
>> considering the symbol was global so far.
>
> one or two might be global, but for the most part, they were all local.  look
> at his patch ... he deletes the "static" keyword in many places.

"Were" is precisely the point. They were indeed proper locals before the 
patch. With it, they are not any more -- they are not used locally, they 
are used globally.

> this style i'm proposing has been used in the kernel in subsystems, and some
> of them end up using the same variable name in diff modules.  like crypto/
> which uses "alg" as the name for all of its shash drivers.
>
>> So we add the static qualifier despite the object actually not being
>> static; and because the object is not actually static, that qualifier
>> causes a legit diagnostic; and to eliminate that diagnostic, we add an
>> 'unused' attribute. This I find less than good programming practice.
>
> no, the unused attribute was added *after* removing "static".  i'm proposing
> adding "used" so gcc won't strip it regardless of what else happens while
> retaining all other benefits that "static" brings us.

Correct, but it does not change my overall point that there is no point 
in adding a qualifier and an attribute to avoid name collisions that do 
not actually happen in the first place and that we can easily avoid by 
simply giving meaningful names to each of theses structs -- a practice 
which, again, is currently and successfully applied.

Plus, having globals is a good thing, because we can put names, so to 
speak, in the map file on what was collected in the section, whereas 
with statics, the section would just be just a black box and, were we to 
check if such entry was actually put in it, we would have to dig in the 
source code and build system.

> -mike

Amicalement,
-- 
Albert.