🚀 go-pugleaf

I just spent a couple of days looking for a bug in the linux sources
that was "less than obvious", and due to a gcc __asm__ statement
changing memory - without gcc knowing about it, and optimizing away (or
moving) some needed instructions. 

The problem essentially looked like this:

  void clear_inode(struct inode * inode)
  {
	struct wait_queue * tmp;

	tmp = inode->i_wait;
	memset(inode,0,sizeof(*inode));
	inode->i_wait = tmp;
  }

Now, this looks like it simply couldn't fail, so I was searching for the
bug elsewhere for days, until I in desperation looked at the assembly
output of the above:

  _clear_inode:
	pushl %edi
	movl 8(%esp),%edx
	xorb %al,%al
	movl %edx,%edi
	movl $164,%ecx
  /APP
	cld
	rep
	stosb
  /NO_APP
	movl 52(%edx),%eax		<<<<<<
	movl %eax,52(%edx)		<<<<<<
	popl %edi
	ret

Well, the problem was obvious: I was using inline assembly for the
memset routines (as at least gcc-2.2.2 doesn't yet do it by
__builtin_memset).  The exact definition I used for the above code is
(yes, I know - I should re-write it to use long-word clearing):

  extern inline void * memset(void * s,char c,size_t count)
  {
  __asm__ __volatile__("cld\n\t"
	"rep\n\t"
	"stosb"
	::"a" (c),"D" (s),"c" (count)
	:"cx","di");
  return s;
  }

Note that the gcc optimizer hasn't noticed that the inline assembly has
changed the inode data, so it thinks that it can just move the first
assignment to *after* 'memset()'.  Even the __volatile__ doesn't help:
it seems __volatile__ stops only major re-organizations.

Ok, it may not be a bug - the docs do state that you have to be careful
about __asm__ statements with side-effects, but it's a bad feature:
especially as I couldn't find a good workaround.  I tried adding a
output field of the form '"=m" (*s)' to tell gcc that the memory pointed
to by 's' got changed, but this (not surprisingly) only resulted in a
bad type error due to the dereferencing of a void pointer. 

The '"=m" (*s)' trick does work when the type of the pointer is known,
but not with a void pointer (and I can't just cast the void pointer to
something else: it would have to be cast to something of "infinite size"
to be safe).  The optimal solution would be to allow '*s' in this kind
of context, but I don't know how well that meshes with the rest of the
compiler... 

I solved the problem by changing the temporary variable assignments to
be volatile (ie "tmp = ((volatile inode *) inode)->i_wait" etc), so I
got the bug corrected, but I was left wondering if there was nothing
cleaner I could do..  (the only manual I have is a pre-version for 2.0). 

		Linus