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 ...
[ message continues ]

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.
--

[ message continues ]

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


--

[ message continues ]

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

-Andi

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

[ message continues ]

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


--

[ message continues ]

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.
--

[ message continues ]

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


--

[ message continues ]

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
--

[ message continues ]

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
--

[ message continues ]

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


--

[ message continues ]

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


--

[ message continues ]

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

	-hpa
--

[ message continues ]

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

--

[ message continues ]

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))
--

[ message continues ]

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
--

[ message continues ]

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

	-hpa

--

[ message continues ]