diff mbox series

gas: fix building tc-bpf.c on s390x

Message ID 20230427125607.362035-1-iii@linux.ibm.com
State Unresolved
Headers
Series gas: fix building tc-bpf.c on s390x |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Ilya Leoshkevich April 27, 2023, 12:56 p.m. UTC 
  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)
          |              ^~

Make ch explicitly signed.

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.
---
 gas/config/tc-bpf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich April 28, 2023, 6:42 a.m. UTC | #1 
On 27.04.2023 14:56, Ilya Leoshkevich via Binutils wrote:
> --- a/gas/config/tc-bpf.c
> +++ b/gas/config/tc-bpf.c
> @@ -885,7 +885,7 @@ get_token (const char **insn, char *token, size_t *tlen)
>      } while (0)
>  
>    const char *str = *insn;
> -  char ch, ch2 = 0;
> +  signed char ch, ch2 = 0;

But this doesn't make things any better. If you grep for uses of EOF in
gas, you'll find that the corresponding variables typically are of type
int, and that's what I expect you want to use here as well.

> @@ -1362,7 +1362,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,

The variable surely wants an initializer, but I'm uncertain whether the
one you picked is suitable. I don't know bpf, but I see only two options:
There is a default endianness, in which case that wants to be the
initializer. Or endianness needs to be specified explicitly before any
of the constructs leading to build_bpf_endianness() may be used. In that
case the initializer chosen is perhaps fine, but the variable then still
having that value would need to be diagnosed. With what you've done we
now end up with an out of bounds array access in build_bpf_endianness().

You would better have Cc-ed the arch maintainer anyway; doing so now.

Jan
Ilya Leoshkevich April 28, 2023, 9:37 a.m. UTC | #2 
On Fri, 2023-04-28 at 08:42 +0200, Jan Beulich wrote:
> On 27.04.2023 14:56, Ilya Leoshkevich via Binutils wrote:
> > --- a/gas/config/tc-bpf.c
> > +++ b/gas/config/tc-bpf.c
> > @@ -885,7 +885,7 @@ get_token (const char **insn, char *token,
> > size_t *tlen)
> >      } while (0)
> >  
> >    const char *str = *insn;
> > -  char ch, ch2 = 0;
> > +  signed char ch, ch2 = 0;
> 
> But this doesn't make things any better. If you grep for uses of EOF
> in
> gas, you'll find that the corresponding variables typically are of
> type
> int, and that's what I expect you want to use here as well.

You are right, int is better. Thanks.

> > @@ -1362,7 +1362,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,
> 
> The variable surely wants an initializer, but I'm uncertain whether
> the
> one you picked is suitable. I don't know bpf, but I see only two
> options:
> There is a default endianness, in which case that wants to be the
> initializer. Or endianness needs to be specified explicitly before
> any
> of the constructs leading to build_bpf_endianness() may be used. In
> that
> case the initializer chosen is perhaps fine, but the variable then
> still
> having that value would need to be diagnosed. With what you've done
> we
> now end up with an out of bounds array access in
> build_bpf_endianness().
> 
> You would better have Cc-ed the arch maintainer anyway; doing so now.

In that regard the patch does not make things worse.
If we end up not initializing the variable on the intended path, with
today's code we would still have an OOB access (but with a random
offset). The goal here is only to silence the warning, which I believe
is emitted incorrectly.

If I read the FSM correctly, this can only happen due to a bug, no
user input (valid or invalid) should be leading to this. So, just to be
on the safe side, I would add gas_assert() to build_bpf_endianness().

> Jan
diff mbox series

Patch

diff --git a/gas/config/tc-bpf.c b/gas/config/tc-bpf.c
index 171fc682806..adcf5edc645 100644
--- a/gas/config/tc-bpf.c
+++ b/gas/config/tc-bpf.c
@@ -885,7 +885,7 @@  get_token (const char **insn, char *token, size_t *tlen)
     } while (0)
 
   const char *str = *insn;
-  char ch, ch2 = 0;
+  signed char ch, ch2 = 0;
   enum bpf_token_type ttype = BPF_UNKNOWN;
   size_t len = 0;
   const char *expr = NULL;
@@ -1362,7 +1362,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.  */