Fix assertion when passing function into inline asm's input operand

[jasonliuMTK]

Summary

This seem to be a regression caused by this change: https://reviews.llvm.org/D60943.
Since we delayed report the error, we would run into some invalid state in clang and llvm.

Without this fix, clang would assert when passing function into inline asm's input operand.
If building without assertion, we would see segfault in llvm: https://godbolt.org/z/hb4acs8aG

This is essentially a partial reverse of this commit: https://github.com/llvm/llvm-project/commit/d68b2d043865e1c106432f2ab9c1b99a5a2ba86e

[jasonliuMTK]

Fix CI build.

[jasonliuMTK]

Gentle ping.

[void]

Wouldn't it be better to check if it's an argument to an inline asm block instead?

[rjmccall]

This is a reasonable assertion; the bug should be fixed on the caller side. Probably Sema should be performing function-to-pointer decay on r-value asm operands.

[jasonliuMTK]

Thanks for the comments. I will try to seek a solution on the Sema side.

[jasonliuMTK]

Try another approach.

[jasonliuMTK]

@void @rjmccall
I took a look at the Sema, and thought that inlining won't turn a function into a constant. So it might be okay to detect this error case earlier, instead of decaying to pointer and propagate the error to a later stage.
What do you think?

[rjmccall]

I think the right approach here is that, in the non l-value case (i.e. having not passed if (Info.allowsMemory() && !Info.allowsRegister())), you should be doing DefaultFunctionArrayLvalueConversion regardless of whether the operand requires a constant, and then you can do the check for a constant as a follow-up to that. As it is, the bug is that you're bypassing DefaultFunctionArrrayLvalueConversion when the operand requires a constant, and that's just wrong.

[jasonliuMTK]

Address comments.

[rjmccall]

Thanks, that looks a lot better.

clang/lib/Sema/SemaStmtAsm.cpp

401 ↗

(On Diff #368779)

The constant evaluator is fairly permissive about r-values, but still, you should probably update InputExpr here, too.

[jasonliuMTK]

@rjmccall
Thanks John. Hopefully that addressed the concern.

[rjmccall]

It does, thanks. LGTM

clang/lib/CodeGen/CGExpr.cpp

if (LV.getQuals().getObjCLifetime() == Qualifiers::OCL_Weak) {
 
     // In ARC mode, we load retained and then consume the value.
     llvm::Value *Object = EmitARCLoadWeakRetained(LV.getAddress(*this));
     Object = EmitObjCConsumeObject(LV.getType(), Object);
     return RValue::get(Object);
   }
 
   if (LV.isSimple()) {
-    assert(!LV.getType()->isFunctionType());
+    if (LV.getType()->isFunctionType())
+      return RValue::get(LV.getPointer(*this));
 
     if (LV.getType()->isConstantMatrixType())
       return EmitLoadOfMatrixLValue(LV, Loc, *this);
 
     // Everything needs a load.
     return RValue::get(EmitLoadOfScalar(LV, Loc));
   }
 

clang/test/CodeGen/asm-call-func-lvalue.c

+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-unknown-linux-gnu| FileCheck %s
+
+void callee(void);
+void caller() {
+  //CHECK: call void asm sideeffect "rcall $0", "n,~{dirflag},~{fpsr},~{flags}"(void ()* @callee)
+  asm("rcall %0"::"n"(callee));
+}