[CodeGen] Emit Constants, not ScalarExprs, for "immediate"-constrained inlineasm arguments.

[ab]

Summary

Same problem as D10045 / PR23517, slightly different solution.

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, I think).

(re-send on cfe-commits of D10254)

[ahatanak]

This fixes https://llvm.org/bugs/show_bug.cgi?id=19763.

[ahatanak]

This patch looks fine to me.

Do we have to handle an expression like (long)&object - (long)&base when it's used with constraint "i"? I believe clang currently cannot handle this expression.

[dsanders]

I'm wondering if the assert ought to be a normal error message and I think we may need a REQUIRES for buildbots lacking sanitizer support but apart from that this patch LGTM.

Just to mention it, there's something odd about the 'i' constraint in the backends but I haven't had chance to investigate it yet. For some reason, it has to be handled in *DAGToDAGISel::SelectInlineAsmMemoryOperand() despite not being a memory operand.

lib/CodeGen/CGStmt.cpp

1759–1760 ↗

(On Diff #27147)

Should this cause an error to be emitted on release builds too?

test/CodeGen/inline-asm-immediate-ubsan.c

2 ↗

(On Diff #27147)

Sanitizers aren't always available. It's possible we need a 'REQUIRES:' line to disable the test for some buildbots.

[ab]

In D10255#184520, @ahatanak wrote:

Do we have to handle an expression like (long)&object - (long)&base when it's used with constraint "i"? I believe clang currently cannot handle this expression.

Yes, I don't think 'i' supports that.

In D10255#184893, @dsanders wrote:

Just to mention it, there's something odd about the 'i' constraint in the backends but I haven't had chance to investigate it yet. For some reason, it has to be handled in *DAGToDAGISel::SelectInlineAsmMemoryOperand() despite not being a memory operand.

I'm not too familiar with inline-asm handling, but could it be because it supports symbols, and symbols aren't really immediates (e.g. aren't usually supported in non-addressing mode contexts) ?

lib/CodeGen/CGStmt.cpp

1759–1760 ↗

(On Diff #27147)

I'm not sure, my understanding of clang is, by the time we get to IR gen, source errors have already been caught and reported.

test/CodeGen/inline-asm-immediate-ubsan.c

2 ↗

(On Diff #27147)

That makes sense for runtime tests, but is it necessary for codegen tests? I thought all of IR gen was always available, much like targets.

[dsanders]

LGTM

In D10255#184893, @dsanders wrote:
Just to mention it, there's something odd about the 'i' constraint in the backends but I haven't had chance to investigate it yet. For some reason, it has to be handled in
*DAGToDAGISel::SelectInlineAsmMemoryOperand() despite not being a memory operand.

I'm not too familiar with inline-asm handling, but could it be because it supports symbols, and symbols aren't really immediates (e.g. aren't usually supported in non-addressing mode
contexts) ?

Possibly, but it could also be unintentional and just happened to turn out ok in the end. I noticed it while removing a hardcoded 'm' that caused all memory constraints to behave like 'm'.

lib/CodeGen/CGStmt.cpp

1759–1760 ↗

(On Diff #27147)

I've just tried it on X86 with 'I' and it does emit an error so something else must be handling it.

test/CodeGen/inline-asm-immediate-ubsan.c

2 ↗

(On Diff #27147)

I seem to remember it reporting errors early for some reason. I've just tried it and codegen is working without working sanitizers being available.

cfe/trunk/lib/CodeGen/CGStmt.cpp

CodeGenFunction::EmitAsmInputLValue(const TargetInfo::ConstraintInfo &Info,
 
   return Arg;
 }
 
 llvm::Value* CodeGenFunction::EmitAsmInput(
                                          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()))
+      return llvm::ConstantInt::get(getLLVMContext(), Result);
+    assert(!Info.requiresImmediateConstant() &&
+           "Required-immediate inlineasm arg isn't constant?");
+  }
+
   if (Info.allowsRegister() || !Info.allowsMemory())
     if (CodeGenFunction::hasScalarEvaluationKind(InputExpr->getType()))
       return EmitScalarExpr(InputExpr);
 
   InputExpr = InputExpr->IgnoreParenNoopCasts(getContext());
   LValue Dest = EmitLValue(InputExpr);
   return EmitAsmInputLValue(Info, Dest, InputExpr->getType(), ConstraintStr,
                             InputExpr->getExprLoc());

cfe/trunk/test/CodeGen/inline-asm-immediate-ubsan.c

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN:     -fsanitize=signed-integer-overflow \
+// RUN:   | FileCheck %s --check-prefix=CHECK
+
+// Verify we emit constants for "immediate" inline assembly arguments.
+// Emitting a scalar expression can make the immediate be generated as
+// overflow intrinsics, if the overflow sanitizer is enabled.
+
+// Check both 'i' and 'I':
+// - 'i' accepts symbolic constants.
+// - 'I' doesn't, and is really an immediate-required constraint.
+
+// See also PR23517.
+
+// CHECK-LABEL: @test_inlineasm_i
+// CHECK: call void asm sideeffect "int $0", "i{{.*}}"(i32 2)
+void test_inlineasm_i() {
+  __asm__ __volatile__("int %0" :: "i"(1 + 1));
+}
+
+// CHECK-LABEL: @test_inlineasm_I
+// CHECK: call void asm sideeffect "int $0", "I{{.*}}"(i32 2)
+void test_inlineasm_I() {
+  __asm__ __volatile__("int %0" :: "I"(1 + 1));
+}