svn commit: r216161 - in head/sys: amd64/amd64 i386/i386

[Bruce Evans]

On Fri, 3 Dec 2010, Jung-uk Kim wrote:

> On Friday 03 December 2010 05:43 pm, Jung-uk Kim wrote:
>> On Friday 03 December 2010 05:08 pm, John Baldwin wrote:
>>> On Friday, December 03, 2010 4:54:10 pm Jung-uk Kim wrote:
>>>> Author: jkim
>>>> Date: Fri Dec  3 21:54:10 2010
>>>> New Revision: 216161
>>>> URL: http://svn.freebsd.org/changeset/base/216161
>>>>
>>>> Log:
>>>>   Explicitly initialize TSC frequency.  To calibrate TSC
>>>> frequency, we use DELAY(9) and it may use TSC in turn if TSC
>>>> frequency is non-zero.
>>>
>>> We zero the BSS, so these were already zero.  This just makes the
>>> actual kernel file on disk larger by wasting space in .data
>>> instead of .bss.
>>
>> Please note that I didn't touch other variables, e.g.,
>> tsc_is_broken, because I knew that.  However, I just wanted to do
>> that *explicitly*. Anyway, it is reverted now and SVN will remember
>> what I wanted to do. ;-)
>>
>> BTW, if my memory serves, GCC (and all modern C compilers) put(s)
>> zero-initialized variables back in .bss.

Yes, this is just a style bug, since it doesn't even waste space :-).
It still gives control over the layout, at least with gcc-3.3.3 on i386,
but most places don't care about the layout.

Why are you doing style bugs explicitly? :-)

> I just tried it.  GCC generates identical binaries as I thought.
> However, Clang doesn't do the optimization. :-/

I get large differences even with gcc for assembler files (.zero and
.p2align directives for explicit initialization -- this gives control
over the layout) and small differences in .o files).

BTW, at least i386 has some nonsense initialization related to this:
from i386/initcpu.c:

% /* Must *NOT* be BSS or locore will bzero these after setting them */
% int	cpu = 0;		/* Are we 386, 386sx, 486, etc? */
% u_int	cpu_feature = 0;	/* Feature flags */
% u_int	cpu_feature2 = 0;	/* Feature flags */
% u_int	amd_feature = 0;	/* AMD feature flags */
% u_int	amd_feature2 = 0;	/* AMD feature flags */
% u_int	amd_pminfo = 0;		/* AMD advanced power management info */
% u_int	via_feature_rng = 0;	/* VIA RNG features */
% u_int	via_feature_xcrypt = 0;	/* VIA ACE features */
% u_int	cpu_high = 0;		/* Highest arg to CPUID */
% u_int	cpu_id = 0;		/* Stepping ID */
% u_int	cpu_procinfo = 0;	/* HyperThreading Info / Brand Index / CLFUSH */
% u_int	cpu_procinfo2 = 0;	/* Multicore info */
% char	cpu_vendor[20] = "";	/* CPU Origin code */
% u_int	cpu_vendor_id = 0;	/* CPU vendor ID */
% u_int	cpu_clflush_line_size = 32;

But all of these except the ones initialized to nonzero ARE in the
BSS.  The comment is wrong because locore does the bzeroing as early
as possible so as to avoid this problem.  It has done the bss bzeroing
early enough since at least RELENG_3, and probably always in FreeBSD.
Perhaps there was a problem with bzeroing NOT related to the bss (for
page tables?), but it doesn't seem to be a problem now.  The comment
was added in 2001 not long before gcc invalidated it.  BSS.

These declarations also have many comments that do less than echo the
variable names.

ISTR than there are a few variables in locore that should be declared
here but aren't, since they were put in locore for some bad reason.
bootinfo may be one.  Now, only the PC98 pc98_system_parameter variable
seems to be written to by locore before the bzero.  After fixing this,
only easier control over the layout justifies locore allocating any
non-static variable.

I fixed the comment and removed the explicit initializations long ago.
and haven't noticed any problems:

% Index: initcpu.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/i386/i386/initcpu.c,v
% retrieving revision 1.49
% diff -u -2 -r1.49 initcpu.c
% --- initcpu.c	10 Nov 2003 15:48:30 -0000	1.49
% +++ initcpu.c	11 Nov 2003 08:07:01 -0000
% @@ -77,15 +77,13 @@
%      &hw_instruction_sse, 0, "SIMD/MMX2 instructions available in CPU");
% 
% -/* Must *NOT* be BSS or locore will bzero these after setting them */
% -int	cpu = 0;		/* Are we 386, 386sx, 486, etc? */
% -u_int	cpu_feature = 0;	/* Feature flags */
% -u_int	cpu_high = 0;		/* Highest arg to CPUID */
% -u_int	cpu_id = 0;		/* Stepping ID */
% -u_int	cpu_procinfo = 0;	/* HyperThreading Info / Brand Index / CLFUSH */
% -char	cpu_vendor[20] = "";	/* CPU Origin code */
% -
% +int	cpu;			/* Are we 386, 386sx, 486, etc? */
% +u_int	cpu_feature;		/* Feature flags */
%  #ifdef CPU_ENABLE_SSE
%  u_int	cpu_fxsr;		/* SSE enabled */
%  #endif
% +u_int	cpu_high;		/* Highest arg to CPUID */
% +u_int	cpu_id;			/* Stepping ID */
% +u_int	cpu_procinfo;		/* HyperThreading Info / Brand Index / CLFUSH */
% +char	cpu_vendor[20];		/* CPU Origin code */
% 
%  #ifdef I486_CPU

This fixes some style bugs (unsorted declarations) but not the comments.
There many more variables now, with O(number of new variables) sorting
errors.  The largest sorting error is attaching a nonzero-initialized
variable at the end of this list variables that is misclaimed to need
special handling because they are zero.

Bruce