[PATCH v2] gas: fix building tc-bpf.c on s390x

[PATCH v2] gas: fix building tc-bpf.c on s390x

[Ilya Leoshkevich]

char is unsigned on s390x, so there are a lot of warnings like:

    gas/config/tc-bpf.c: In function 'get_token':
    gas/config/tc-bpf.c:900:14: error: comparison is always false due to limited range of data type [-Werror=type-limits]
      900 |       if (ch == EOF || len > MAX_TOKEN_SZ)
          |              ^~

Change its type to int, like in the other similar code.

There is also:

    gas/config/tc-bpf.c:735:30: error: 'bpf_endianness' may be used uninitialized in this function [-Werror=maybe-uninitialized]
      735 |    dst, be ? size[endianness - BPF_BE16] : size[endianness - BPF_LE16]);
          |                   ~~~~~~~~~~~^~~~~~~~~~

-Wmaybe-uninitialized doesn't seem to understand the FSM; just
initialize bpf_endianness to silence it.  Add an assertion to
build_bpf_endianness() in order to catch potential bugs.
---

v1: https://sourceware.org/pipermail/binutils/2023-April/127235.html
v1 -> v2: Change signed char to int (Jan).
          Add an assertion.

 gas/config/tc-bpf.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gas/config/tc-bpf.c b/gas/config/tc-bpf.c
index 171fc682806..3b86f9c89cb 100644
--- a/gas/config/tc-bpf.c
+++ b/gas/config/tc-bpf.c
@@ -730,6 +730,8 @@ build_bpf_endianness (char *dst, enum bpf_token_type endianness)
       || endianness == BPF_LE32
       || endianness == BPF_LE64)
     be = 0;
+  else
+    gas_assert (endianness == BPF_BE16 || endianness == BPF_BE32 || endianness == BPF_BE64);
 
   bpf_insn = xasprintf ("%s %%%s,%s", be ? "endbe" : "endle",
 			dst, be ? size[endianness - BPF_BE16] : size[endianness - BPF_LE16]);
@@ -885,7 +887,7 @@ get_token (const char **insn, char *token, size_t *tlen)
     } while (0)
 
   const char *str = *insn;
-  char ch, ch2 = 0;
+  int ch, ch2 = 0;
   enum bpf_token_type ttype = BPF_UNKNOWN;
   size_t len = 0;
   const char *expr = NULL;
@@ -1362,7 +1364,7 @@ bpf_pseudoc_to_normal_syntax (const char *str, char **errmsg)
     } while (0)
 
   enum bpf_token_type ttype;
-  enum bpf_token_type bpf_endianness,
+  enum bpf_token_type bpf_endianness = BPF_UNKNOWN,
 		      bpf_atomic_insn;
   enum bpf_token_type bpf_jmp_op = BPF_JEQ; /* Arbitrary.  */
   enum bpf_token_type bpf_cast = BPF_CAST_U8; /* Arbitrary.  */
-- 
2.40.0

Re: [PATCH v2] gas: fix building tc-bpf.c on s390x

[Jan Beulich]

On 03.05.2023 14:02, Ilya Leoshkevich wrote:
> char is unsigned on s390x, so there are a lot of warnings like:
> 
>     gas/config/tc-bpf.c: In function 'get_token':
>     gas/config/tc-bpf.c:900:14: error: comparison is always false due to limited range of data type [-Werror=type-limits]
>       900 |       if (ch == EOF || len > MAX_TOKEN_SZ)
>           |              ^~
> 
> Change its type to int, like in the other similar code.
> 
> There is also:
> 
>     gas/config/tc-bpf.c:735:30: error: 'bpf_endianness' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>       735 |    dst, be ? size[endianness - BPF_BE16] : size[endianness - BPF_LE16]);
>           |                   ~~~~~~~~~~~^~~~~~~~~~
> 
> -Wmaybe-uninitialized doesn't seem to understand the FSM; just
> initialize bpf_endianness to silence it.  Add an assertion to
> build_bpf_endianness() in order to catch potential bugs.
> ---
> 
> v1: https://sourceware.org/pipermail/binutils/2023-April/127235.html
> v1 -> v2: Change signed char to int (Jan).
>           Add an assertion.

Looks okay to me now, but will want Jose's approval.

I'm a little puzzled, btw, why e.g. bpf_atomic_insn doesn't also expose
a "maybe uninitialized" warning.

Jan

> --- a/gas/config/tc-bpf.c
> +++ b/gas/config/tc-bpf.c
> @@ -730,6 +730,8 @@ build_bpf_endianness (char *dst, enum bpf_token_type endianness)
>        || endianness == BPF_LE32
>        || endianness == BPF_LE64)
>      be = 0;
> +  else
> +    gas_assert (endianness == BPF_BE16 || endianness == BPF_BE32 || endianness == BPF_BE64);
>  
>    bpf_insn = xasprintf ("%s %%%s,%s", be ? "endbe" : "endle",
>  			dst, be ? size[endianness - BPF_BE16] : size[endianness - BPF_LE16]);
> @@ -885,7 +887,7 @@ get_token (const char **insn, char *token, size_t *tlen)
>      } while (0)
>  
>    const char *str = *insn;
> -  char ch, ch2 = 0;
> +  int ch, ch2 = 0;
>    enum bpf_token_type ttype = BPF_UNKNOWN;
>    size_t len = 0;
>    const char *expr = NULL;
> @@ -1362,7 +1364,7 @@ bpf_pseudoc_to_normal_syntax (const char *str, char **errmsg)
>      } while (0)
>  
>    enum bpf_token_type ttype;
> -  enum bpf_token_type bpf_endianness,
> +  enum bpf_token_type bpf_endianness = BPF_UNKNOWN,
>  		      bpf_atomic_insn;
>    enum bpf_token_type bpf_jmp_op = BPF_JEQ; /* Arbitrary.  */
>    enum bpf_token_type bpf_cast = BPF_CAST_U8; /* Arbitrary.  */

Re: [PATCH v2] gas: fix building tc-bpf.c on s390x

[Jose E. Marchesi]

> On 03.05.2023 14:02, Ilya Leoshkevich wrote:
>> char is unsigned on s390x, so there are a lot of warnings like:
>> 
>>     gas/config/tc-bpf.c: In function 'get_token':
>>     gas/config/tc-bpf.c:900:14: error: comparison is always false due to limited range of data type [-Werror=type-limits]
>>       900 |       if (ch == EOF || len > MAX_TOKEN_SZ)
>>           |              ^~
>> 
>> Change its type to int, like in the other similar code.
>> 
>> There is also:
>> 
>>     gas/config/tc-bpf.c:735:30: error: 'bpf_endianness' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>       735 |    dst, be ? size[endianness - BPF_BE16] : size[endianness - BPF_LE16]);
>>           |                   ~~~~~~~~~~~^~~~~~~~~~
>> 
>> -Wmaybe-uninitialized doesn't seem to understand the FSM; just
>> initialize bpf_endianness to silence it.  Add an assertion to
>> build_bpf_endianness() in order to catch potential bugs.
>> ---
>> 
>> v1: https://sourceware.org/pipermail/binutils/2023-April/127235.html
>> v1 -> v2: Change signed char to int (Jan).
>>           Add an assertion.
>
> Looks okay to me now, but will want Jose's approval.

I'm OK with the patch.

Thanks Ilya for looking into this.  I'm currently in a whirpool of work,
but I plan to revisit both lexer and parser soon to simplify the
implementation.

> I'm a little puzzled, btw, why e.g. bpf_atomic_insn doesn't also expose
> a "maybe uninitialized" warning.
>
> Jan
>
>> --- a/gas/config/tc-bpf.c
>> +++ b/gas/config/tc-bpf.c
>> @@ -730,6 +730,8 @@ build_bpf_endianness (char *dst, enum bpf_token_type endianness)
>>        || endianness == BPF_LE32
>>        || endianness == BPF_LE64)
>>      be = 0;
>> +  else
>> +    gas_assert (endianness == BPF_BE16 || endianness == BPF_BE32 || endianness == BPF_BE64);
>>  
>>    bpf_insn = xasprintf ("%s %%%s,%s", be ? "endbe" : "endle",
>>  			dst, be ? size[endianness - BPF_BE16] : size[endianness - BPF_LE16]);
>> @@ -885,7 +887,7 @@ get_token (const char **insn, char *token, size_t *tlen)
>>      } while (0)
>>  
>>    const char *str = *insn;
>> -  char ch, ch2 = 0;
>> +  int ch, ch2 = 0;
>>    enum bpf_token_type ttype = BPF_UNKNOWN;
>>    size_t len = 0;
>>    const char *expr = NULL;
>> @@ -1362,7 +1364,7 @@ bpf_pseudoc_to_normal_syntax (const char *str, char **errmsg)
>>      } while (0)
>>  
>>    enum bpf_token_type ttype;
>> -  enum bpf_token_type bpf_endianness,
>> +  enum bpf_token_type bpf_endianness = BPF_UNKNOWN,
>>  		      bpf_atomic_insn;
>>    enum bpf_token_type bpf_jmp_op = BPF_JEQ; /* Arbitrary.  */
>>    enum bpf_token_type bpf_cast = BPF_CAST_U8; /* Arbitrary.  */