Re: [Qemu-devel] [RFC PATCH 05/11] decodetree: Force Python to print unsigned values

[Eduardo Habkost <ehabkost@redhat.com>]

On Mon, Nov 12, 2018 at 07:52:28PM +0100, Philippe Mathieu-Daudé wrote:
> On Mon, Nov 12, 2018 at 6:03 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Mon, Nov 12, 2018 at 12:36:16AM +0100, Philippe Mathieu-Daudé wrote:
> > > Python internal representation is signed, so unsigned values
> > > bigger than 31-bit are interpreted as signed (and printed with
> > > a '-' signed).
> > > Mask out to force unsigned values.
> >
> > I don't understand this commit description. Python surely
> > supports integers larger than 2^31, and its internal
> > representation shouldn't matter at all:
> >
> >   >>> '0x{0:08x}'.format(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
> >   '0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'
> >
> > Can you explain how the code ends up with a negative value in the
> > `self.thismask` or `b` variables?  If `self.subs` contains
> > negative values, this is likely to break other parts of the code.
> 
> I guess I misunderstood the error, thus the description is invalid.
> 
> >
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > ---
> > > TODO: display error encountered:
> > >
> > >    case 0x-1:
> > >        ....
> >
> > How can I reproduce it?
> 
> $ scripts/decodetree.py /dev/null
> 
>     switch (insn & 0x-0000001) {
>     }
> 
> main() ->
>   build_tree(patterns, 0, outermask = 0) ->
>        innermask = ~outermask # = -1
>       Tree(fullmask, innermask = -1) ->
>         __init__(self, fm, tm = -1) ->
>           self.thismask = tm # -1
>   t.output_code(4, False, 0, 0) ->
>     sh = is_contiguous(self.thismask) # -1
>         output(ind, 'switch (', str_switch(self.thismask), ') {\n')
> 
>         with:
> 
>             def str_switch(b):
>                 return 'insn & 0x{0:08x}'.format(b)
> 
> So the fix is rather:
> 
> -- >8 --
> diff --git a/scripts/decodetree.py b/scripts/decodetree.py
> @@ -916,7 +916,7 @@ class Tree:
> 
> def build_tree(pats, outerbits, outermask):
>      # Find the intersection of all remaining fixedmask.
> -    innermask = ~outermask
> +    innermask = ~outermask & insnmask
>      for i in pats:
>          innermask &= i.fixedmask

Nice.  Thanks for the explanation.

This really seems to be the only place in the code where the bit
operations may generate a negative number.  All remaining usage
of the ~ operator in the code is already inside & expressions
that will ensure the result is positive.

To the alternative fix above:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo