Move #ifdef around function definiton into the function and
unconditionally return on X86_32. Saves an ifdef from the
one callsite.
Signed-off-by: Harvey Harrison <[email protected]>
---
Ingo, Masami, final leftovers from some unsent kprobes unification work.
Net reduction of one #ifdef section.
arch/x86/kernel/kprobes.c | 11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
index b1804e4..1ac532e 100644
--- a/arch/x86/kernel/kprobes.c
+++ b/arch/x86/kernel/kprobes.c
@@ -263,15 +263,16 @@ static int __kprobes is_IF_modifier(kprobe_opcode_t *insn)
return 0;
}
-#ifdef CONFIG_X86_64
/*
* Adjust the displacement if the instruction uses the %rip-relative
* addressing mode.
* If it does, Return the address of the 32-bit displacement word.
* If not, return null.
+ * Only applicable to X86_64
*/
static void __kprobes fix_riprel(struct kprobe *p)
{
+#ifdef CONFIG_X86_64
u8 *insn = p->ainsn.insn;
s64 disp;
int need_modrm;
@@ -335,15 +336,17 @@ static void __kprobes fix_riprel(struct kprobe *p)
*(s32 *)insn = (s32) disp;
}
}
-}
+#else
+ return;
#endif
+}
static void __kprobes arch_copy_kprobe(struct kprobe *p)
{
memcpy(p->ainsn.insn, p->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
-#ifdef CONFIG_X86_64
+
fix_riprel(p);
-#endif
+
if (can_boost(p->addr))
p->ainsn.boostable = 0;
else
--
1.5.4.rc0.1143.g1a8a
Hello Harvey,
A similar idea was already nack-ed by Ananth.
http://sources.redhat.com/ml/systemtap/2007-q4/msg00468.html
And I agree his thought.
Especially, "riprel" does not exist on x86_32, so fix_riprel()
is meaningless on it.
Thus, I think it would better be ifdef'd in call-site.
Harvey Harrison wrote:
> Move #ifdef around function definiton into the function and
> unconditionally return on X86_32. Saves an ifdef from the
> one callsite.
>
> Signed-off-by: Harvey Harrison <[email protected]>
> ---
> Ingo, Masami, final leftovers from some unsent kprobes unification work.
>
> Net reduction of one #ifdef section.
>
> arch/x86/kernel/kprobes.c | 11 +++++++----
> 1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
> index b1804e4..1ac532e 100644
> --- a/arch/x86/kernel/kprobes.c
> +++ b/arch/x86/kernel/kprobes.c
> @@ -263,15 +263,16 @@ static int __kprobes is_IF_modifier(kprobe_opcode_t *insn)
> return 0;
> }
>
> -#ifdef CONFIG_X86_64
> /*
> * Adjust the displacement if the instruction uses the %rip-relative
> * addressing mode.
> * If it does, Return the address of the 32-bit displacement word.
> * If not, return null.
> + * Only applicable to X86_64
> */
> static void __kprobes fix_riprel(struct kprobe *p)
> {
> +#ifdef CONFIG_X86_64
> u8 *insn = p->ainsn.insn;
> s64 disp;
> int need_modrm;
> @@ -335,15 +336,17 @@ static void __kprobes fix_riprel(struct kprobe *p)
> *(s32 *)insn = (s32) disp;
> }
> }
> -}
> +#else
> + return;
> #endif
> +}
>
> static void __kprobes arch_copy_kprobe(struct kprobe *p)
> {
> memcpy(p->ainsn.insn, p->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
> -#ifdef CONFIG_X86_64
> +
> fix_riprel(p);
> -#endif
> +
> if (can_boost(p->addr))
> p->ainsn.boostable = 0;
> else
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: [email protected], [email protected]
* Masami Hiramatsu <[email protected]> wrote:
> Hello Harvey,
>
> A similar idea was already nack-ed by Ananth.
> http://sources.redhat.com/ml/systemtap/2007-q4/msg00468.html
> And I agree his thought.
>
> Especially, "riprel" does not exist on x86_32, so fix_riprel()
> is meaningless on it.
> Thus, I think it would better be ifdef'd in call-site.
but we regularly do this in generic code: we add calls that are NOPs on
some architectures. For example flush_cache_page() makes no sense on the
x86 architecture. So i'm inclined to apply Harvey's cleanup - less
#ifdef complexity in higher-level code is very much favored, even
if "riprel" is a NOP concept on 32-bit.
Ingo
Hello Ingo,
Ingo Molnar wrote:
> * Masami Hiramatsu <[email protected]> wrote:
>
>> Hello Harvey,
>>
>> A similar idea was already nack-ed by Ananth.
>> http://sources.redhat.com/ml/systemtap/2007-q4/msg00468.html
>> And I agree his thought.
>>
>> Especially, "riprel" does not exist on x86_32, so fix_riprel()
>> is meaningless on it.
>> Thus, I think it would better be ifdef'd in call-site.
>
> but we regularly do this in generic code: we add calls that are NOPs on
> some architectures. For example flush_cache_page() makes no sense on the
> x86 architecture.
Indeed.
By the way, flush_cache_page() is defined as a do-while(0) on x86.
Would it better to define fix_riprel() as a do-while(0) on x86-32?
I think this obviously indicates that function has no effect.
> So i'm inclined to apply Harvey's cleanup - less
> #ifdef complexity in higher-level code is very much favored, even
> if "riprel" is a NOP concept on 32-bit.
OK, I agree about that fix_riprel() which is ifdef'd twice is too much ifdef'd.
Reducing ifdef is good to me.
Thanks,
>
> Ingo
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: [email protected], [email protected]
* Masami Hiramatsu <[email protected]> wrote:
> Indeed.
> By the way, flush_cache_page() is defined as a do-while(0) on x86.
> Would it better to define fix_riprel() as a do-while(0) on x86-32?
> I think this obviously indicates that function has no effect.
NOPs should always be an inline. flush_cache_page()'s macro use is
historic - feel free to send cleanup patches against cacheflush.h.
or even better, since most architectures dont need explicit
cache-flushes, provide an asm-generic/cache_flush-nop.h file that is
#include-ed by asm-x86/cacheflush.h. (and by other architectures)
Ingo
Ingo Molnar wrote:
> * Masami Hiramatsu <[email protected]> wrote:
>
>> Indeed.
>> By the way, flush_cache_page() is defined as a do-while(0) on x86.
>> Would it better to define fix_riprel() as a do-while(0) on x86-32?
>> I think this obviously indicates that function has no effect.
>
> NOPs should always be an inline. flush_cache_page()'s macro use is
> historic - feel free to send cleanup patches against cacheflush.h.
OK, in that case, harvey's patch is good to me.
> or even better, since most architectures dont need explicit
> cache-flushes, provide an asm-generic/cache_flush-nop.h file that is
> #include-ed by asm-x86/cacheflush.h. (and by other architectures)
>
> Ingo
Thanks,
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: [email protected], [email protected]