mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH] Fix RISC-V a_cas inline asm operand sign extension
@ 2020-01-15 13:24 Luís Marques
  2020-01-15 13:50 ` Florian Weimer
  2020-01-22 14:31 ` [musl] " Luís Marques
  0 siblings, 2 replies; 6+ messages in thread
From: Luís Marques @ 2020-01-15 13:24 UTC (permalink / raw)
  To: musl; +Cc: 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;
 }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] [PATCH] Fix RISC-V a_cas inline asm operand sign extension
  2020-01-15 13:24 [musl] [PATCH] Fix RISC-V a_cas inline asm operand sign extension Luís Marques
@ 2020-01-15 13:50 ` Florian Weimer
  2020-01-15 14:18   ` Luís Marques
  2020-01-22 14:31 ` [musl] " Luís Marques
  1 sibling, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2020-01-15 13:50 UTC (permalink / raw)
  To: Luís Marques; +Cc: musl

* 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] [PATCH] Fix RISC-V a_cas inline asm operand sign extension
  2020-01-15 13:50 ` Florian Weimer
@ 2020-01-15 14:18   ` Luís Marques
  0 siblings, 0 replies; 6+ messages in thread
From: Luís Marques @ 2020-01-15 14:18 UTC (permalink / raw)
  To: Florian Weimer; +Cc: musl

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [musl] Re: [PATCH] Fix RISC-V a_cas inline asm operand sign extension
  2020-01-15 13:24 [musl] [PATCH] Fix RISC-V a_cas inline asm operand sign extension Luís Marques
  2020-01-15 13:50 ` Florian Weimer
@ 2020-01-22 14:31 ` Luís Marques
  2020-01-22 14:46   ` Rich Felker
  1 sibling, 1 reply; 6+ messages in thread
From: Luís Marques @ 2020-01-22 14:31 UTC (permalink / raw)
  To: musl

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] Re: [PATCH] Fix RISC-V a_cas inline asm operand sign extension
  2020-01-22 14:31 ` [musl] " Luís Marques
@ 2020-01-22 14:46   ` Rich Felker
  2020-01-22 16:57     ` Luís Marques
  0 siblings, 1 reply; 6+ messages in thread
From: Rich Felker @ 2020-01-22 14:46 UTC (permalink / raw)
  To: Luís Marques; +Cc: musl

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] Re: [PATCH] Fix RISC-V a_cas inline asm operand sign extension
  2020-01-22 14:46   ` Rich Felker
@ 2020-01-22 16:57     ` Luís Marques
  0 siblings, 0 replies; 6+ messages in thread
From: Luís Marques @ 2020-01-22 16:57 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-01-22 19:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15 13:24 [musl] [PATCH] Fix RISC-V a_cas inline asm operand sign extension Luís Marques
2020-01-15 13:50 ` Florian Weimer
2020-01-15 14:18   ` Luís Marques
2020-01-22 14:31 ` [musl] " Luís Marques
2020-01-22 14:46   ` Rich Felker
2020-01-22 16:57     ` Luís Marques

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).