* Small PIC Fixes
@ 2011-01-05 5:58 ehem+grub
2011-01-05 8:06 ` Vladimir 'φ-coder/phcoder' Serbinenko
0 siblings, 1 reply; 4+ messages in thread
From: ehem+grub @ 2011-01-05 5:58 UTC (permalink / raw)
To: grub-devel
[-- Attachment #1: Type: text/plain, Size: 860 bytes --]
Perhaps not the biggest deal, but I do like to get low-hanging fixes out
of the way if they appear.
One very significant item I found. It appears GCC is fine with %rbx being
clobbered when building PIC in 64-bit mode, even though it has problems
with %ebx being clobbered when building PIC in 32-bit mode.
One other item I did notice. Are there really any processors in the amd64
class that *don't* support CPUID? I'd like to hardcode
grub_cpu_is_cpuid_supported() to return 1 if __x86_64__ is defined, but
I'm a tad worried I'll be unpleasantly surprised.
--
(\___(\___(\______ --=> 8-) EHM <=-- ______/)___/)___/)
\BS ( | EHeM@gremlin.m5p.com PGP F6B23DE0 | ) /
\_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0
[-- Attachment #2: Type: text/x-patch, Size: 6316 bytes --]
=== modified file 'ChangeLog'
--- ChangeLog 2011-01-05 00:28:28 +0000
+++ ChangeLog 2011-01-05 05:42:55 +0000
@@ -1,3 +1,13 @@
+2011-01-04 Elliott Mitchell <ehem+grub@m5p.com>
+
+ * include/grub/i386/tsc.h: Create shared macros for handling all the
+ tiny little blobs of assembly where we'd like to be able to clobber
+ %ebx. Replace the many uses of #ifdef(APPLE_CC)/#ifdef(__x64_64__)
+ with usage of these macros. Testing found %rbx *can* be clobbered in
+ amd64/64-bit mode.
+ * grub-core/loader/i386/xnu.c: Propogate usage of the macros created
+ above to this file. Removes sections of copy&paste code.
+
2011-01-05 Vladimir Serbinenko <phcoder@gmail.com>
* util/grub-install.in: Determine ofpathname, nvsetenv and efibootmgr
=== modified file 'grub-core/loader/i386/xnu.c'
--- grub-core/loader/i386/xnu.c 2010-09-05 11:05:36 +0000
+++ grub-core/loader/i386/xnu.c 2011-01-05 01:47:27 +0000
@@ -1,6 +1,6 @@
/*
* GRUB -- GRand Unified Bootloader
- * Copyright (C) 2009 Free Software Foundation, Inc.
+ * Copyright (C) 2009,2011 Free Software Foundation, Inc.
*
* GRUB is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -130,62 +130,35 @@
if (! grub_cpu_is_cpuid_supported ())
return sane_value;
-#ifdef APPLE_CC
asm volatile ("movl $0, %%eax\n"
-#ifdef __x86_64__
- "push %%rbx\n"
-#else
- "push %%ebx\n"
-#endif
+ SAVE_REG_B
"cpuid\n"
-#ifdef __x86_64__
- "pop %%rbx\n"
-#else
- "pop %%ebx\n"
+#ifdef REGISTER_B_PRECIOUS
+ "movl %ebx, %edi"
#endif
+ RESTORE_REG_B
: "=a" (max_cpuid),
- "=d" (manufacturer[1]), "=c" (manufacturer[2]));
-
- /* Only Intel for now is done. */
- if (grub_memcmp (manufacturer + 1, "ineIntel", 12) != 0)
- return sane_value;
-
+#ifdef REGISTER_B_PRECIOUS
+ "=di" (manufacturer[0]),
#else
- asm volatile ("movl $0, %%eax\n"
- "cpuid"
- : "=a" (max_cpuid), "=b" (manufacturer[0]),
- "=d" (manufacturer[1]), "=c" (manufacturer[2]));
+ "=b" (manufacturer[0]),
+#endif
+ "=d" (manufacturer[1]), "=c" (manufacturer[2]));
/* Only Intel for now is done. */
if (grub_memcmp (manufacturer, "GenuineIntel", 12) != 0)
return sane_value;
-#endif
/* Check Speedstep. */
if (max_cpuid < 1)
return sane_value;
-#ifdef APPLE_CC
- asm volatile ("movl $1, %%eax\n"
-#ifdef __x86_64__
- "push %%rbx\n"
-#else
- "push %%ebx\n"
-#endif
- "cpuid\n"
-#ifdef __x86_64__
- "pop %%rbx\n"
-#else
- "pop %%ebx\n"
-#endif
- : "=c" (capabilities):
- : "%rax", "%rdx");
-#else
- asm volatile ("movl $1, %%eax\n"
+ asm volatile ("movl $1, %%eax\n"
+ SAVE_REG_B
"cpuid"
+ RESTORE_REG_B
: "=c" (capabilities):
- : "%rax", "%rbx", "%rdx");
-#endif
+ : "%rax" CLOBBER_REG_B, "%rdx");
if (! (capabilities & (1 << 7)))
return sane_value;
=== modified file 'include/grub/i386/tsc.h'
--- include/grub/i386/tsc.h 2010-01-03 22:05:07 +0000
+++ include/grub/i386/tsc.h 2011-01-05 05:23:07 +0000
@@ -1,6 +1,6 @@
/*
* GRUB -- GRand Unified Bootloader
- * Copyright (C) 2008,2009 Free Software Foundation, Inc.
+ * Copyright (C) 2008,2009,2011 Free Software Foundation, Inc.
*
* GRUB is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -21,6 +21,21 @@
#include <grub/types.h>
+/* for use with assembly segments utilizing the B register, alas in PIC
+* non-64-bit mode, GCC can't deal with it being clobbered. */
+#if (defined(APPLE_CC) || defined(__pic__)) && !defined(__x86_64__)
+#define REGISTER_C_PRECIOUS 1
+#define SAVE_REG_B "push %ebx\n"
+#define RESTORE_REG_B "pop %ebx\n"
+#define CLOBBER_REG_B
+
+#else
+#define SAVE_REG_B
+#define RESTORE_REG_B
+#define CLOBBER_REG_B "%rbx".
+#endif
+
+
/* Read the TSC value, which increments with each CPU clock cycle. */
static __inline grub_uint64_t
grub_get_tsc (void)
@@ -29,24 +44,11 @@
/* The CPUID instruction is a 'serializing' instruction, and
avoids out-of-order execution of the RDTSC instruction. */
-#ifdef APPLE_CC
__asm__ __volatile__ ("xorl %%eax, %%eax\n\t"
-#ifdef __x86_64__
- "push %%rbx\n"
-#else
- "push %%ebx\n"
-#endif
+ SAVE_REG_B
"cpuid\n"
-#ifdef __x86_64__
- "pop %%rbx\n"
-#else
- "pop %%ebx\n"
-#endif
- :::"%rax", "%rcx", "%rdx");
-#else
- __asm__ __volatile__ ("xorl %%eax, %%eax\n\t"
- "cpuid":::"%rax", "%rbx", "%rcx", "%rdx");
-#endif
+ RESTORE_REG_B
+ :::"%rax", CLOBBER_REG_B "%rcx", "%rdx");
/* Read TSC value. We cannot use "=A", since this would use
%rax on x86_64. */
__asm__ __volatile__ ("rdtsc":"=a" (lo), "=d" (hi));
@@ -54,12 +56,17 @@
return (((grub_uint64_t) hi) << 32) | lo;
}
-#ifdef __x86_64__
static __inline int
grub_cpu_is_cpuid_supported (void)
{
- grub_uint64_t id_supported;
+#ifdef __x86_64__
+ /* there really any processors in this class that *don't* support cpuid?! */
+#if 0
+ return 1;
+#endif
+
+ grub_size_t id_supported;
__asm__ ("pushfq\n\t"
"popq %%rax /* Get EFLAGS into EAX */\n\t"
@@ -75,14 +82,8 @@
: /* Clobbered: */ "%rcx");
return id_supported != 0;
-}
-
#else
-
-static __inline int
-grub_cpu_is_cpuid_supported (void)
-{
- grub_uint32_t id_supported;
+ grub_size_t id_supported;
__asm__ ("pushfl\n\t"
"popl %%eax /* Get EFLAGS into EAX */\n\t"
@@ -98,6 +99,7 @@
: /* Clobbered: */ "%rcx");
return id_supported != 0;
+#endif
}
#endif
@@ -109,29 +111,13 @@
return 0;
grub_uint32_t features;
-#ifdef APPLE_CC
- __asm__ ("movl $1, %%eax\n\t"
-#ifdef __x86_64__
- "push %%rbx\n"
-#else
- "push %%ebx\n"
-#endif
- "cpuid\n"
-#ifdef __x86_64__
- "pop %%rbx\n"
-#else
- "pop %%ebx\n"
-#endif
- : "=d" (features)
- : /* No inputs. */
- : /* Clobbered: */ "%rax", "%rcx");
-#else
- __asm__ ("movl $1, %%eax\n\t"
- "cpuid\n"
- : "=d" (features)
- : /* No inputs. */
- : /* Clobbered: */ "%rax", "%rbx", "%rcx");
-#endif
+ __asm__ ("movl $1, %%eax\n\t"
+ SAVE_REG_B
+ "cpuid\n"
+ RESTORE_REG_B
+ : "=d" (features)
+ : /* No inputs. */
+ : /* Clobbered: */ "%rax", CLOBBER_REG_B "%rcx");
return (features & (1 << 4)) != 0;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Small PIC Fixes
2011-01-05 5:58 Small PIC Fixes ehem+grub
@ 2011-01-05 8:06 ` Vladimir 'φ-coder/phcoder' Serbinenko
2011-01-06 3:07 ` ehem+grub
0 siblings, 1 reply; 4+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2011-01-05 8:06 UTC (permalink / raw)
To: grub-devel
[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]
On 01/05/2011 06:58 AM, ehem+grub@m5p.com wrote:
> Perhaps not the biggest deal, but I do like to get low-hanging fixes out
> of the way if they appear.
>
> One very significant item I found. It appears GCC is fine with %rbx being
> clobbered when building PIC in 64-bit mode, even though it has problems
> with %ebx being clobbered when building PIC in 32-bit mode.
>
>
This patch only increases the number of possible ways preprocessor
defines can be resolved, which in turn increases the maintenance cost.
Restoring %ebx/%rbx unconditionally is cheap. Maintaining exponentially
growing number of the way #if's can be resolved isn't.
I don't see any problem to enduser with these 2 small instructions
always being there.
> One other item I did notice. Are there really any processors in the amd64
> class that *don't* support CPUID? I'd like to hardcode
> grub_cpu_is_cpuid_supported() to return 1 if __x86_64__ is defined, but
> I'm a tad worried I'll be unpleasantly surprised.
>
>
Similar problems. Maintaining something that is always the same is
easier than something with loads of #if's.
Rule of thumb is: "if it works and your improvement isn't visible to any
enduser, don't touch it".
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
--
Regards
Vladimir 'φ-coder/phcoder' Serbinenko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Small PIC Fixes
2011-01-05 8:06 ` Vladimir 'φ-coder/phcoder' Serbinenko
@ 2011-01-06 3:07 ` ehem+grub
2011-01-06 3:16 ` crocket
0 siblings, 1 reply; 4+ messages in thread
From: ehem+grub @ 2011-01-06 3:07 UTC (permalink / raw)
To: The development of GNU GRUB
[-- Attachment #1: Type: text/plain, Size: 3034 bytes --]
>From: Vladimir '?-coder/phcoder' Serbinenko <phcoder@gmail.com>
> On 01/05/2011 06:58 AM, ehem+grub@m5p.com wrote:
> > Perhaps not the biggest deal, but I do like to get low-hanging fixes out
> > of the way if they appear.
> >
> > One very significant item I found. It appears GCC is fine with %rbx being
> > clobbered when building PIC in 64-bit mode, even though it has problems
> > with %ebx being clobbered when building PIC in 32-bit mode.
> This patch only increases the number of possible ways preprocessor
> defines can be resolved, which in turn increases the maintenance cost.
> Restoring %ebx/%rbx unconditionally is cheap. Maintaining exponentially
> growing number of the way #if's can be resolved isn't.
> I don't see any problem to enduser with these 2 small instructions
> always being there.
Please point to where the number of ways the preprocessor defines can be
resolved has increased. I did switch the direction of two cases (APPLE_CC
and __x86_64__ defined; plus __pic__ defined, but APPLE_CC undefined),
but those two cases already existed.
> > One other item I did notice. Are there really any processors in the amd64
> > class that *don't* support CPUID? I'd like to hardcode
> > grub_cpu_is_cpuid_supported() to return 1 if __x86_64__ is defined, but
> > I'm a tad worried I'll be unpleasantly surprised.
> Similar problems. Maintaining something that is always the same is
> easier than something with loads of #if's.
Great, I concur. Notice how the patch rips out *12* #if/#else/#endif
blocks, and replaces all of them with *1* #if/#else/#endif block that
completely lacks any nesting!
Making more systems run over a piece of code will merely provide more
testing for the common case; it won't increase testing of the uncommon
case, alas that is by *far* the more valuable testing since that is where
bugs tend to show up. In the case of grub_cpu_is_cpuid_supported(),
unless you specifically know of an amd64-class processor that lacks the
cpuid instruction, attempting to maintain the extra code means more time
spent there for no gain. Worse, copy&paste code is pretty well a magnet
for bugs from typos or missing a varient somewhere, and it makes any
attempt at coverage testing impossible (since no machine exists to test
it).
> Rule of thumb is: "if it works and your improvement isn't visible to any
> enduser, don't touch it".
Well, "crocket" managed to uncover the situation. The situation of
attempting to build -fpic might rarely come up, but s/he managed to find
it. I doubt the patch covers everything needed to make -fpic work, but it
takes a few steps in that direction.
If you want to criticize, criticize for lack of checking before sending
the patch (revised patch attached).
--
(\___(\___(\______ --=> 8-) EHM <=-- ______/)___/)___/)
\BS ( | EHeM@gremlin.m5p.com PGP F6B23DE0 | ) /
\_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0
[-- Attachment #2: Type: text/x-patch, Size: 6450 bytes --]
=== modified file 'ChangeLog'
--- ChangeLog 2011-01-05 11:23:06 +0000
+++ ChangeLog 2011-01-06 01:29:08 +0000
@@ -1,3 +1,13 @@
+2011-01-05 Elliott Mitchell <ehem+grub@m5p.com>
+
+ * include/grub/i386/tsc.h: Create shared macros for handling all the
+ tiny little blobs of assembly where we'd like to be able to clobber
+ %ebx. Replace the many uses of #ifdef(APPLE_CC)/#ifdef(__x64_64__)
+ with usage of these macros. Testing found %rbx *can* be clobbered in
+ amd64/64-bit mode.
+ * grub-core/loader/i386/xnu.c: Propogate usage of the macros created
+ above to this file. Removes sections of copy&paste code.
+
2011-01-05 Vladimir Serbinenko <phcoder@gmail.com>
Run terminfo_cls on initing terminfo output to clear the screen and
=== modified file 'grub-core/loader/i386/xnu.c'
--- grub-core/loader/i386/xnu.c 2010-09-05 11:05:36 +0000
+++ grub-core/loader/i386/xnu.c 2011-01-06 01:22:33 +0000
@@ -1,6 +1,6 @@
/*
* GRUB -- GRand Unified Bootloader
- * Copyright (C) 2009 Free Software Foundation, Inc.
+ * Copyright (C) 2009,2011 Free Software Foundation, Inc.
*
* GRUB is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -130,62 +130,27 @@
if (! grub_cpu_is_cpuid_supported ())
return sane_value;
-#ifdef APPLE_CC
asm volatile ("movl $0, %%eax\n"
-#ifdef __x86_64__
- "push %%rbx\n"
-#else
- "push %%ebx\n"
-#endif
+ ASM_SAVE_REG_B
"cpuid\n"
-#ifdef __x86_64__
- "pop %%rbx\n"
-#else
- "pop %%ebx\n"
-#endif
- : "=a" (max_cpuid),
- "=d" (manufacturer[1]), "=c" (manufacturer[2]));
-
- /* Only Intel for now is done. */
- if (grub_memcmp (manufacturer + 1, "ineIntel", 12) != 0)
- return sane_value;
-
-#else
- asm volatile ("movl $0, %%eax\n"
- "cpuid"
- : "=a" (max_cpuid), "=b" (manufacturer[0]),
+ ASM_REST_REG_B_XCHG
+ : "=a" (max_cpuid), ASM_REG_B_OUT (manufacturer[0]),
"=d" (manufacturer[1]), "=c" (manufacturer[2]));
/* Only Intel for now is done. */
if (grub_memcmp (manufacturer, "GenuineIntel", 12) != 0)
return sane_value;
-#endif
/* Check Speedstep. */
if (max_cpuid < 1)
return sane_value;
-#ifdef APPLE_CC
asm volatile ("movl $1, %%eax\n"
-#ifdef __x86_64__
- "push %%rbx\n"
-#else
- "push %%ebx\n"
-#endif
+ ASM_SAVE_REG_B
"cpuid\n"
-#ifdef __x86_64__
- "pop %%rbx\n"
-#else
- "pop %%ebx\n"
-#endif
- : "=c" (capabilities):
- : "%rax", "%rdx");
-#else
- asm volatile ("movl $1, %%eax\n"
- "cpuid"
- : "=c" (capabilities):
- : "%rax", "%rbx", "%rdx");
-#endif
+ ASM_REST_REG_B
+ : "=c" (capabilities):
+ : "%rax", ASM_CLOB_REG_B, "%rdx");
if (! (capabilities & (1 << 7)))
return sane_value;
=== modified file 'include/grub/i386/tsc.h'
--- include/grub/i386/tsc.h 2010-01-03 22:05:07 +0000
+++ include/grub/i386/tsc.h 2011-01-06 01:27:48 +0000
@@ -1,6 +1,6 @@
/*
* GRUB -- GRand Unified Bootloader
- * Copyright (C) 2008,2009 Free Software Foundation, Inc.
+ * Copyright (C) 2008,2009,2011 Free Software Foundation, Inc.
*
* GRUB is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -21,6 +21,24 @@
#include <grub/types.h>
+/* for use with assembly segments utilizing the B register, alas in PIC
+* non-64-bit mode, GCC can't deal with it being clobbered. */
+#if (defined(APPLE_CC) || defined(__pic__)) && !defined(__x86_64__)
+#define ASM_SAVE_REG_B "movel %%ebx, %%edi\n"
+#define ASM_REST_REG_B "movel %%edi, %%edx\n"
+#define ASM_REST_REG_B_XCHG "xchg %%edi, %%edi\n"
+#define ASM_CLOB_REG_B "%edi"
+#define ASM_REG_B_OUT "=di"
+
+#else
+#define ASM_SAVE_REG_B
+#define ASM_REST_REG_B
+#define ASM_REST_REG_B_XCHG
+#define ASM_CLOB_REG_B "%rbx"
+#define ASM_REG_B_OUT "=b"
+#endif
+
+
/* Read the TSC value, which increments with each CPU clock cycle. */
static __inline grub_uint64_t
grub_get_tsc (void)
@@ -29,24 +47,11 @@
/* The CPUID instruction is a 'serializing' instruction, and
avoids out-of-order execution of the RDTSC instruction. */
-#ifdef APPLE_CC
- __asm__ __volatile__ ("xorl %%eax, %%eax\n\t"
-#ifdef __x86_64__
- "push %%rbx\n"
-#else
- "push %%ebx\n"
-#endif
+ __asm__ __volatile__ ("movl $0, %%eax\n\t"
+ ASM_SAVE_REG_B
"cpuid\n"
-#ifdef __x86_64__
- "pop %%rbx\n"
-#else
- "pop %%ebx\n"
-#endif
- :::"%rax", "%rcx", "%rdx");
-#else
- __asm__ __volatile__ ("xorl %%eax, %%eax\n\t"
- "cpuid":::"%rax", "%rbx", "%rcx", "%rdx");
-#endif
+ ASM_REST_REG_B
+ :::"%rax", ASM_CLOB_REG_B, "%rcx", "%rdx");
/* Read TSC value. We cannot use "=A", since this would use
%rax on x86_64. */
__asm__ __volatile__ ("rdtsc":"=a" (lo), "=d" (hi));
@@ -54,12 +59,17 @@
return (((grub_uint64_t) hi) << 32) | lo;
}
-#ifdef __x86_64__
static __inline int
grub_cpu_is_cpuid_supported (void)
{
- grub_uint64_t id_supported;
+#ifdef __x86_64__
+ /* there really any processors in this class that *don't* support cpuid?! */
+#if 0
+ return 1;
+#endif
+
+ grub_size_t id_supported;
__asm__ ("pushfq\n\t"
"popq %%rax /* Get EFLAGS into EAX */\n\t"
@@ -75,14 +85,8 @@
: /* Clobbered: */ "%rcx");
return id_supported != 0;
-}
-
#else
-
-static __inline int
-grub_cpu_is_cpuid_supported (void)
-{
- grub_uint32_t id_supported;
+ grub_size_t id_supported;
__asm__ ("pushfl\n\t"
"popl %%eax /* Get EFLAGS into EAX */\n\t"
@@ -98,10 +102,9 @@
: /* Clobbered: */ "%rcx");
return id_supported != 0;
+#endif
}
-#endif
-
static __inline int
grub_cpu_is_tsc_supported (void)
{
@@ -109,29 +112,13 @@
return 0;
grub_uint32_t features;
-#ifdef APPLE_CC
- __asm__ ("movl $1, %%eax\n\t"
-#ifdef __x86_64__
- "push %%rbx\n"
-#else
- "push %%ebx\n"
-#endif
- "cpuid\n"
-#ifdef __x86_64__
- "pop %%rbx\n"
-#else
- "pop %%ebx\n"
-#endif
- : "=d" (features)
- : /* No inputs. */
- : /* Clobbered: */ "%rax", "%rcx");
-#else
- __asm__ ("movl $1, %%eax\n\t"
- "cpuid\n"
- : "=d" (features)
- : /* No inputs. */
- : /* Clobbered: */ "%rax", "%rbx", "%rcx");
-#endif
+ __asm__ ("movl $1, %%eax\n\t"
+ ASM_SAVE_REG_B
+ "cpuid\n"
+ ASM_REST_REG_B
+ : "=d" (features)
+ : /* No inputs. */
+ : /* Clobbered: */ "%rax", ASM_CLOB_REG_B, "%rcx");
return (features & (1 << 4)) != 0;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Small PIC Fixes
2011-01-06 3:07 ` ehem+grub
@ 2011-01-06 3:16 ` crocket
0 siblings, 0 replies; 4+ messages in thread
From: crocket @ 2011-01-06 3:16 UTC (permalink / raw)
To: The development of GNU GRUB
I am "he" for your information.
Don't expect anything from "she", sir.
----- Original Message ----
From: "ehem+grub@m5p.com" <ehem+grub@m5p.com>
To: The development of GNU GRUB <grub-devel@gnu.org>
Sent: Thu, January 6, 2011 12:07:44 PM
Subject: Re: Small PIC Fixes
>From: Vladimir '?-coder/phcoder' Serbinenko <phcoder@gmail.com>
> On 01/05/2011 06:58 AM, ehem+grub@m5p.com wrote:
> > Perhaps not the biggest deal, but I do like to get low-hanging fixes out
> > of the way if they appear.
> >
> > One very significant item I found. It appears GCC is fine with %rbx being
> > clobbered when building PIC in 64-bit mode, even though it has problems
> > with %ebx being clobbered when building PIC in 32-bit mode.
> This patch only increases the number of possible ways preprocessor
> defines can be resolved, which in turn increases the maintenance cost.
> Restoring %ebx/%rbx unconditionally is cheap. Maintaining exponentially
> growing number of the way #if's can be resolved isn't.
> I don't see any problem to enduser with these 2 small instructions
> always being there.
Please point to where the number of ways the preprocessor defines can be
resolved has increased. I did switch the direction of two cases (APPLE_CC
and __x86_64__ defined; plus __pic__ defined, but APPLE_CC undefined),
but those two cases already existed.
> > One other item I did notice. Are there really any processors in the amd64
> > class that *don't* support CPUID? I'd like to hardcode
> > grub_cpu_is_cpuid_supported() to return 1 if __x86_64__ is defined, but
> > I'm a tad worried I'll be unpleasantly surprised.
> Similar problems. Maintaining something that is always the same is
> easier than something with loads of #if's.
Great, I concur. Notice how the patch rips out *12* #if/#else/#endif
blocks, and replaces all of them with *1* #if/#else/#endif block that
completely lacks any nesting!
Making more systems run over a piece of code will merely provide more
testing for the common case; it won't increase testing of the uncommon
case, alas that is by *far* the more valuable testing since that is where
bugs tend to show up. In the case of grub_cpu_is_cpuid_supported(),
unless you specifically know of an amd64-class processor that lacks the
cpuid instruction, attempting to maintain the extra code means more time
spent there for no gain. Worse, copy&paste code is pretty well a magnet
for bugs from typos or missing a varient somewhere, and it makes any
attempt at coverage testing impossible (since no machine exists to test
it).
> Rule of thumb is: "if it works and your improvement isn't visible to any
> enduser, don't touch it".
Well, "crocket" managed to uncover the situation. The situation of
attempting to build -fpic might rarely come up, but s/he managed to find
it. I doubt the patch covers everything needed to make -fpic work, but it
takes a few steps in that direction.
If you want to criticize, criticize for lack of checking before sending
the patch (revised patch attached).
--
(\___(\___(\______ --=> 8-) EHM <=-- ______/)___/)___/)
\BS ( | EHeM@gremlin.m5p.com PGP F6B23DE0 | ) /
\_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-01-06 3:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-01-05 5:58 Small PIC Fixes ehem+grub
2011-01-05 8:06 ` Vladimir 'φ-coder/phcoder' Serbinenko
2011-01-06 3:07 ` ehem+grub
2011-01-06 3:16 ` crocket
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.