r239549 - [CodeGen] Emit Constants for immediate inlineasm arguments.

[Ahmed Bougacha]

On Fri, Jun 12, 2015 at 12:46 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Fri, Jun 12, 2015 at 11:31 AM, Ahmed Bougacha <ahmed.bougacha at gmail.com>
> wrote:
>>
>> On Thu, Jun 11, 2015 at 2:17 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > On Thu, Jun 11, 2015 at 2:13 PM, Richard Smith <richard at metafoo.co.uk>
>> > wrote:
>> >>
>> >> On Thu, Jun 11, 2015 at 11:19 AM, Ahmed Bougacha
>> >> <ahmed.bougacha at gmail.com> wrote:
>> >>>
>> >>> Author: ab
>> >>> Date: Thu Jun 11 13:19:34 2015
>> >>> New Revision: 239549
>> >>>
>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=239549&view=rev
>> >>> Log:
>> >>> [CodeGen] Emit Constants for immediate inlineasm arguments.
>> >>>
>> >>> For inline assembly immediate constraints, we currently always use
>> >>> EmitScalarExpr, instead of directly emitting the constant. When the
>> >>> overflow sanitizer is enabled, this generates overflow intrinsics
>> >>> instead of constants.
>> >>>
>> >>> Instead, emit a constant for constraints that either require an
>> >>> immediate (e.g. 'I' on X86), or only accepts constants (immediate
>> >>> or symbolic; i.e., don't accept registers or memory).
>> >>>
>> >>> Fixes PR19763.
>> >>>
>> >>> Differential Revision: http://reviews.llvm.org/D10255
>> >>>
>> >>> Added:
>> >>>     cfe/trunk/test/CodeGen/inline-asm-immediate-ubsan.c
>> >>> Modified:
>> >>>     cfe/trunk/lib/CodeGen/CGStmt.cpp
>> >>>
>> >>> Modified: cfe/trunk/lib/CodeGen/CGStmt.cpp
>> >>> URL:
>> >>>
>> >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGStmt.cpp?rev=239549&r1=239548&r2=239549&view=diff
>> >>>
>> >>>
>> >>> ==============================================================================
>> >>> --- cfe/trunk/lib/CodeGen/CGStmt.cpp (original)
>> >>> +++ cfe/trunk/lib/CodeGen/CGStmt.cpp Thu Jun 11 13:19:34 2015
>> >>> @@ -1750,6 +1750,16 @@ llvm::Value* CodeGenFunction::EmitAsmInp
>> >>>                                           const
>> >>> TargetInfo::ConstraintInfo &Info,
>> >>>                                             const Expr *InputExpr,
>> >>>                                             std::string
>> >>> &ConstraintStr) {
>> >>> +  // If this can't be a register or memory, i.e., has to be a
>> >>> constant
>> >>> +  // (immediate or symbolic), try to emit it as such.
>> >>> +  if (!Info.allowsRegister() && !Info.allowsMemory()) {
>> >>> +    llvm::APSInt Result;
>> >>> +    if (InputExpr->isIntegerConstantExpr(Result, getContext()))
>> >>
>> >>
>> >> Please use EvaluateKnownConstInt here instead. This construct will
>> >> redundantly re-check whether the expression is an ICE first.
>> >
>> >
>> > Ah, sorry, you don't know that this is constant except in the
>> > requiresImmediateConstant() case. You should be calling EvaluateAsInt
>> > here
>> > instead.

Done in r239651.

>> Yes, I'm relying on the ICE-checking here, but I just realized Sema
>> also calls EvaluateAsInt, and because of that doesn't check that the
>> requiresImmediateConstant() operand is an ICE.
>>
>> Does the below patch make sense?  It lets us emit an error for the
>> overflowing "constant" expressions we want to catch here.
>
>
> Yes, that looks fine. Please also include a testcase (if you need an example
> of a non-ICE that we can evaluate, something like "(long)(void*)0" should
> work).

Perhaps isICE() isn't appropriate, as it's not true for C static const
(used in e.g. test/Sema/inline-asm-validate-x86.c).

Really, all I want here is not to silently ignore overflow;  here's
another hacky try:

diff --git a/cfe/trunk/lib/Sema/SemaStmtAsm.cpp
b/cfe/trunk/lib/Sema/SemaStmtAsm.cpp
--- a/cfe/trunk/lib/Sema/SemaStmtAsm.cpp
+++ b/cfe/trunk/lib/Sema/SemaStmtAsm.cpp
@@ -255,6 +255,7 @@
                          << InputExpr->getSourceRange());
     } else if (Info.requiresImmediateConstant() && !Info.allowsRegister()) {
       llvm::APSInt Result;
+      CheckForIntOverflow(InputExpr);
       if (!InputExpr->EvaluateAsInt(Result, Context))
         return StmtError(
             Diag(InputExpr->getLocStart(), diag::err_asm_immediate_expected)


I thought this would be checked for any expr in any context, but it's
only currently done for full-exprs (so IIUC doesn't apply for
inline-asm operands).  Is there a reason for that?  I can see it being
expensive, but it could be useful.  For instance, currently we'll warn
on:

  int a = (4 * (1 << 30));

but not on:

  foo(int a);
  foo((4 * (1 << 30)));

-Ahmed

>> With that, we can use EvaluateAsInt in CGStmt.cpp, and we'll do the
>> right thing for all combinations except overflowing "i".  I'm not
>> quite sure how to handle that, if it's even possible in the general
>> case: what happens if a link-time constant expression overflows?
>
>
> In C++11 onwards, it's not an ICE. In any case, the evaluator will produce
> the two's complement result.
>
>> Thanks for having a look!
>> -Ahmed
>>
>>
>> diff --git a/cfe/trunk/lib/Sema/SemaStmtAsm.cpp
>> b/cfe/trunk/lib/Sema/SemaStmtAsm.cpp
>> --- a/cfe/trunk/lib/Sema/SemaStmtAsm.cpp (revision 239545)
>> +++ b/cfe/trunk/lib/Sema/SemaStmtAsm.cpp (working copy)
>> @@ -255,7 +255,7 @@
>>                           << InputExpr->getSourceRange());
>>      } else if (Info.requiresImmediateConstant() &&
>> !Info.allowsRegister()) {
>>        llvm::APSInt Result;
>> -      if (!InputExpr->EvaluateAsInt(Result, Context))
>> +      if (!InputExpr->isIntegerConstantExpr(Result, Context))
>>          return StmtError(
>>              Diag(InputExpr->getLocStart(),
>> diag::err_asm_immediate_expected)
>>              << Info.getConstraintStr() << InputExpr->getSourceRange());
>
>