Re: puzzling code in pcpu stuff

On 2 Aug 2009, at 12:34, Christoph Mallon wrote:

> Julian Elischer schrieb:
>> I simplified the output of the preprocessor for a PCPU_SET(xx,  
>> newval)
>> (to look at it).
>> I came down to: (after formatting) for i386..
>> {
>>        __typeof(((struct pcpu *)0)->pc_xx) __val;
>>        struct __s
>>        {
>>                u_char __b[(((sizeof(__val)) < (4)) ?
>>                                   (sizeof(__val)) : (4))];
>>        } __s;
>>        __val = (newval); /* aligned */
>>        if (sizeof(__val) == 1
>>        || sizeof(__val) == 2
>>        || sizeof(__val) == 4) {
>>                __s = *(struct __s *)(void *)&__val;
>>                __asm volatile("mov %1,%%fs:%0" : "=m"
>>                  (*(struct __s *)(__builtin_offsetof(
>>                       struct pcpu,  pc_xx))) : "r" (__s));
>>        } else {
>>                *__extension__ (
>>                {
>>                        __typeof(__val) *__p;
>>                        __asm volatile("movl %%fs:%1,%0;
>>                        addl %2,%0" : "=r" (__p) : "m"
>>  (*(struct pcpu *)(__builtin_offsetof(struct pcpu, pc_prvspace))),
>>  "i"
>>  (__builtin_offsetof(struct pcpu, pc_xx)));
>>   __p;
>>                }) = __val;
>>        }
>> }
>> having had my brain explode on this several times,
>> I can't figure out exactly what teh clause after the else is doing.
>> anyone better at reading __asm better than me care to explain it in
>> simple words?
>
> First, ({}) is a statement expression - a GCC extension (not to be  
> confused with expression statement, which is an expression followed  
> by a semicolon). It wraps a compound statement, i.e. {}, and turns  
> it into an expression. The value of the last statement is the value  
> of the expression. In this case it's __p;.

Speaking as an outsider I'd better be careful with any criticism, but  
the first thing I noticed here was the lack of comments. From Julian's  
question it seems obvious that this function could do with some. I  
wonder what people would make of this in a couple of years when none  
of the (then) active developers has any intimate knowledge of the  
workings of functions like this one?

I got from the posted code sample that __extension__ is probably a no- 
op meant for documentation purposes only. I think it would help to add  
what extension is being used though, maybe as a comment (but what  
would be the use of defining __extension__ then) or as a parameter.  
That's up to you though.

Not being familiar with x86 assembly doesn't help me here of course,  
the last time I touched assembly was on a Z80 a couple of decades  
back...

> Let's have a closer look at the else clause: The asm reads the  
> pointer to per-cpu information into __p and the statement expression  
> returns it. This pointer gets dereferenced (mind the * before  
> __extension__ - __extension__ does nothing, it just marks that the  
> following is a GCC extension) and __val is assigned.


As I read it the value of __val is read into the memory address  
calculated by the expression in the statement expression (__p  
internally), am I right?

Just chiming in with my opinion. I'm sure you'll do fine without it,  
but still.

Alban Hertroys

--
If you can't see the forest for the trees,
cut the trees and you'll see there is no forest.


!DSPAM:930,4a75a40e10131514347140!