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

Michael Zimmermann posted 1 patch 5 years, 9 months ago
Failed in applying to current master (apply log)
ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[edk2] [PATCH] ArmPkg/ArmDisassemblerLib: fix check for MSR instruction
Posted by Michael Zimmermann 5 years, 9 months ago
From: M1cha <sigmaepsilon92@gmail.com>

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>
-- 
2.17.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/ArmDisassemblerLib: fix check for MSR instruction
Posted by Michael Zimmermann 5 years, 9 months ago
CC the arm maintainers
On Thu, Jun 7, 2018 at 7:07 AM Michael Zimmermann
<sigmaepsilon92@gmail.com> wrote:
>
> From: M1cha <sigmaepsilon92@gmail.com>
>
> 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>
> --
> 2.17.1
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/ArmDisassemblerLib: fix check for MSR instruction
Posted by Ard Biesheuvel 5 years, 9 months ago
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,
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/ArmDisassemblerLib: fix check for MSR instruction
Posted by Michael Zimmermann 5 years, 9 months ago
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,
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel