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

From: Jiri Slaby
Date: Fri Oct 14 2016 - 02:31:10 EST


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