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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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,