Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers

[Jiri Slaby]

On 10/14/2016, 08:25 AM, Vegard Nossum wrote:
> On 10/14/2016 07:52 AM, Jiri Slaby wrote:
>> On 06/26/2016, 07:17 PM, Linus Torvalds wrote:
>>> On Sun, Jun 26, 2016 at 2:24 AM, Vegard Nossum
>>> <vegard.nossum@xxxxxxxxx> wrote:
>>>>
>>>> This is the best I could come up with: assuming gcc is not allowed to
>>>> reason about what's inside the asm(), this is the only way I could
>>>> think of to lose the array information without incurring unnecessary
>>>> overheads. It should also be relatively safe as there is no way to
>>>> accidentally use the underlying arrays without explicitly declaring
>>>> them.
>>>
>>> Ugh. I worry about the other places where we do things like this,
>>> depending on the linker just assigning the addresses and us being able
>>> to compare them.
>>>
>>> If there is a compiler option to disable this optimization, I would
>>> almost prefer that.. Because we really do have a whole slew of these
>>> things.
>>
>> Any update on this? Couple months later and I still hit this.
> 
> I didn't find any compiler flags or anything that would let us get by
> with the current code.

FWIW, this changed with 73447cc5d17 in gcc. There are no flags added by
the commit, so I assume this sole optimization cannot be turned off.

> I have a branch fixing a bunch of these (many of them in tracing code,
> like you wrote) using my old approach (declaring the arrays with a
> macro) but it looks so ugly I got discouraged from submitting any of it.
> I also don't have a great way to actually test a lot of the fixes.
> 
> Could something like this work?
> 
> #define DECLARE_EXTERNAL_ARRAY(type, name) \
>     extern type __##name##_start;
>     static inline type name##_start(void) \
>     { \
>         type tmp = __##name##_start; \
>         OPTIMIZER_HIDE_VAR(tmp); \
>         return tmp; \
>     } \
>     static inline type name##_end(void) \
>     { \
>         type tmp = __##name##_end; \
>         OPTIMIZER_HIDE_VAR(tmp); \
>         return tmp; \
>     } \
>     static inline size_t name##_size(void) \
>     { \
>         return name##_end() - name##_start(); \
>     }
> 
> #define ext_for_each(var, name) \
>     for (var = name##_start(); var != name##_end(); var++)
> 
> and then for the firmware case you would do
> 
> DECLARE_EXTERNAL_ARRAY(struct builtin_fw, builtin_fw);
> 
> struct builtin_fw *fw;
> ext_for_each(fw, builtin_fw)
>     ...;
> 
> You'd also have to adjust the names in the linker script accordingly.
> 
> The points here being:
> 
> 1) DECLARE_*() follows the kernel convention (you get what you expect,
> more or less)
> 
> 2) the real variables defined in the linker script are hidden behind
> some generated names so you don't use them by accident
> 
> 3) no need to sprinkle your code with OPTIMIZER_HIDE_VAR() or anything
> else, but you do need to use function calls to access the variables
> (e.g. firmware_start() and firmware_end()).
> 
> What do you think?

FWIW LGTM

thanks,
-- 
js
suse labs