grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Small PIC Fixes


From: ehem+grub
Subject: Re: Small PIC Fixes
Date: Wed, 5 Jan 2011 19:07:44 -0800 (PST)

>From: Vladimir '?-coder/phcoder' Serbinenko <address@hidden>
> On 01/05/2011 06:58 AM, address@hidden 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 (    |         address@hidden PGP F6B23DE0         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0


Attachment: pic.diff
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]