Subject: [PATCH] incorrect asm constraints for ll/sc constructs

From: Daniel Jacobowitz <[email-masked]>
Date: Wed, 23 May 2001 14:52:57 -0700

The ll/sc constructs in the kernel use ".set noat" to inhibit use of $at,
and proceed to use it themselves.  This is fine, except for one problem: the
constraints on memory operands are "o" and "=o", which means offsettable
memory references.  If I'm not mistaken, the assembler will (always?)
turn these into uses of $at if the offset is not 0 - at least, it certainly
seems to do that here (gcc 2.95.3, binutils 2.10.91.0.2).  Just being honest
with the compiler and asking for a real memory reference does the trick. 
Does this patch look right?

-- 
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team
Index: arch/mips/kernel/sysmips.c
===================================================================
RCS file: /cvs/linux/arch/mips/kernel/sysmips.c,v
retrieving revision 1.18
diff -u -r1.18 sysmips.c
--- arch/mips/kernel/sysmips.c	2001/04/08 13:24:27	1.18
+++ arch/mips/kernel/sysmips.c	2001/05/23 21:49:29
@@ -99,8 +99,8 @@
 			".word\t1b, 3b\n\t"
 			".word\t2b, 3b\n\t"
 			".previous\n\t"
-			: "=&r" (tmp), "=o" (* (u32 *) p), "=r" (errno)
-			: "r" (arg2), "o" (* (u32 *) p), "2" (errno)
+			: "=&r" (tmp), "=m" (* (u32 *) p), "=r" (errno)
+			: "r" (arg2), "m" (* (u32 *) p), "2" (errno)
 			: "$1");
 
 		if (errno)
Index: include/asm-mips/system.h
===================================================================
RCS file: /cvs/linux/include/asm-mips/system.h,v
retrieving revision 1.27
diff -u -r1.27 system.h
--- include/asm-mips/system.h	2001/03/28 01:35:12	1.27
+++ include/asm-mips/system.h	2001/05/23 21:49:29
@@ -219,8 +219,8 @@
 		" ll\t%0, %3\n\t"
 		".set\tat\n\t"
 		".set\treorder"
-		: "=r" (val), "=o" (*m), "=r" (dummy)
-		: "o" (*m), "2" (val)
+		: "=r" (val), "=m" (*m), "=r" (dummy)
+		: "m" (*m), "2" (val)
 		: "memory");
 
 	return val;

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Thu, 24 May 2001 15:42:56 +0200 (MET DST)

> with the compiler and asking for a real memory reference does the trick. 

 Both "m" and "o" seem to be incorrect here as both are the same for MIPS; 
"R" seems to be appropriate, OTOH.  Still gcc 2.95.3 doesn't handle "R" 
fine for all cases, but it works most of the time and emits a warning
otherwise.  I can't comment on 3.0.

 Note that if noat is in effect and at is to be used, gas should bail out
with an error.  There is a bug, if it doesn't.

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: [email-masked], PGP key available        +

---

From: Daniel Jacobowitz <[email-masked]>
Date: Thu, 24 May 2001 16:44:59 -0700

> otherwise.  I can't comment on 3.0.

They aren't the same for MIPS, though.  I exhibit as evidence the fact
that my patch fixed the problem I was seeing.  I didn't know about 'R';
I suppose that it is more correct.  'm' at least is closer than 'o',
though.

If 'R' will behave correctly, could that be applied to CVS, then?

> with an error.  There is a bug, if it doesn't.

It issues a warning, currently (2.10.91.0.2 - I think 2.11 behaves the
same).

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Fri, 25 May 2001 15:13:04 +0200 (MET DST)

> I suppose that it is more correct.  'm' at least is closer than 'o',

 The following program cannot be compiled with gcc 2.95.3, because the
offset is out of range (I consider it a bug in gcc -- it should allocate
and load a temporary register itself and pass it appropriately as %0,
matching the "R" constraint; still it's better than generating bad code): 

int main(void)
{
	int *p;

	asm volatile(".set push\n\t"
 		".set noat\n\t"
		"lw $0,%0\n\t"
		".set pop"
		:
		: "R" (p[0x10000]));

	return 0;
}

After changing "R" to "m" or "o", bad assembly is generated if optimizing
as follows: 

 #APP
	.set push
	.set noat
	lw $0,262144($2)
	.set pop
 #NO_APP

Note that it's an expected behaviour -- there are no non-offsettable
address modes for MIPS.

> If 'R' will behave correctly, could that be applied to CVS, then?

 I suppose so -- I'm not in a position to apply changes.

---

From: Ralf Baechle <[email-masked]>
Date: Fri, 25 May 2001 17:27:46 -0300

> otherwise.  I can't comment on 3.0.

I admit the construction is somewhat fragile and will take any patches to
cleanup this.

  Ralf

---

From: Daniel Jacobowitz <[email-masked]>
Date: Fri, 25 May 2001 13:49:09 -0700

> I admit the construction is somewhat fragile and will take any patches to

How about the attached, then?  If the p[0x100000] case is of sufficient
concern, we can work around that too, but this catches all current
uses.

+			: "=&r" (tmp), "=R" (* (u32 *) p), "=r" (errno)
+			: "r" (arg2), "R" (* (u32 *) p), "2" (errno)
 			: "$1");
 
 		if (errno)
+		: "=r" (val), "=R" (*m), "=r" (dummy)
+		: "R" (*m), "2" (val)
 		: "memory");
 
 	return val;

---

From: "Kevin D. Kissell" <[email-masked]>
Date: Fri, 25 May 2001 23:15:48 +0200

> and load a temporary register itself and pass it appropriately as %0,

I think gcc can be forgiven for not allocating a temporary,
given the ".set noat"...

---

From: Daniel Jacobowitz <[email-masked]>
Date: Fri, 25 May 2001 14:49:37 -0700

> I think gcc can be forgiven for not allocating a temporary,

Except, of course, gcc doesn't even know the set noat is there.  It
doesn't parse the interior of asm() statements.

---

From: "Kevin D. Kissell" <[email-masked]>
Date: Sun, 27 May 2001 00:14:43 +0200

> doesn't parse the interior of asm() statements.

Fair enough.  It was an offhand remark.  But seriously, what does
the "R" constraint mean here?  The only documentation I've got
(http://linux.fh-heilbronn.de/doku/GNU/docs/gcc/gcc_163.html#SEC163)
says that "Q" through "U" are reserved for use with EXTRA_CONSTRAINT
in machine-dependent definitions of arbitrary operand types.  When
and where does it get bound for MIPS gcc, and what is it supposed
to mean?  If I compile this kind of fragment using a "m" constraint,
it seems to do the right thing, at least on my archaic native compiler.

            Kevin K.

---

From: Ralf Baechle <[email-masked]>
Date: Sat, 26 May 2001 19:23:40 -0300

> it seems to do the right thing, at least on my archaic native compiler.

Correct, "R" is a machine dependent constraint.  At least when it's working
right it's supposed to expand into offset(reg) where offset is limited
to 16 bits.  That's implemented in gcc/config/gcc/mips/mips.h's
EXTRA_CONSTRAINT macro.  In case of an "R" constraint gcc calls the
simple_memory_operand() function which will return 1 if the memory operand
fits into a single instruction.

  Ralf

---

From: "Kevin D. Kissell" <[email-masked]>
Date: Mon, 28 May 2001 15:48:09 +0200

>  Is it gcc generating right code or gas expanding a macro?  Try `gcc -S'
> -- for me "m" generates "lw $0,262144($2)", which is unacceptable when
> ".set noat" is in effect (and perfectly fine otherwise).

I'd been disassembling the resulting .o files, as I didn't care whether
it's the compiler or the assembler that ultimately makes things right.
Repeating your experiment using -S gives the following results:

egcs-2.90.29 (native) and
egcs-2.91.66 (x86 cross) : Optimiser produces "impossible" load
                                              offset you describe if "m"  or
"o" constraint,
                                              compiler barfs if "R"
constraint.

gcc 2.96-mips3264-000710 from Algorithmics (x86 cross):
                                              Compiler generates "correct"
code
                                              (Address is calculated with an
explicit
                                               add prior to the asm
expansion) for
                                               any of the three constraints.

However, if one compiles all the way to object code and looks
at what the assembler is actually doing with those "impossible"
offsets under gcc 2.90 and 2.91, technically, it's not violating ".noat"
in the "m" and "o" constraint  cases.   It is *not* using the "at" register.
It is, however, cleverly using the load destination  register as a temporary
to calculate  the correct address.  As there are no memory operations,
these instructions should have no effect  on the correct execution
of the ll/sc sequence  (though they will  increase the statistical
probability
of a context  switch between ll and sc).

            Regards,

            Kevin K.

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 28 May 2001 13:20:41 +0200 (MET DST)

> (http://linux.fh-heilbronn.de/doku/GNU/docs/gcc/gcc_163.html#SEC163)

 `info gcc' has most relevant data, at least for 2.95.3.

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 28 May 2001 13:09:49 +0200 (MET DST)

> concern, we can work around that too, but this catches all current

 Fine for me.  Don't worry of the gcc bug -- such large offests are rare
and I doubt anyone will get hurt.  Hopefully gcc will get fixed meanwhile,
either by me or by someone else.

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 28 May 2001 13:20:41 +0200 (MET DST)

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 28 May 2001 15:59:29 +0200 (MET DST)

> it's the compiler or the assembler that ultimately makes things right.

 It's good to check the generated assembly if you suspect a tool bug.

> Repeating your experiment using -S gives the following results:

 Thanks for testing other versions.

> to calculate  the correct address.  As there are no memory operations,

 That's clever, indeed...

> of a context  switch between ll and sc).

 ... but that won't work for a lone store, so we need a properly working
'R' constraint in the compiler.  Since 3.0 works, as you report, there is
no need to worry (but I might consider backporting changes to 2.95.3).

  Maciej

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 28 May 2001 15:59:29 +0200 (MET DST)

That's clever, indeed...

  Maciej

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 28 May 2001 15:59:29 +0200 (MET DST)

That's clever, indeed...

  Maciej

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 28 May 2001 15:59:29 +0200 (MET DST)

That's clever, indeed...

  Maciej

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 28 May 2001 15:59:29 +0200 (MET DST)

That's clever, indeed...

  Maciej

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 28 May 2001 15:59:29 +0200 (MET DST)

That's clever, indeed...

  Maciej

---

From: Daniel Jacobowitz <[email-masked]>
Date: Tue, 29 May 2001 17:17:48 -0700

> > > otherwise.  I can't comment on 3.0.

Back to quibbling - that's just not true.  For one thing, from the info
documentation:
    `R'   
          Memory reference that can be loaded with one instruction (`m'
          is preferable for `asm' statements)

For another, using the patch I posted below, I get inconsistent
constraint errors.  I'm not entirely sure why.  Is there any reason not
to use the "m" version?  I can't see any case in which it would not
behave correctly.

---

From: "Kevin D. Kissell" <[email-masked]>
Date: Wed, 30 May 2001 09:02:15 +0200

> to use the "m" version?  I can't see any case in which it would not

There seems to be a bug in many older gcc's, where if you use
"m" or "o", and the effective address requires more than 16 bits
of offset relative to an available base register, store address
calculations override the "noat" setting.  Load address calculations
are at least smart enough to use the load destination as the
temporary register.  Of the compilers that I was able to test,
"R" constraint worked only on the most recent gcc version.
And with that compiler, "m" also generated correct code.
The "m" constraint also worked on earlier gcc's,  but
violated the noat constraint on stores.  The compilers where
"m" violated noat were also those where the "R" constraint
was rejected as being inconsistent.  None of the compilers tested
generated correct code for "R" but incorrect code for "m".

So it could be argued that "m" constraints will in fact
function with a broader range of compilers, albeit with
the quirk that a "noat" override warning may be produced
in the case of older gcc's.

            Kevin K.