Re: [Xen-devel] [PATCH 6 of 6] x86: explicitly mark an __initdata variable as used

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.04.12 at 16:11, Tim Deegan <tim@xxxxxxx> wrote:
> At 14:44 +0100 on 05 Apr (1333637053), Jan Beulich wrote:
>> >>> On 05.04.12 at 14:07, Tim Deegan <tim@xxxxxxx> wrote:
>> > x86: explicitly mark an __initdata variable as used.
>> > 
>> > This stops LLVM from replacing it with a different, auto-generated
>> > variable as part of an optimization.  (The auto-generated variable
>> > ends up in the normal data section, failing the check that this
>> > file only contains __initdata vars).
>> > 
>> > Signed-off-by: Tim Deegan <tim@xxxxxxx>
>> > 
>> > diff -r 5101e5ed2473 -r 5a2f5ab5128e xen/arch/x86/domain_build.c
>> > --- a/xen/arch/x86/domain_build.c  Thu Apr 05 12:55:54 2012 +0100
>> > +++ b/xen/arch/x86/domain_build.c  Thu Apr 05 12:55:54 2012 +0100
>> > @@ -129,7 +129,7 @@ static struct page_info * __init alloc_c
>> >      struct domain *d, unsigned long max_pages)
>> >  {
>> >      static unsigned int __initdata last_order = MAX_ORDER;
>> > -    static unsigned int __initdata memflags = MEMF_no_dma;
>> > +    static unsigned int __initdata __attribute__((used)) memflags = 
> MEMF_no_dma;
>> 
>> Without a code comment, the mere fact that this is (a) totally
>> non-obvious, (b) being done differently for two neighboring
>> variables of otherwise the exact same kind, and (c) probably
>> a compiler bug makes it quite likely that your change will get
>> removed again by a future commit.
> 
> Sorry, yes this deserves a code comment.  I'll add one. 
> 
> I agree that this is a compiler bug, but the LLVM developers disagree,
> and I have limited time for arguing with compiler devs.

Which I can well understand.

>> Furthermore, it being needed on one but not the other variable
>> makes it highly likely that the same issue could surface at any
>> time here or elsewhere in the code.
> 
> I considered putting the __attribute__((used)) into the definition of
> __initdata but thought it was more invasive.  I'm happy to reconsider. 

That's a reasonable idea imo, and you may want to do this for
other section placement directives (__read_mostly) too. And then
ideally only for clang (at least for the time being). Maybe we should
even have a __section()-like thing as Linux has (just that we'd likely
want one for text and one for data, so that the attribute wouldn't
needlessly get applied to functions).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel