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

[Ahmed Bougacha]

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.

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.

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?

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());