Skip to contentSkip to conversation

incorrect asm constraints for ll/sc constructs

PATCH In review Daniel Jacobowitz 4 participants 16 messages May 23, 2001 – May 30, 2001 MIPS

lore
Share & tools
lore

Daniel Jacobowitz

author
#0 lore raw

PATCH 1/1 incorrect asm constraints for ll/sc constructs

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?

Signature · 2 lines
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

mips/kernel/sysmips.c +2 -2
diff -u -r1.18 sysmips.c
@@ -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

asm-mips/system.h +2 -2
diff -u -r1.27 system.h
@@ -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;
#1 lore raw
Quoted 7 lines · Daniel Jacobowitz

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.

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.

Signature · 3 lines
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: [email protected], PGP key available        +
author
#2 lore raw
Quoted 10 lines · Maciej W. Rozycki
Quoted 7 lines · Daniel Jacobowitz

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.

Quoted 4 lines

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.

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?

Quoted 2 lines

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.

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

Signature · 2 lines
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team
#3 lore raw
Quoted 4 lines · Daniel Jacobowitz

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.

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.

Quoted 1 line

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

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

Signature · 3 lines
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: [email protected], PGP key available        +
#4 lore raw
Quoted 3 lines

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,

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

Quoted 15 lines

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;
}

author
#5 lore raw
Quoted 6 lines · Kevin D. Kissell
Quoted 3 lines

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,

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

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

Quoted 19 lines
Quoted 16 lines

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;
}

Signature · 2 lines
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team
#6 lore raw
Quoted 3 lines · Kevin D. Kissell
Quoted 2 lines
Quoted 2 lines

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

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

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"...

Except, of course, gcc doesn't even know the set noat is there. It 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.

#7 lore raw
Quoted 8 lines · Kevin D. Kissell

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.

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

#8 lore raw
In reply to Kevin D. Kissell
Quoted 3 lines · Kevin D. Kissell

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)

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

Quoted 5 lines

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.

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).

Signature · 3 lines
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: [email protected], PGP key available        +
#9 lore raw
Quoted 9 lines
Quoted 5 lines

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.

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.

#10 lore raw
Quoted 2 lines · Kevin D. Kissell

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.

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

Quoted 1 line

Repeating your experiment using -S gives the following results:

Thanks for testing other versions.

Quoted 6 lines

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,

That's clever, indeed...

Quoted 4 lines

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).

... 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

Signature · 3 lines
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: [email protected], PGP key available        +
#11 lore raw
In reply to Maciej W. Rozycki
Quoted 8 lines · Maciej W. Rozycki
Quoted 7 lines

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.

Quoted 4 lines

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.

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

Ralf

author
#12 lore raw
Quoted 10 lines · Ralf Baechle
Quoted 8 lines · Maciej W. Rozycki
Quoted 7 lines

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.

Quoted 7 lines
Quoted 4 lines

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.

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

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.

Signature · 2 lines
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

mips/kernel/sysmips.c +2 -2
diff -u -r1.18 sysmips.c
@@ -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), "=R" (* (u32 *) p), "=r" (errno)
+			: "r" (arg2), "R" (* (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

asm-mips/system.h +2 -2
diff -u -r1.27 system.h
@@ -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), "=R" (*m), "=r" (dummy)
+		: "R" (*m), "2" (val)
 		: "memory");
 
 	return val;
#13 lore raw
Quoted 3 lines · Daniel Jacobowitz

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.

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.

Signature · 3 lines
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: [email protected], PGP key available        +
author
#14 lore raw
In reply to Daniel Jacobowitz
Quoted 11 lines · Daniel Jacobowitz
Quoted 10 lines · Ralf Baechle
Quoted 8 lines · Maciej W. Rozycki
Quoted 7 lines

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.

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

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.

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.

Signature · 2 lines
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team
#15 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

The ll/sc constructs in the kernel use ".set noat" to inhibit use

of $at,

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

and proceed to use it themselves. This is fine, except for one

problem: the

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

constraints on memory operands are "o" and "=o", which means

offsettable

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

memory references. If I'm not mistaken, the assembler will

(always?)

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

turn these into uses of $at if the offset is not 0 - at least, it

certainly

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

seems to do that here (gcc 2.95.3, binutils 2.10.91.0.2). Just

being honest

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

with the compiler and asking for a real memory reference does the

trick.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Both "m" and "o" seem to be incorrect here as both are the same for

MIPS;

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"R" seems to be appropriate, OTOH. Still gcc 2.95.3 doesn't handle

"R"

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

fine for all cases, but it works most of the time and emits a

warning

Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

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)

Quoted 4 lines

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.

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.