Re: [patch 2/8] compiler-gcc.h: add more comments to RELOC_HIDE

Re: [patch 2/8] compiler-gcc.h: add more comments to RELOC_HIDE

[Ingo Molnar]

* Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Saturday 10 January 2009 11:10:53 akpm@linux-foundation.org wrote:
> > From: Andi Kleen <andi@firstfloor.org>
> ...
> > + * This is needed because the C standard makes it undefined to do
> > + * pointer arithmetic on "objects" outside their boundaries and the
> > + * gcc optimizers assume this is the case. In particular they
> > + * assume such arithmetic does not wrap.

that is a rather useless comment without real examples.

> ...
>
> That last sentence seems misleading to me; the case I know of gcc 
> hurting us is the powerpc literal strcpy case.  See prom_init.c where 
> vars are not yet mapped at KERNEL_BASE, so they offset them manually: 
> gcc turned a strcpy(dest, "literal"+KERNEL_BASE) into memcpy(dest, 
> "literal"+KERNEL_BASE, 7-KERNEL_BASE).  Boom.
> 
> Richard came up with the RELOC_HIDE macro, to future-proof against other 
> such optimizations.  I can't remember if I was clever enough to use it 
> for per-cpu on my own, or whether he pointed it out.
> 
> I hope that clarifies once and for all!

Note that 32-bit x86 lived just fine for years without RELOC_HIDE() - and 
it has similar large-offset pointer arithmetics both for PAGE_OFFSET and 
for per-cpu accesses.

The obscurity and undocumentedness of RELOC_HIDE() turned it a bit of a 
voodoo programming construct: it got introduced due to unspecified fear 
but there's no real documented specifics of actual GCC breakage caused by 
it in the field, other than a constant string put into a percpu area - 
which is pretty stupid to do to begin with.

So it's a bit of a mystery construct, and if we add a comment, here are a 
few examples that brings the above textbook definition closer to reality, 
examples of what GCC _could_ do and how it could break without RELOC_HIDE 
- and this might be much more useful for the commit log than the obscure 
C-lawyering paragraph above:

1) String operations

 For example if we do something like:

        strcpy(tmp, "abc" + LARGE_OFFSET)

 GCC could assume that "abc" is a 4 bytes long string, and could assume 
 that LARGE_OFFSET points within it, and could optimize this into:

	memcpy(tmp, "abc" + LARGE_OFFSET, 4 - LARGE_OFFSET)

 [ This is a bit far-fetched, as it would need us to put constant strings 
   into PER_CPU areas - but it's not actually wrong to do it so if it 
   happens it's nasty. OTOH, very clear crashes will point to it, so not 
   really relevant. ]

2) Special structure sizes

 For example we want to load a value from "&var + LARGE_OFFSET" (common 
 percpu construct). If 'var' is a C structure that happens to have a size 
 of 255 bytes, then GCC could legitimately assume that 'LARGE_OFFSET' is 
 0..255 [inclusive], and could optimize LARGE_OFFSET to be truncated to a 
 single byte.

3) Memory object aliasing, alias analysis

 A "bad" aliasing optimization happens when GCC detects that a %gs 
 referenced and a kernel linear pointer has the same value - and thus 
 mis-optimizes it assuming that already loaded values must be the same. 

 [ does not really happen as both in the current and in the zerobased case 
   the two memory spaces are clearly separated. ]

 Or if GCC does not recognize that aliased accesses to the same object 
 involve the same object - and optimizes things assuming that it's two 
 separate objects - and could schedule instructions in a mixed up way, 
 corrupting the object.

 Not a real issue either here: the %gs space is very separate from the 
 kernel linear address space, we never access the same object via two
 similar-looking pointers at once. We just set up the percpu area and
 leave those linear addresses alone.

But RELOC_HIDE() does not actually _prevent_ any useful optimizations from 
being done - so it does not really hurt. But the comment added by this 
patch is rather misleading as it's formulated way too generic way without 
pointing out the obscurity of these cases, and thus hurts the end result 
more than it helps.

	Ingo

Re: [patch 2/8] compiler-gcc.h: add more comments to RELOC_HIDE

[Christoph Lameter]

On Sat, 10 Jan 2009, Ingo Molnar wrote:

> 2) Special structure sizes
>
>  For example we want to load a value from "&var + LARGE_OFFSET" (common
>  percpu construct). If 'var' is a C structure that happens to have a size
>  of 255 bytes, then GCC could legitimately assume that 'LARGE_OFFSET' is
>  0..255 [inclusive], and could optimize LARGE_OFFSET to be truncated to a
>  single byte.


But that is not what is occurring. We do not do &var + LARGE_OFFSET.
Instead one does

	((void *)&var) + LARGE_OFFSET

or
	((unsigned long)&var) + LARGE_OFFSET


The cast should cause the C compiler to drop all assumptions about size.
And the definition of RELOC_HIDE for icc and generic compilers in the tree
show that this is working.

Re: [patch 2/8] compiler-gcc.h: add more comments to RELOC_HIDE

[Linus Torvalds]

On Thu, 15 Jan 2009, Rusty Russell wrote:

> On Tuesday 13 January 2009 04:03:49 Christoph Lameter wrote:
> > But that is not what is occurring. We do not do &var + LARGE_OFFSET.
> > Instead one does
> > 
> > 	((void *)&var) + LARGE_OFFSET
> > 
> > or
> > 	((unsigned long)&var) + LARGE_OFFSET
> > 
> > The cast should cause the C compiler to drop all assumptions about size.
> 
> No, and that's the point.  Sorry, at this point you need to talk to a gcc expert.  As I have said, I did and I believe what he told me.

Yeah, I personally believe in the "should cause the C compiler" part, but 
gcc doesn't work that way. It will remember where the value came from, 
even when the pointer has been cast to something else.

			Linus

Re: [patch 2/8] compiler-gcc.h: add more comments to RELOC_HIDE

[Christoph Lameter]

On Wed, 14 Jan 2009, Linus Torvalds wrote:

> > > The cast should cause the C compiler to drop all assumptions about size.
> >
> > No, and that's the point.  Sorry, at this point you need to talk to a gcc expert.  As I have said, I did and I believe what he told me.
>
> Yeah, I personally believe in the "should cause the C compiler" part, but
> gcc doesn't work that way. It will remember where the value came from,
> even when the pointer has been cast to something else.

If so then the compiler is crap. Casting a pointer to long needs to
get a scalar without assumptions about the size of the structure that the
pointer was going to remaining a factor.

All of this originated in some discussion in the 2.5.X days. Could we make
sure that this is indeed the case?I have never seen any ill effect from
the casts to long nor do the failure scenarios given here make too much
sense (like adding a pointer to the address of a constant string passed as
the source argument to strcpy????).

>From the include/compiler*.h files it looks as if we are supporting 3
classes of compilers. Of those only gcc has this sickness.

Re: [patch 2/8] compiler-gcc.h: add more comments to RELOC_HIDE

[Andi Kleen]

Christoph Lameter wrote:

> 
> From the include/compiler*.h files it looks as if we are supporting 3
> classes of compilers. Of those only gcc has this sickness.

The other compilers are essentially un (or very little-)tested. There
are undoubtedly some problems there that just noone hit due to lack
of testing.

-andi

Re: [patch 2/8] compiler-gcc.h: add more comments to RELOC_HIDE

[Christoph Lameter]

On Thu, 15 Jan 2009, Rusty Russell wrote:

> > The cast should cause the C compiler to drop all assumptions about size.
>
> No, and that's the point.  Sorry, at this point you need to talk to a gcc expert.  As I have said, I did and I believe what he told me.

The gcc expert that created this measss is cced on this thread and so
far he not spoken up. Richard?

Re: [patch 2/8] compiler-gcc.h: add more comments to RELOC_HIDE

[Richard Henderson]

Christoph Lameter wrote:
> On Thu, 15 Jan 2009, Rusty Russell wrote:
> 
>>> The cast should cause the C compiler to drop all assumptions about size.
>> No, and that's the point.  Sorry, at this point you need to talk to a gcc expert.  As I have said, I did and I believe what he told me.
> 
> The gcc expert that created this measss is cced on this thread and so
> far he not spoken up. Richard?

It has been a long time, and I don't recall all of the assumptions 
involved from the time.

It was probably a combination of object size assumptions, as well as 
problems with relocations.  Stuff like "int foo" is known to be 
allocated within the small data structure, and thus various types of 
small-data-section relocations are valid for it.  Then we do stuff like 
"(void *)&foo - large_constant" which don't work with those sorts of 
relocations.

I didn't explore the space of possible solutions, merely gave Rusty a 
solution that I knew would work, and would never fail because the 
compiler would never look through the asm.

I wouldn't be surprised if the compiler thought "(long)&foo - 
large_constant" could be put back together into a small-data relocation, 
simply because at the level at which that optimization is performed, 
we've thrown away type data like long and void*; we only have modes.

Why are you wanting to change this at all?  It works as it is.


r~

Re: [patch 2/8] compiler-gcc.h: add more comments to RELOC_HIDE

[Christoph Lameter]

On Thu, 15 Jan 2009, Richard Henderson wrote:

> I didn't explore the space of possible solutions, merely gave Rusty a solution
> that I knew would work, and would never fail because the compiler would never
> look through the asm.
>
> I wouldn't be surprised if the compiler thought "(long)&foo - large_constant"
> could be put back together into a small-data relocation, simply because at the
> level at which that optimization is performed, we've thrown away type data
> like long and void*; we only have modes.

We are talking about

(long)&foo + long_variable

Are you saying that the compiler will be ignoring the high bits in
variable because of the size of foo?

> Why are you wanting to change this at all?  It works as it is.

It looks like its useless and more an indication of either a broken
compiler or wrong assumptions about the compiler. Removing RELOC_HIDE
should allow the compiler to freely optimize the per cpu address
calculations.

Re: [patch 2/8] compiler-gcc.h: add more comments to RELOC_HIDE

[Richard Henderson]

Christoph Lameter wrote:
> On Thu, 15 Jan 2009, Richard Henderson wrote:
> 
>> I didn't explore the space of possible solutions, merely gave Rusty a solution
>> that I knew would work, and would never fail because the compiler would never
>> look through the asm.
>>
>> I wouldn't be surprised if the compiler thought "(long)&foo - large_constant"
>> could be put back together into a small-data relocation, simply because at the
>> level at which that optimization is performed, we've thrown away type data
>> like long and void*; we only have modes.
> 
> We are talking about
> 
> (long)&foo + long_variable
> 
> Are you saying that the compiler will be ignoring the high bits in
> variable because of the size of foo?

No, I'm saying that all those high bits will be passed along and won't
fit in the 16-bit relocation that'll come out of the assembler, leading
to a hard linker error.

> It looks like its useless and more an indication of either a broken
> compiler or wrong assumptions about the compiler. Removing RELOC_HIDE
> should allow the compiler to freely optimize the per cpu address
> calculations.

Something I'm pretty sure we don't want the compiler to be able to do.


r~

Re: [patch 2/8] compiler-gcc.h: add more comments to RELOC_HIDE

[Christoph Lameter]

On Mon, 19 Jan 2009, Richard Henderson wrote:

> > We are talking about
> >
> > (long)&foo + long_variable
> >
> > Are you saying that the compiler will be ignoring the high bits in
> > variable because of the size of foo?
>
> No, I'm saying that all those high bits will be passed along and won't
> fit in the 16-bit relocation that'll come out of the assembler, leading
> to a hard linker error.

Why would a 16 bit relocation be generated if the compiler knows that a 32
bit/64 bit entity is added to the address of a variable?

> > It looks like its useless and more an indication of either a broken
> > compiler or wrong assumptions about the compiler. Removing RELOC_HIDE
> > should allow the compiler to freely optimize the per cpu address
> > calculations.
>
> Something I'm pretty sure we don't want the compiler to be able to do.

Other compilers (like icc) seem to have no problem with it. Why not?