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

[Christoph Mallon]

Bruce Evans wrote:
> 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:

I think the bug was the following:
subcc %foo, %bar, %g0 /* SPARC compare */
#APP
/* inline assembler of in_addword() here, which modifies the condition 
codes */
#NO_APP
bpl $somewhere /* condition branch depending on condition code */

The bpl is supposed to jump depending on the condition of the subcc, but 
if the compiler schedules the inline assembler block between the subcc 
and the bpl, it jumps depending on garbage.
It was not expected to preserve the condition codes across separate 
inline assembler blocks, but it was not specified that the single inline 
assembler block did not modify the condition codes.

> %  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.
>   ^^^^^^^^^^^^^^^^^^^^^^

I think the excerpt does not apply here, because the problem is the 
other way round (inline assembler interrupting C, not C interrupting 
multiple inline assembler blocks), see above.

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

I have to disagree. Where does the GCC documentation state, that "cc" 
has no effect on x86? It is the other way round: The compiler assumes, 
the condition codes are *not* modified, if it is not explicitely stated. 
Exactly this caused the bug (though here on SPARC), which was tried to 
be solved by volatile. I still am convinced that specifying "cc" in the 
clobber list and not using volatile is the correct solution. If it is a 
style bug to specify "cc", the style should be changed, otherwise you 
cannot use inline assembler correctly.

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

As far as I can see the rule in the gcc documentation was not followed 
in the x86 version: The assembler block is volatile and "cc" is not 
specified as clobbered. This, imo, is a bug.

Regards
	Christoph