Re: [PATCH] ArmPkg/ArmDisassemblerLib: fix check for MSR instruction

[Michael Zimmermann <sigmaepsilon92@gmail.com>]

yes I'll do that next time. Thanks for the hint.

Thanks
Michael
On Thu, Jun 7, 2018 at 9:10 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On 7 June 2018 at 07:08, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
> > CC the arm maintainers
> > On Thu, Jun 7, 2018 at 7:07 AM Michael Zimmermann
> > <sigmaepsilon92@gmail.com> wrote:
> >>
> >> From: M1cha <sigmaepsilon92@gmail.com>
> >>
>
> Could you please use the same 'from' name+address as in the signoff?
> That saves me the hassle of fixing it up manually.
>
> >> GCC8 reported it with the following warning:
> >> ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c: In function 'DisassembleArmInstruction':
> >> ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c:397:30: error: bitwise
> >> comparison always evaluates to false [-Werror=tautological-compare]
> >> if ((OpCode  & 0x0db00000) == 0x03200000) {
> >>
> >> This condition trys to be true for both the immediate and the register
> >> version of the MSR instruction. They get identified inside the if-block
> >> using the variable I, which contains the value of bit 25.
> >>
> >> The problem with the comparison reported by GCC is that the
> >> bitmask excludes bit 25, while the value requires it to be set to one:
> >> 0x0db00000: 0000 11011 0 11 00 00 0000 000000000000
> >> 0x03200000: 0000 00110 0 10 00 00 0000 000000000000
> >>                    ^
> >> So the solution is to just don't require that bit to be set, because
> >> it gets checked later using 'I', which results in the following value:
> >> 0x01200000: 0000 00010 0 10 00 00 0000 000000000000
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
> >> ---
> >>  ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> >> index 29d9414a78b3..b449a5d3cd83 100644
> >> --- a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> >> +++ b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> >> @@ -394,7 +394,7 @@ DisassembleArmInstruction (
> >>    }
> >>
> >>
> >> -  if ((OpCode  & 0x0db00000) == 0x03200000) {
> >> +  if ((OpCode  & 0x0db00000) == 0x01200000) {
> >>      // A4.1.38 MSR{<cond>} CPSR_<fields>, #<immediate> MSR{<cond>} CPSR_<fields>, <Rm>
> >>      if (I) {
> >>        // MSR{<cond>} CPSR_<fields>, #<immediate>
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Pushed as b20085454e91bb1ded87009722c9994b4684472c
>
> Thanks,