This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[patch, rfc] store motion and volatile asms


Hi,

we tripped over this error actually with the hammer branch (were we
enabled gcse-sm) on the linux kernel, but it's still hiding in 3.4 and
4.x.  This function is miscompiled by each of them (a variation of it was
the actually failing one, the asm is a hand-coded strncmp in reality):

    void check (void)
    {
        char str[9];
        int res;
        *(int*)str = 42;  // #1
        __asm__ __volatile__("movl (%1), %0" : "=r" (res) : "r" (&str[0]));
        if (res == 42) {
          global--;
        } else {
          *(int*)str = 42;  // #2
          consume(str);
        }
    }

The problem is, that the store at #1 is moved downwards into the if tree, 
i.e. over the asm.

Now, my initial reaction to this was, that they should add a "memory" 
clobber to the asm, or use a "m" operand making it explicit what memory is 
actually accessed in which way.  But then I thought that this is a 
volatile asm, and someone expecting that nothing moves over such asm 
probably does have a point.  Explicitely having to add a memory clobber 
feels a bit to redundant.  Most users writing asms probably are aware of 
the need for volatile sometimes, but not many of them are aware that this 
is not enough.  So I suspect a large (relative to those using inline 
asm at all) code base which currently is broken in that respect.  The 
linux kernel is for certain.  Furthermore the user above might feel that 
he actually _did_ mention the memory operand, on the grounds that he has 
given the address of 'str' to the asm.  What purposes does giving the 
address to something have, if not for accessing the memory behind.  Yes, 
that reasoning is wrong, but I can sympathize with it.

I've looked at various places in GCC and sometimes we actually do prevent
code moving over volatile asms (e.g. scheduler, or combine), and
sometimes we just ignore this.  It seems to be the case that old code did
prevent it, and newer code did not.  At least the whole gcse.c file is not
checking ASM_OPERANDS && MEM_VOLATILE_P.  Not to speak of the tree-ssa
passes.

The patch below prevents store motion from moving something over volatile 
asms, like is done in various other parts of the compiler.  It fixes the 
testcase.  But looking at other parts of the compiler I think that this 
would not be a complete solution.  For instance load motion also does not 
test this, and one could probably construct a testcase failing that too.

So another idea would be to use a big hammer and create an implicit 
"memory" clobber for each volatile asm, which probably matches the 
expectation of most people who are not reading gcc{,-patches}@ .  I've no 
idea about performance effects, but I would reason that people caring for 
performance should write really exact operands, obviating the need to put 
a volatile at the asm.

What do people think about this?  Should this be a "go away, fix your 
asms", the partial solution (plus auditing the rest of gcc for similar 
errors), or the big hammer?

I'll bootstrap/regtest the patch, and would like to see it in 4.0 and 4.1 
if it passes, if not the big hammer is the way to go.


Ciao,
Michael.
-- 
/* -O2 -fgcse-sm */
extern void abort(void);
int global;

void __attribute__((noinline)) consume (const char* c)
{
  global++;
}

void check (void)
{
        char str[9];
	int res;

        *(int*)str = 42;
        *(int*)(str+4) = 0;

	__asm__ __volatile__("movl (%1), %0" : "=r" (res) : "r" (&str[0]));
	if (res == 42) {
          global--;
        } else {
          *(int*)str = 42;
          *(int*)(str+4) = 0;
	  consume(str);
	}
}

int main()
{
  global = 1;
  check();
  if (global)
    abort();
}

Index: gcse.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gcse.c,v
retrieving revision 1.335
diff -u -p -r1.335 gcse.c
--- gcse.c	24 Feb 2005 21:47:24 -0000	1.335
+++ gcse.c	29 Jun 2005 01:38:10 -0000
@@ -5830,6 +5830,9 @@ find_loads (rtx x, rtx store_pattern, in
 	return true;
     }
 
+  if (GET_CODE (x) == ASM_OPERANDS && MEM_VOLATILE_P (x))
+    return true;
+
   /* Recursively process the insn.  */
   fmt = GET_RTX_FORMAT (GET_CODE (x));
 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]