Re: puzzling code in pcpu stuff

Alban Hertroys wrote:
> 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?

there are no comments in this cut-n-paste because it is the output of 
the C preprocessor.. of course the source doesn't have many comments 
either.. (in i386/include/pcpu.h)


> 
> 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:929,4a75a40b10131060118257!
>