This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jasonliuMTK on Aug 11 2021, 6:26 PM.

Details

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

Diff Detail

Event Timeline

jasonliuMTK requested review of this revision.Aug 11 2021, 6:26 PM
jasonliuMTK created this revision.

Fix CI build.

void added a comment.Aug 19 2021, 9:20 AM

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

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 planned changes to this revision.Aug 20 2021, 6:38 AM

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

Try another approach.

@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?

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.

Address comments.

Thanks, that looks a lot better.

clang/lib/Sema/SemaStmtAsm.cpp
401

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

@rjmccall
Thanks John. Hopefully that addressed the concern.

jasonliuMTK marked an inline comment as done.Aug 26 2021, 6:54 AM
rjmccall accepted this revision.Aug 26 2021, 11:00 AM

It does, thanks. LGTM

This revision is now accepted and ready to land.Aug 26 2021, 11:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2021, 10:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript