Date: Sun, 02 Aug 2009 09:55:42 -0700 From: Julian Elischer <julian@elischer.org> To: Alban Hertroys <dalroi@solfertje.student.utwente.nl> Cc: Christoph Mallon <christoph.mallon@gmx.de>, FreeBSD Current <current@freebsd.org> Subject: Re: puzzling code in pcpu stuff Message-ID: <4A75C50E.5020203@elischer.org> In-Reply-To: <9A2BA686-016B-4B60-A247-7321C1E7F51A@solfertje.student.utwente.nl> References: <4A756214.8010002@elischer.org> <4A756BA1.90002@gmx.de> <9A2BA686-016B-4B60-A247-7321C1E7F51A@solfertje.student.utwente.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
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! >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4A75C50E.5020203>