From: Jiri Slaby <jslaby@suse.cz>
To: Vegard Nossum <vegard.nossum@oracle.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Vegard Nossum <vegard.nossum@gmail.com>
Cc: Ming Lei <ming.lei@canonical.com>,
LKML <linux-kernel@vger.kernel.org>,
Steven Rostedt <srostedt@redhat.com>,
Richard Biener <RGuenther@suse.com>
Subject: Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers
Date: Fri, 14 Oct 2016 08:30:48 +0200 [thread overview]
Message-ID: <3ce66a53-334d-f854-f196-7263b725b71d@suse.cz> (raw)
In-Reply-To: <4ae97e84-5a9a-c862-1324-82586cd44913@oracle.com>
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
next prev parent reply other threads:[~2016-10-14 6:31 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-25 15:04 [PATCH] firmware: declare __{start,end}_builtin_fw as pointers Vegard Nossum
2016-06-25 21:06 ` Vegard Nossum
2016-06-26 9:24 ` Vegard Nossum
2016-06-26 17:17 ` Linus Torvalds
2016-10-14 5:52 ` Jiri Slaby
2016-10-14 6:25 ` Vegard Nossum
2016-10-14 6:30 ` Jiri Slaby [this message]
2016-06-28 12:23 George Spelvin
2016-06-28 20:08 ` Linus Torvalds
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3ce66a53-334d-f854-f196-7263b725b71d@suse.cz \
--to=jslaby@suse.cz \
--cc=RGuenther@suse.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ming.lei@canonical.com \
--cc=srostedt@redhat.com \
--cc=torvalds@linux-foundation.org \
--cc=vegard.nossum@gmail.com \
--cc=vegard.nossum@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).