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