[llvm] r207747 - Correction to assert statemtent to allow 32-bit unsigned numbers with the top bit set.

[Saleem Abdulrasool]

On Thursday, May 1, 2014, Richard Barton <richard.barton at arm.com> wrote:

> Hi James
>
>
>
> My assumption is that the underlying type of Value is defined by the
> MCExpr class which presumably needs to be able to handle multiple value
> sizes and signednesses, so int64 was chosen as it can accommodate all
> values.
>
>
>
> I was taking the assert statement on face value “expression value must be
> representable in 32 bits” which means signed or unsigned to me. There are
> obviously loads of ways that you can code the check.
>
>
>
> Hopefully Saleem can update us on whether the value should indeed be
> unsigned.
>
Correct.  The underlying type is int64_t as we are evaluating a machine
expression.  Treating the value as unsigned here is correct.  The
constraint is that we must be able to represent the immediate as part of
the instruction.  The assertion is there since the underlying type is
larger and would simply pass through unnoticed and that worried me.

> Ta
>
> Rich
>
>
>
>
>
> *From:* mankeyrabbit at gmail.com<javascript:_e(%7B%7D,'cvml','mankeyrabbit at gmail.com');>[mailto:
> mankeyrabbit at gmail.com<javascript:_e(%7B%7D,'cvml','mankeyrabbit at gmail.com');>]
> *On Behalf Of *James Molloy
> *Sent:* 01 May 2014 13:01
> *To:* Richard Barton
> *Cc:* LLVM Commits
> *Subject:* Re: [llvm] r207747 - Correction to assert statemtent to allow
> 32-bit unsigned numbers with the top bit set.
>
>
>
> Hi Rich,
>
>
>
> The assert statement now looks rather odd. From the testcase it looks like
> Value needs to be unsigned, something like:
>
>
>
> +   assert((uint64_t)Value <= UINT32_MAX);
>
>
>
> Or does that code also need to handle signed values for other cases?
>
>
>
> Cheers,
>
>
>
> James
>
>
>
> On 1 May 2014 12:37, Richard Barton <richard.barton at arm.com<javascript:_e(%7B%7D,'cvml','richard.barton at arm.com');>>
> wrote:
>
> Author: rbarton
> Date: Thu May  1 06:37:44 2014
> New Revision: 207747
>
> URL: http://llvm.org/viewvc/llvm-project?rev=207747&view=rev
> Log:
> Correction to assert statemtent to allow 32-bit unsigned numbers with the
> top bit set.
>
> This fixes an ARM assembler crash - regression test added.
>
>
> Modified:
>     llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
>     llvm/trunk/test/MC/ARM/thumb2-diagnostics.s
>
> Modified: llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp?rev=207747&r1=207746&r2=207747&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp Thu May  1
> 06:37:44 2014
> @@ -9487,8 +9487,8 @@ unsigned ARMAsmParser::validateTargetOpe
>        int64_t Value;
>        if (!SOExpr->EvaluateAsAbsolute(Value))
>          return Match_Success;
> -      assert((Value >= INT32_MIN && Value <= INT32_MAX) &&
> -             "expression value must be representiable in 32 bits");
> +      assert((Value >= INT32_MIN && Value <= UINT32_MAX) &&
> +             "expression value must be representable in 32 bits");
>      }
>      break;
>    case MCK_GPRPair:
>
> Modified: llvm/trunk/test/MC/ARM/thumb2-diagnostics.s
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/ARM/thumb2-diagnostics.s?rev=207747&r1=207746&r2=207747&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/MC/ARM/thumb2-diagnostics.s (original)
> +++ llvm/trunk/test/MC/ARM/thumb2-diagnostics.s Thu May  1 06:37:44 2014
> @@ -81,3 +81,10 @@ foo2:
>  @ CHECK-ERRORS:                  ^
>  @ CHECK-ERRORS: error: immediate expression for mov requires :lower16: or
> :upper16
>  @ CHECK-ERRORS:                  ^
> +
> +        and sp, r1, #80008000
> +        and pc, r1, #80008000
> +@ CHECK-ERRORS: error: invalid operand for instruction
> +@ CHECK-ERRORS: error: invalid operand for instruction
> +
> +
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu<javascript:_e(%7B%7D,'cvml','llvm-commits at cs.uiuc.edu');>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>


-- 
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140501/6b7649dd/attachment.html>