[PATCH] x86: remove __phys_reloc_hide

[PATCH] x86: remove __phys_reloc_hide

[Namhyung Kim]

remove unnecessary use of RELOC_HIDE(). It only does simple addition of ptr
and offset, and in this case, offset 0, does nothing. It does NOT do anything
with linker relocation things. I could find no reason to use it.

The only user of __phys_reloc_hide() was __pa_symbol() so it can be removed
safely here.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 arch/x86/include/asm/page.h          |    5 ++---
 arch/x86/include/asm/page_32.h       |    1 -
 arch/x86/include/asm/page_64_types.h |    1 -
 3 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
index 625c3f0..3da2a8e 100644
--- a/arch/x86/include/asm/page.h
+++ b/arch/x86/include/asm/page.h
@@ -35,9 +35,8 @@ static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
 
 #define __pa(x)		__phys_addr((unsigned long)(x))
 #define __pa_nodebug(x)	__phys_addr_nodebug((unsigned long)(x))
-/* __pa_symbol should be used for C visible symbols.
-   This seems to be the official gcc blessed way to do such arithmetic. */
-#define __pa_symbol(x)	__pa(__phys_reloc_hide((unsigned long)(x)))
+/* __pa_symbol should be used for C visible symbols. */
+#define __pa_symbol(x)	__pa(x)
 
 #define __va(x)			((void *)((unsigned long)(x)+PAGE_OFFSET))
 
diff --git a/arch/x86/include/asm/page_32.h b/arch/x86/include/asm/page_32.h
index da4e762..e78e52a 100644
--- a/arch/x86/include/asm/page_32.h
+++ b/arch/x86/include/asm/page_32.h
@@ -15,7 +15,6 @@ extern unsigned long __phys_addr(unsigned long);
 #else
 #define __phys_addr(x)		__phys_addr_nodebug(x)
 #endif
-#define __phys_reloc_hide(x)	RELOC_HIDE((x), 0)
 
 #ifdef CONFIG_FLATMEM
 #define pfn_valid(pfn)		((pfn) < max_mapnr)
diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
index 7639dbf..5a63066 100644
--- a/arch/x86/include/asm/page_64_types.h
+++ b/arch/x86/include/asm/page_64_types.h
@@ -59,7 +59,6 @@ extern unsigned long max_pfn;
 extern unsigned long phys_base;
 
 extern unsigned long __phys_addr(unsigned long);
-#define __phys_reloc_hide(x)	(x)
 
 #define vmemmap ((struct page *)VMEMMAP_START)
 
-- 
1.7.0.4

Re: [PATCH] x86: remove __phys_reloc_hide

[Andi Kleen]

Namhyung Kim <namhyung@gmail.com> writes:

> remove unnecessary use of RELOC_HIDE(). It only does simple addition of ptr
> and offset, and in this case, offset 0, does nothing. It does NOT do anything
> with linker relocation things. I could find no reason to use it.

It's for the benefit of the compiler, we've had miscompilations
due to undefined overflow for addresses in the past. The optimizer
assumes this won't happen.

Given the x86-64 version normally doesn't overflow, but it's
still safer to have it.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] x86: remove __phys_reloc_hide

[Namhyung Kim]

2010-08-09 (월), 08:22 +0200, Andi Kleen:

> It's for the benefit of the compiler, we've had miscompilations
> due to undefined overflow for addresses in the past. The optimizer
> assumes this won't happen.
> 
> Given the x86-64 version normally doesn't overflow, but it's
> still safer to have it.
> 
> -Andi
> 

Hi, 

I'm not talking about the RELOC_HIDE itself. I do know we need it for
some specific case, ie. percpu. But in this case __pa_symbol(x) is
expanded to RELOC_HIDE((unsigned long)(x), 0) which does nothing
meaningful. I believe the overflow is not a concern here.

-- 
Regards,
Namhyung Kim

Re: [PATCH] x86: remove __phys_reloc_hide

[Andi Kleen]

> I'm not talking about the RELOC_HIDE itself. I do know we need it for
> some specific case, ie. percpu. But in this case __pa_symbol(x) is
> expanded to RELOC_HIDE((unsigned long)(x), 0) which does nothing
> meaningful. I believe the overflow is not a concern here.

It hides the value conversion from the compiler through asm()

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] x86: remove __phys_reloc_hide

[Namhyung Kim]

> It hides the value conversion from the compiler through asm()
> 
> -Andi
> 

Yes, indeed. But for what? __pa_symbol() is just used to get the address
of some linker symbols in forms of unsigned long which has same bit
representation as pointer in x86 (and all supported archs). So do we
still need it or am I missing something?


-- 
Regards,
Namhyung Kim

Re: [PATCH] x86: remove __phys_reloc_hide

[Andi Kleen]

On Mon, Aug 09, 2010 at 04:04:45PM +0900, Namhyung Kim wrote:
> > It hides the value conversion from the compiler through asm()
> > 
> > -Andi
> > 
> 
> Yes, indeed. But for what? __pa_symbol() is just used to get the address
> of some linker symbols in forms of unsigned long which has same bit
> representation as pointer in x86 (and all supported archs). So do we
> still need it or am I missing something?

The original reason was that the C standard allows the compiler
to make some assumptions on the pointer arithmetic that is done 
on symbol addresses (e.g. no wrapping). This is exploited
by the optimizer in the compiler to generate better code.

This lead to a miscompilation on PowerPC a couple of years back at 
least with the va->pa conversion.

After that RELOC_HIDE was introduced after funelling the
symbol address through an empty asm statement was recommended 
as the official way to do this by the gcc developers.

I think x86-64 does not normally wrap here, but it's 
still safer to do it this way.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] x86: remove __phys_reloc_hide

[Namhyung Kim]

2010-08-09 (월), 09:22 +0200, Andi Kleen:

> The original reason was that the C standard allows the compiler
> to make some assumptions on the pointer arithmetic that is done 
> on symbol addresses (e.g. no wrapping). This is exploited
> by the optimizer in the compiler to generate better code.
> 
> This lead to a miscompilation on PowerPC a couple of years back at 
> least with the va->pa conversion.
> 
> After that RELOC_HIDE was introduced after funelling the
> symbol address through an empty asm statement was recommended 
> as the official way to do this by the gcc developers.
> 
> I think x86-64 does not normally wrap here, but it's 
> still safer to do it this way.
> 
> -Andi

OK, then. Thanks for the comment.

p.s. The funny thing I found is there's no use of RELOC_HIDE on
arch/powerpc. Hmm...

-- 
Regards,
Namhyung Kim

Re: [PATCH] x86: remove __phys_reloc_hide

[Ingo Molnar]

* Namhyung Kim <namhyung@gmail.com> wrote:

> remove unnecessary use of RELOC_HIDE(). It only does simple addition of ptr
> and offset, and in this case, offset 0, does nothing. It does NOT do anything
> with linker relocation things. I could find no reason to use it.
> 
> The only user of __phys_reloc_hide() was __pa_symbol() so it can be removed
> safely here.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  arch/x86/include/asm/page.h          |    5 ++---
>  arch/x86/include/asm/page_32.h       |    1 -
>  arch/x86/include/asm/page_64_types.h |    1 -
>  3 files changed, 2 insertions(+), 5 deletions(-)

We do this as a general Voodoo barrier against GCC miscompilations.

You are right that it's largely moot by today (and especially so on x86 - i 
only remember a single instance of miscompilation that Rusty mentioned few 
years ago, and that was on powerpc), but the wrapper is simple enough, so 
unless there's some real tangible improvement in the binary output we might as 
well keep it.

Peter, what do you think?

	Ingo

Re: [PATCH] x86: remove __phys_reloc_hide

[H. Peter Anvin]

On 08/09/2010 12:22 AM, Andi Kleen wrote:
> On Mon, Aug 09, 2010 at 04:04:45PM +0900, Namhyung Kim wrote:
>>> It hides the value conversion from the compiler through asm()
>>>
>>> -Andi
>>>
>>
>> Yes, indeed. But for what? __pa_symbol() is just used to get the address
>> of some linker symbols in forms of unsigned long which has same bit
>> representation as pointer in x86 (and all supported archs). So do we
>> still need it or am I missing something?
> 
> The original reason was that the C standard allows the compiler
> to make some assumptions on the pointer arithmetic that is done 
> on symbol addresses (e.g. no wrapping). This is exploited
> by the optimizer in the compiler to generate better code.
> 
> This lead to a miscompilation on PowerPC a couple of years back at 
> least with the va->pa conversion.
> 
> After that RELOC_HIDE was introduced after funelling the
> symbol address through an empty asm statement was recommended 
> as the official way to do this by the gcc developers.
> 
> I think x86-64 does not normally wrap here, but it's 
> still safer to do it this way.
> 

We pass -fno-strict-overflow to the kernel now, which takes care of the
underlying problem, at least for current versions of gcc.  Unfortunately
we still have people who want to use very old gcc versions to compile
the kernel, so it's probably better to leave it in at least until we
formally kill off support for gcc 3.

	-hpa

Re: [PATCH] x86: remove __phys_reloc_hide

[H. Peter Anvin]

On 08/09/2010 01:07 AM, Ingo Molnar wrote:
> 
> * Namhyung Kim <namhyung@gmail.com> wrote:
> 
>> remove unnecessary use of RELOC_HIDE(). It only does simple addition of ptr
>> and offset, and in this case, offset 0, does nothing. It does NOT do anything
>> with linker relocation things. I could find no reason to use it.
>>
>> The only user of __phys_reloc_hide() was __pa_symbol() so it can be removed
>> safely here.
>>
>> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
>> ---
>>  arch/x86/include/asm/page.h          |    5 ++---
>>  arch/x86/include/asm/page_32.h       |    1 -
>>  arch/x86/include/asm/page_64_types.h |    1 -
>>  3 files changed, 2 insertions(+), 5 deletions(-)
> 
> We do this as a general Voodoo barrier against GCC miscompilations.
> 
> You are right that it's largely moot by today (and especially so on x86 - i 
> only remember a single instance of miscompilation that Rusty mentioned few 
> years ago, and that was on powerpc), but the wrapper is simple enough, so 
> unless there's some real tangible improvement in the binary output we might as 
> well keep it.
> 
> Peter, what do you think?
> 

I agree... I suspect it might make some difference for gcc 3 stragglers.

	-hpa

Re: [PATCH] x86: remove __phys_reloc_hide

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> On 08/09/2010 12:22 AM, Andi Kleen wrote:
> > On Mon, Aug 09, 2010 at 04:04:45PM +0900, Namhyung Kim wrote:
> >>> It hides the value conversion from the compiler through asm()
> >>>
> >>> -Andi
> >>>
> >>
> >> Yes, indeed. But for what? __pa_symbol() is just used to get the address
> >> of some linker symbols in forms of unsigned long which has same bit
> >> representation as pointer in x86 (and all supported archs). So do we
> >> still need it or am I missing something?
> > 
> > The original reason was that the C standard allows the compiler
> > to make some assumptions on the pointer arithmetic that is done 
> > on symbol addresses (e.g. no wrapping). This is exploited
> > by the optimizer in the compiler to generate better code.
> > 
> > This lead to a miscompilation on PowerPC a couple of years back at 
> > least with the va->pa conversion.
> > 
> > After that RELOC_HIDE was introduced after funelling the
> > symbol address through an empty asm statement was recommended 
> > as the official way to do this by the gcc developers.
> > 
> > I think x86-64 does not normally wrap here, but it's 
> > still safer to do it this way.
> > 
> 
> We pass -fno-strict-overflow to the kernel now, which takes care of the
> underlying problem, at least for current versions of gcc.  Unfortunately
> we still have people who want to use very old gcc versions to compile
> the kernel, so it's probably better to leave it in at least until we
> formally kill off support for gcc 3.

Namhyung, mind sending a patch that adds a comment to __pa_symbol() to point 
out the connection to -fno-strict-overflow and that it can be removed once all 
supported versions of GCC understand -fno-strict-overflow?

That would make for one less piece of legacy voodoo code in the kernel ;-)

Thanks,

	Ingo

Re: [PATCH] x86: remove __phys_reloc_hide

[Namhyung Kim]

2010-08-10 (화), 09:05 +0200, Ingo Molnar:
> * H. Peter Anvin <hpa@zytor.com> wrote:
> 
> > We pass -fno-strict-overflow to the kernel now, which takes care of the
> > underlying problem, at least for current versions of gcc.  Unfortunately
> > we still have people who want to use very old gcc versions to compile
> > the kernel, so it's probably better to leave it in at least until we
> > formally kill off support for gcc 3.
> 
> Namhyung, mind sending a patch that adds a comment to __pa_symbol() to point 
> out the connection to -fno-strict-overflow and that it can be removed once all 
> supported versions of GCC understand -fno-strict-overflow?
> 
> That would make for one less piece of legacy voodoo code in the kernel ;-)
> 
> Thanks,
> 
> 	Ingo

No problem. :-) But before that, let me clarify this: It seems
-fno-strict-overflow is all about the signed arithmetic and
__pa_symbol() does unsigned one. Is it really matters here?


-- 
Regards,
Namhyung Kim

Re: [PATCH] x86: remove __phys_reloc_hide

[Namhyung Kim]

2010-08-10 (화), 19:46 +0900, Namhyung Kim:

> No problem. :-) But before that, let me clarify this: It seems
> -fno-strict-overflow is all about the signed arithmetic and
> __pa_symbol() does unsigned one. Is it really matters here?
> 
> 

Oops, my bad, I found the description of -fstrict-overflow in gcc manual
that it also affects the semantics of pointer and unsigned integer
arithmetic. Sorry for the noise.

-- 
Regards,
Namhyung Kim

[PATCH] x86: add a comment to __pa_symbol

[Namhyung Kim]

Until all supported versions of gcc recognize -fno-strict-overflow, we should
keep the RELOC_HIDE magic on __pa_symbol(). Comment it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Suggested-by: Ingo Molnar <mingo@elte.hu>

---

 I hope I'm doing right. :-)

 arch/x86/include/asm/page.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
index 625c3f0..06cb6f3 100644
--- a/arch/x86/include/asm/page.h
+++ b/arch/x86/include/asm/page.h
@@ -37,6 +37,13 @@ static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
 #define __pa_nodebug(x)	__phys_addr_nodebug((unsigned long)(x))
 /* __pa_symbol should be used for C visible symbols.
    This seems to be the official gcc blessed way to do such arithmetic. */
+/*
+ * we need __phys_reloc_hide() here because gcc may assume that there is no
+ * overflow during __pa() calculation and can optimize it unexpectedly.
+ * Newer versions of gcc provide -fno-strict-overflow switch to handle this
+ * case properly. Once all supported versions of gcc understand it, we can
+ * remove this Voodoo magic stuff.
+ */
 #define __pa_symbol(x)	__pa(__phys_reloc_hide((unsigned long)(x)))
 
 #define __va(x)			((void *)((unsigned long)(x)+PAGE_OFFSET))
-- 
1.7.0.4

[tip:x86/urgent] x86: Document __phys_reloc_hide() usage in __pa_symbol()

[tip-bot for Namhyung Kim]

Commit-ID:  8fd49936a8cac246fc9ed85508556c82cd44cf68
Gitweb:     http://git.kernel.org/tip/8fd49936a8cac246fc9ed85508556c82cd44cf68
Author:     Namhyung Kim <namhyung@gmail.com>
AuthorDate: Wed, 11 Aug 2010 15:37:41 +0900
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 11 Aug 2010 08:43:49 +0200

x86: Document __phys_reloc_hide() usage in __pa_symbol()

Until all supported versions of gcc recognize
-fno-strict-overflow, we should keep the RELOC_HIDE() magic in
__pa_symbol(). Comment it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
LKML-Reference: <1281508661-29507-1-git-send-email-namhyung@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/page.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
index 625c3f0..8ca8283 100644
--- a/arch/x86/include/asm/page.h
+++ b/arch/x86/include/asm/page.h
@@ -37,6 +37,13 @@ static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
 #define __pa_nodebug(x)	__phys_addr_nodebug((unsigned long)(x))
 /* __pa_symbol should be used for C visible symbols.
    This seems to be the official gcc blessed way to do such arithmetic. */
+/*
+ * We need __phys_reloc_hide() here because gcc may assume that there is no
+ * overflow during __pa() calculation and can optimize it unexpectedly.
+ * Newer versions of gcc provide -fno-strict-overflow switch to handle this
+ * case properly. Once all supported versions of gcc understand it, we can
+ * remove this Voodoo magic stuff. (i.e. once gcc3.x is deprecated)
+ */
 #define __pa_symbol(x)	__pa(__phys_reloc_hide((unsigned long)(x)))
 
 #define __va(x)			((void *)((unsigned long)(x)+PAGE_OFFSET))

Re: [PATCH] x86: remove __phys_reloc_hide

[H. Peter Anvin]

On 08/10/2010 10:44 PM, Namhyung Kim wrote:
> 2010-08-10 (화), 19:46 +0900, Namhyung Kim:
> 
>> No problem. :-) But before that, let me clarify this: It seems
>> -fno-strict-overflow is all about the signed arithmetic and
>> __pa_symbol() does unsigned one. Is it really matters here?
>>
> 
> Oops, my bad, I found the description of -fstrict-overflow in gcc manual
> that it also affects the semantics of pointer and unsigned integer
> arithmetic. Sorry for the noise.
> 

Not unsigned integers, those are defined by the C standard to wrap.

	-hpa

Re: [PATCH] x86: remove __phys_reloc_hide

[Thomas Gleixner]

On Mon, 9 Aug 2010, Andi Kleen wrote:

   ^^^^^^^^^^^^^^^

Care to fix your system clock?