Messages in this thread |  | | Subject | Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers | From | Jiri Slaby <> | Date | Fri, 14 Oct 2016 08:30:48 +0200 |
| |
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@gmail.com> 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
|  |