I am "he" for your information.
Don't expect anything from "she", sir.
----- Original Message ----
From: "ehem+g...@m5p.com" <ehem+g...@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 <phco...@gmail.com>
> On 01/05/2011 06:58 AM, ehem+g...@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 ( | e...@gremlin.m5p.com PGP F6B23DE0 | ) /
\_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
