cvs commit: src/sys/sparc64/include in_cksum.h

[Bruce Evans]

On Fri, 27 Jun 2008, Christoph Mallon wrote:

> Marius Strobl wrote:
>> marius      2008-06-25 21:04:59 UTC
>> 
>>   FreeBSD src repository
>> 
>>   Modified files:
>>     sys/sparc64/include  in_cksum.h   Log:
>>   SVN rev 180011 on 2008-06-25 21:04:59Z by marius
>>     Use "__asm __volatile" rather than "__asm" for instruction sequences
>>   that modify condition codes (the carry bit, in this case). Without
>> ...
>
> This approach seems wrong to me and I think it works only by chance. The 
> condition codes ("cc") should be added to the clobbered list of the asm 
> statement instead of making the statement volatile: __asm("..." : $OUT : $IN 
> : "cc");

> This very case is also mentioned in the GCC documentation:
> "If your assembler instruction can alter the condition code register,
> add `cc' to the list of clobbered registers.  GCC on some machines
> represents the condition codes as a specific hardware register; `cc'
> serves to name this register.  On other machines, the condition code is
> handled differently, and specifying `cc' has no effect.  But it is
> valid no matter what the machine." (Section 5.35 Assembler Instructions with 
> C Expression Operands)

Does sparc64 do anything good with this?  Later it says that this is
irrelevant for the type type of bug in in_cksum.* (expecting cc to
be preserved across separate asms).  From gcc.info:

%  Similarly, you can't expect a sequence of volatile `asm' instructions
% to remain perfectly consecutive.  If you want consecutive output, use a
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
% single `asm'.  Also, GCC will perform some optimizations across a
   ^^^^^^^^^^^^^
% volatile `asm' instruction; GCC does not "forget everything" when it
% encounters a volatile `asm' instruction the way some other compilers do.
% 
%  It is a natural idea to look for a way to give access to the condition
% code left by the assembler instruction.  However, when we attempted to
% implement this, we found no way to make it work reliably.  The problem
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
% is that output operands might need reloading, which would result in
% additional following "store" instructions.  On most machines, these
% instructions would alter the condition code before there was time to
% test it.  This problem doesn't arise for ordinary "test" and "compare"
% instructions because they don't have any output operands.
% 
%  For reasons similar to those described above, it is not possible to
                                                  ^^^^^^^^^^^^^^^^^^^^^
% give an assembler instruction access to the condition code left by
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
% previous instructions.
   ^^^^^^^^^^^^^^^^^^^^^^
On i386, specifying cc has no effect (the compiler must always assume that
cc is clobbered), and specifying cc in asms is a style bug in FreeBSD.

It took about 15 years (1992-2007) for the bugs in in_cksum.* to be
finally fixed on i386, by following the rule given in gcc.info.  IIRC,
this rule wasn't present in 1992, and wasn't really needed then because
the optimizer wasn't agressive enough to move things, especially volatile
asms, but it has been there for about 10 years.

> I wrote a letter directly to Marius about this issue two days ago, but I got 
> no response so far. Because this change has a MFC after 3 days, I'm writing 
> to this list.

I wrote a similar reply with a similar response.

The i386 version, though fixed, still has a lot of historical cruft
related to this bug (bogus volatile declarations, bogus comments about
volatile's affect.  and triplication of in_cksum.c to work around the
bug being so large for INTEL_COMPILER that the volatile hack never
helped).  The amd64 version never had the bug since it was bogusly
duplicated from the ia64 version.  It is just not in asm and thus probably
slow.  It only has a duplicated in_cksum.c.  The MI version in netinet
should be used instead of n-tuplicated, but has rotted.

Bruce