Fix RISC-V a_cas inline asm operand sign extension

Submitted by Luís Marques on Jan. 15, 2020, 1:24 p.m.

Details

Message ID 20200115132438.5809-1-luismarques@lowrisc.org
State New
Series "Fix RISC-V a_cas inline asm operand sign extension"
Headers show

Commit Message

Luís Marques Jan. 15, 2020, 1:24 p.m.
This patch adds an explicit cast to the int arguments passed to the inline asm
used in the RISC-V's implementation of `a_cas`, to ensure that they are properly
sign extended to 64 bits. They aren't automatically sign extended by Clang, and
GCC technically also doesn't guarantee that they will be sign extended.

---
 arch/riscv64/atomic_arch.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/arch/riscv64/atomic_arch.h b/arch/riscv64/atomic_arch.h
index 41ad4d04..0c382588 100644
--- a/arch/riscv64/atomic_arch.h
+++ b/arch/riscv64/atomic_arch.h
@@ -15,7 +15,7 @@  static inline int a_cas(volatile int *p, int t, int s)
 		"	bnez %1, 1b\n"
 		"1:"
 		: "=&r"(old), "=&r"(tmp)
-		: "r"(p), "r"(t), "r"(s)
+		: "r"(p), "r"((long)t), "r"((long)s)
 		: "memory");
 	return old;
 }

Comments

Florian Weimer Jan. 15, 2020, 1:50 p.m.
* Luís Marques:

> This patch adds an explicit cast to the int arguments passed to the inline asm
> used in the RISC-V's implementation of `a_cas`, to ensure that they are properly
> sign extended to 64 bits. They aren't automatically sign extended by Clang, and
> GCC technically also doesn't guarantee that they will be sign extended.
>
> ---
>  arch/riscv64/atomic_arch.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv64/atomic_arch.h b/arch/riscv64/atomic_arch.h
> index 41ad4d04..0c382588 100644
> --- a/arch/riscv64/atomic_arch.h
> +++ b/arch/riscv64/atomic_arch.h
> @@ -15,7 +15,7 @@ static inline int a_cas(volatile int *p, int t, int s)
>  		"	bnez %1, 1b\n"
>  		"1:"
>  		: "=&r"(old), "=&r"(tmp)
> -		: "r"(p), "r"(t), "r"(s)
> +		: "r"(p), "r"((long)t), "r"((long)s)
>  		: "memory");
>  	return old;
>  }

Are casts in this place really casts, and not merely type assertions?
I think you have to use a temporarily variable or maybe a redundant +,
to change the syntax.

Thanks,
Florian
Luís Marques Jan. 15, 2020, 2:18 p.m.
On Wed, Jan 15, 2020 at 1:50 PM Florian Weimer <fweimer@redhat.com> wrote:
> Are casts in this place really casts, and not merely type assertions?
> I think you have to use a temporarily variable or maybe a redundant +,
> to change the syntax.

AFAIK the expression inside the parenthesis is just a regular C
expression, which in this case is a cast expression of a variable.
Here's what the GCC documentation says for the input operands (section
"6.47.2.5 Input Operands"):

"Each operand has this format: [ [asmSymbolicName] ] constraint (cexpression)
(...)
cexpression
This is the C variable or expression being passed to the asm statement
as input."

Is there a particular reason why you believe this might be a type
assertion instead?

Thanks,
Luís
Luís Marques Jan. 22, 2020, 2:31 p.m.
On Wed, Jan 15, 2020 at 1:33 PM Luís Marques <luismarques@lowrisc.org> wrote:
> This patch adds an explicit cast to the int arguments passed to the inline asm
> used in the RISC-V's implementation of `a_cas`, to ensure that they are properly
> sign extended to 64 bits. They aren't automatically sign extended by Clang, and
> GCC technically also doesn't guarantee that they will be sign extended.

Does anyone have any feedback regarding this patch?
If not, perhaps it could be merged?

Best regards,
Luís Marques
Rich Felker Jan. 22, 2020, 2:46 p.m.
On Wed, Jan 22, 2020 at 02:31:25PM +0000, Luís Marques wrote:
> On Wed, Jan 15, 2020 at 1:33 PM Luís Marques <luismarques@lowrisc.org> wrote:
> > This patch adds an explicit cast to the int arguments passed to the inline asm
> > used in the RISC-V's implementation of `a_cas`, to ensure that they are properly
> > sign extended to 64 bits. They aren't automatically sign extended by Clang, and
> > GCC technically also doesn't guarantee that they will be sign extended.
> 
> Does anyone have any feedback regarding this patch?
> If not, perhaps it could be merged?

Thanks for pinging this again. It's unfortunate that clang doesn't do
this right to begin with, but the patch is not terribly ugly and
probably okay.

Can you clarify why it's needed and why sign-extending is the right
thing to do, though? Does lr.w sign-extend the loaded value (vs
zero-extend or something else)?

Rich
Luís Marques Jan. 22, 2020, 4:57 p.m.
On Wed, Jan 22, 2020 at 2:46 PM Rich Felker <dalias@libc.org> wrote:
> Can you clarify why it's needed and why sign-extending is the right
> thing to do, though? Does lr.w sign-extend the loaded value (vs
> zero-extend or something else)?

LR.W does indeed sign-extend the value read from memory, before
storing it in the destination register (the ISA is quite consistent
about sign-extending). The branch operates on the whole 64-bits, so
the value to compare with must also be sign-extended. That's generally
not a problem because the ABI mandates the sign-extension of the
32-bit int to XLEN (64 bits on rv64), and that's something that the
compiler normally does automatically. The exception is the inline
assembler, which in this case becomes a problem when the function is
inlined.

> Thanks for pinging this again. It's unfortunate that clang doesn't do
> this right to begin with, but the patch is not terribly ugly and
> probably okay.

Yeah, this is a tricky issue. Unfortunately, it's a bit more
complicated than just "clang doesn't do the right thing". A patch was
proposed before to try to solve this issue [1], but this is an issue
that already existed for other architectures, and where Clang and GCC
were already inconsistent, both internally and compared to each other.
The difference is that this issue generally only manifested itself
with other C types (e.g. short). We (LLVM) could take a principled
stance where we guaranteed that the values would be properly
sign/zero-extended and truncated back (although my vague recollection
is that that would be tricky, if at all possible), but that wouldn't
guarantee compatibility with GCC, so you couldn't really rely on that
behavior anyway. My understanding is that GCC's sign-extension of the
value is an implementation quirk and not something guaranteed to
happen, and will in fact not happen in other not too dissimilar cases.
And the official stance of the GCC developers is that you can't rely
on it, and a lot of other stuff related to inline assembly. Still,
this is something that we want to improve in the future, because it's
indeed tricky. But the fix might just be to warn of the issue and
request that the value be explicitly casted.

[1] https://reviews.llvm.org/D69212