[PATCH] lib: utils/serial: Fix semihosting compile error using LLVM

[PATCH] lib: utils/serial: Fix semihosting compile error using LLVM

[Anup Patel]

We fix the following semihosting compile error observed using LLVM:
lib/utils/serial/semihosting.c:158:12: error: result of comparison of constant -1 with expression of type 'char' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
                ret = ch > -1 ? ch : -1;
                      ~~ ^ ~~

Fixes: 7f09fba86e43 ("lib: utils/serial: add semihosting support")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 lib/utils/serial/semihosting.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c
index 5012fa1..72bbbb3 100644
--- a/lib/utils/serial/semihosting.c
+++ b/lib/utils/serial/semihosting.c
@@ -155,7 +155,7 @@ static int semihosting_getc(void)
 
 	if (semihosting_infd < 0)  {
 		ch = semihosting_trap(SYSREADC, NULL);
-		ret = ch > -1 ? ch : -1;
+		ret = ((int)ch > -1) ? ch : -1;
 	} else
 		ret = semihosting_read(semihosting_infd, &ch, 1) > 0 ? ch : -1;
 
-- 
2.34.1

[PATCH] lib: utils/serial: Fix semihosting compile error using LLVM

[Andreas Schwab]

On Nov 08 2022, Anup Patel wrote:

> diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c
> index 5012fa1..72bbbb3 100644
> --- a/lib/utils/serial/semihosting.c
> +++ b/lib/utils/serial/semihosting.c
> @@ -155,7 +155,7 @@ static int semihosting_getc(void)
>  
>  	if (semihosting_infd < 0)  {
>  		ch = semihosting_trap(SYSREADC, NULL);

This is using the wrong variable to record the return value of
semihosting_trap (which is of type long, not char).

> -		ret = ch > -1 ? ch : -1;
> +		ret = ((int)ch > -1) ? ch : -1;

I think this line can be removed, and the return value of
semihosting_trap can be returned directly.

-- 
Andreas Schwab, SUSE Labs, schwab at suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCH] lib: utils/serial: Fix semihosting compile error using LLVM

[Xiang W]

? 2022-11-08???? 08:55 +0530?Anup Patel???
> We fix the following semihosting compile error observed using LLVM:
> lib/utils/serial/semihosting.c:158:12: error: result of comparison of constant -1 with expression of type 'char' is always true [-Werror,-
> Wtautological-constant-out-of-range-compare]
> ??????????????? ret = ch > -1 ? ch : -1;
> ????????????????????? ~~ ^ ~~
> 
> Fixes: 7f09fba86e43 ("lib: utils/serial: add semihosting support")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
> ?lib/utils/serial/semihosting.c | 2 +-
> ?1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c
> index 5012fa1..72bbbb3 100644
> --- a/lib/utils/serial/semihosting.c
> +++ b/lib/utils/serial/semihosting.c
> @@ -155,7 +155,7 @@ static int semihosting_getc(void)
> ?
> ????????if (semihosting_infd < 0)? {
> ????????????????ch = semihosting_trap(SYSREADC, NULL);
> -???????????????ret = ch > -1 ? ch : -1;
> +???????????????ret = ((int)ch > -1) ? ch : -1;
> ????????} else
> ????????????????ret = semihosting_read(semihosting_infd, &ch, 1) > 0 ? ch : -1;

There is a bug here. This line should be changed to:
                ret = semihosting_read(semihosting_infd, &ch, 1) == 0 ? ch : -1;

The return value of semihosting_read is buffer_length - bytes_read. Not the length
of characters read. The following is the code comment from openocd about SYS_READ. 

		case SEMIHOSTING_SYS_READ:	/* 0x06 */
			/*
			 * Reads the contents of a file into a buffer. The file position
			 * is specified either:
			 * - Explicitly by a SYS_SEEK.
			 * - Implicitly one byte beyond the previous SYS_READ or
			 * SYS_WRITE request.
			 *
			 * The file position is at the start of the file when it is
			 * opened, and is lost when the file is closed. Perform the
			 * file operation as a single action whenever possible. For
			 * example, do not split a read of 16KB into four 4KB chunks
			 * unless there is no alternative.
			 *
			 * Entry
			 * On entry, the PARAMETER REGISTER contains a pointer to a
			 * three-field data block:
			 * - field 1 Contains a handle for a file previously opened
			 * with SYS_OPEN.
			 * - field 2 Points to a buffer.
			 * - field 3 Contains the number of bytes to read to the buffer
			 * from the file.
			 *
			 * Return
			 * On exit, the RETURN REGISTER contains the number of bytes not
			 * filled in the buffer (buffer_length - bytes_read) as follows:
			 * - If the RETURN REGISTER is 0, the entire buffer was
			 * successfully filled.
			 * - If the RETURN REGISTER is the same as field 3, no bytes
			 * were read (EOF can be assumed).
			 * - If the RETURN REGISTER contains a value smaller than
			 * field 3, the read succeeded but the buffer was only partly
			 * filled. For interactive devices, this is the most common
			 * return value.
			 */

> ?
> -- 
> 2.34.1
> 
> 

[PATCH] lib: utils/serial: Fix semihosting compile error using LLVM

[Xiang W]

? 2022-11-08???? 10:02 +0100?Andreas Schwab???
> On Nov 08 2022, Anup Patel wrote:
> 
> > diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c
> > index 5012fa1..72bbbb3 100644
> > --- a/lib/utils/serial/semihosting.c
> > +++ b/lib/utils/serial/semihosting.c
> > @@ -155,7 +155,7 @@ static int semihosting_getc(void)
> > ?
> > ????????if (semihosting_infd < 0)? {
> > ????????????????ch = semihosting_trap(SYSREADC, NULL);
> 
> This is using the wrong variable to record the return value of
> semihosting_trap (which is of type long, not char).
> 
> > -???????????????ret = ch > -1 ? ch : -1;
> > +???????????????ret = ((int)ch > -1) ? ch : -1;
> 
> I think this line can be removed, and the return value of
> semihosting_trap can be returned directly.

This line is used to handle error codes, semihosting_trap may
have multiple return values less than 0, but console_getc can
only return -1.

Suggest:
		ret = semihosting_trap(SYSREADC, NULL);
		ret = ret < 0 ? -1 : ret;

Regards,
Xiang W
> 
> -- 
> Andreas Schwab, SUSE Labs, schwab at suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE? 1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."
> 

[PATCH] lib: utils/serial: Fix semihosting compile error using LLVM

[Anup Patel]

On Tue, Nov 8, 2022 at 2:32 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Nov 08 2022, Anup Patel wrote:
>
> > diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c
> > index 5012fa1..72bbbb3 100644
> > --- a/lib/utils/serial/semihosting.c
> > +++ b/lib/utils/serial/semihosting.c
> > @@ -155,7 +155,7 @@ static int semihosting_getc(void)
> >
> >       if (semihosting_infd < 0)  {
> >               ch = semihosting_trap(SYSREADC, NULL);
>
> This is using the wrong variable to record the return value of
> semihosting_trap (which is of type long, not char).

I agree with you. We should use a different variable instead of ch.

>
> > -             ret = ch > -1 ? ch : -1;
> > +             ret = ((int)ch > -1) ? ch : -1;
>
> I think this line can be removed, and the return value of
> semihosting_trap can be returned directly.

This is a legacy call where we are supposed to return -1 for failure.
(Refer, https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/riscv-sbi.adoc#extension-console-getchar-eid-0x02)

Regards,
Anup

[PATCH] lib: utils/serial: Fix semihosting compile error using LLVM

[Anup Patel]

On Tue, Nov 8, 2022 at 3:19 PM Xiang W <wxjstz@126.com> wrote:
>
> ? 2022-11-08???? 10:02 +0100?Andreas Schwab???
> > On Nov 08 2022, Anup Patel wrote:
> >
> > > diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c
> > > index 5012fa1..72bbbb3 100644
> > > --- a/lib/utils/serial/semihosting.c
> > > +++ b/lib/utils/serial/semihosting.c
> > > @@ -155,7 +155,7 @@ static int semihosting_getc(void)
> > >
> > >         if (semihosting_infd < 0)  {
> > >                 ch = semihosting_trap(SYSREADC, NULL);
> >
> > This is using the wrong variable to record the return value of
> > semihosting_trap (which is of type long, not char).
> >
> > > -               ret = ch > -1 ? ch : -1;
> > > +               ret = ((int)ch > -1) ? ch : -1;
> >
> > I think this line can be removed, and the return value of
> > semihosting_trap can be returned directly.
>
> This line is used to handle error codes, semihosting_trap may
> have multiple return values less than 0, but console_getc can
> only return -1.
>
> Suggest:
>                 ret = semihosting_trap(SYSREADC, NULL);
>                 ret = ret < 0 ? -1 : ret;

Andreas suggested a similar thing as well so I will send v2 patch.

Regards,
Anup

>
> Regards,
> Xiang W
> >
> > --
> > Andreas Schwab, SUSE Labs, schwab at suse.de
> > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> > "And now for something completely different."
> >
>
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi