[Inline-asm] Add diagnosts for unsupported inline assembly arguments

[pengfei]

Summary

GCC supports power-of-2 size structures for the arguments. Clang supports fewer than GCC. But Clang always crashes for the unsupported cases.

This patch adds sema checks to do the diagnosts to solve these crashes.

[jyu2]

I don't find any document about GCC and ICC's behavior and the generated

code doesn't make much sense to me. Besides, if we change "=rm" to "=r"
or "=m", GCC and ICC will report error differently. Which gives me an
impression that the use of constraint is undefined behavior.

Here is gcc document: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#OutputOperands

To me it is defined behavior. Or I may missing some thing here.

constraint:
A string constant specifying constraints on the placement of the operand; See Constraints, for details.

Output constraints must begin with either ‘=’ (a variable overwriting an existing value) or ‘+’ (when reading and writing). When using ‘=’, do not assume the location contains the existing value on entry to the asm, except when the operand is tied to an input; see Input Operands.

After the prefix, there must be one or more additional constraints (see Constraints) that describe where the value resides. Common constraints include ‘r’ for register and ‘m’ for memory. When you list more than one possible location (for example, "=rm"), the compiler chooses the most efficient one based on the current context. If you list as many alternates as the asm statement allows, you permit the optimizers to produce the best possible code. If you must use a specific register, but your Machine Constraints do not provide sufficient control to select the specific register you want, local register variables may provide a solution (see Local Register Variables).

[pengfei]

@jyu2, Thanks for the detailed information.

Besides, if we change "=rm" to "=r"
or "=m", GCC and ICC will report error differently. Which gives me an
impression that the use of constraint is undefined behavior.
To me it is defined behavior. Or I may missing some thing here.

Sorry, I didn't describe it clearly. I meant it's ub when they are used together with structure type. E.g., https://godbolt.org/z/ssY5K4xEP
As you can see, when we use "=rm", GCC and ICC happen to work, that's because GCC only allows the structure to be passed on register with ICC only allows memory. I didn't find a document which describes the relationships between structure and its output constraint.
Besides, https://godbolt.org/z/5ex9x3dvv shows GCC and ICC have some rules when we passing a structure (Adding one more element in the exapmles will fail on both GCC and ICC). I didn't find the document either.

code doesn't make much sense to me.

This patch doesn't change any functionality of Clang. We just adding more sema checks to avoid the ugly crash. This is required from our user.
I believe we can do more things to march the behavior of GCC and ICC. I have added a FIXME in the code. Considering we don't have a clear document and such a case is in rare. And we don't have such requirment from user. I think this patch is till better than none.
Does it make sense?

[jyu2]

First I did not say "code doesn't make much sense to me". This is copy form your description.

Okay, thank for explanation. So if I understand right, both gcc and icc generated code is not making sense to you. So instead, you want clang give error for this when output size greater than input size.
A question:
What about input size greater than output size? It seems the error already emit for that case in Sema. Should also handle the same with your case in Sema?

clang/lib/Sema/SemaStmtAsm.cpp

671

Why not emit error in here?

[pengfei]

First I did not say "code doesn't make much sense to me". This is copy form your description.

Oop, my bad understanding. :)

both gcc and icc generated code is not making sense to you.

Yes. Unless there's document to specify the behavior clearly.

So instead, you want clang give error for this when output size greater than input size.

Giving error only when output size greater than input size and output type is not one of the 3 cases in CGStmt.cpp:2480. I.e.: OutputDomain != AD_Other || OutSize <= InSize. Otherwise, Clang will crash there.

What about input size greater than output size? It seems the error already emit for that case in Sema. Should also handle the same with your case in Sema?

Sema emits error when 1) InputDomain != AD_Other 2) Output is mentioned and OutSize < InSize. So input size is allow to be greater than output size only when output is not mentioned. I don't fully understand the code in CGStmt.cpp, but according to comment in line 2463, it seems only larger output result needs to be handled.

clang/lib/Sema/SemaStmtAsm.cpp

671

Actually, adding this condition will let it fall through and emit error at line 688.

[pengfei]

Implemented the handling for structure types.

[pengfei]

Address Jennifer comments from off-line.

[pengfei]

Rebase.

[pengfei]

Refactor.

[skan]

clang/test/Sema/asm.c

317–345

Better to add comments about the size of the struct.

360–361

Better to comment that we are checking the size is power of 2 here.

[pengfei]

Thanks @skan !

[pengfei]

Ping @jyu2

[jyu2]

clang/lib/Sema/SemaStmtAsm.cpp

677

Error message is not very clear to me. I think we should create more specified error message there. Like power of two, or size < 8 or > pointer size?

Using error message selector.

[pengfei]

Address review comment.

clang/lib/Sema/SemaStmtAsm.cpp

677

Use getIntTypeForBitwidth instead. So we don't need to check for each case.

[pengfei]

Remove outdated comment

[jyu2]

clang/lib/Sema/SemaStmtAsm.cpp

622

Are you sure you want to change the Input/output Domain? Since you changed this, could you add both codegen and sema check tests for struct type(you already has sema check for struct type, but I don't see any array type) and array type.

Thanks.
Jennifer

675

Do you need return NS after diagnostic?

[pengfei]

Address review comments.

clang/lib/Sema/SemaStmtAsm.cpp

622

The input / output domain are just for sema check here. I don't think it changes codegen behavior.

675

Good catch!

[pengfei]

Ping @jyu2

[jyu2]

clang/lib/Sema/SemaStmtAsm.cpp

622

Yes, for seam check, once error emit, compiler stop at CodeGen. However, if no error emit, compiler will go though the CodeGen.

Since pointer/struct and constantArray are allowed for small size of type, so would you please add code gen test for that?

In your test where no-error should emit part...

asm ("" : "=rm" (a): "0" (1)); // no-error

Thanks.
Jennifer

[pengfei]

Address review comment.

clang/lib/Sema/SemaStmtAsm.cpp

622

Thanks Jennifer! I see your point. I'll only add sema check (rather than loose in some way) in this patch.
The codegen part is complicated, some cases will pass but some still fail. I have left comments in the tests.

[jyu2]

This is okay with me.

[pengfei]

Thanks @jyu2 !

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_invalid_asm_cast_lvalue : Error<
     "remove the cast or build with -fheinous-gnu-extensions">;
   def err_invalid_asm_value_for_constraint
       : Error <"value '%0' out of range for constraint '%1'">;
   def err_asm_non_addr_value_in_memory_constraint : Error <
     "reference to a %select{bit-field|vector element|global register variable}0"
     " in asm %select{input|output}1 with a memory constraint '%2'">;
   def err_asm_input_duplicate_match : Error<
     "more than one input constraint matches the same output '%0'">;
+  def err_store_value_to_reg : Error<
+    "impossible constraint in asm: can't store value into a register">;
 
   def warn_asm_label_on_auto_decl : Warning<
     "ignored asm label '%0' on automatic variable">;
   def warn_invalid_asm_cast_lvalue : Warning<
     "invalid use of a cast in an inline asm context requiring an lvalue: "
     "accepted due to -fheinous-gnu-extensions, but clang may remove support "
     "for this in the future">;
   def warn_asm_mismatched_size_modifier : Warning<

clang/lib/CodeGen/CGStmt.cpp

if ((i < ResultTypeRequiresCast.size()) && ResultTypeRequiresCast[i]) {
       if (getTargetHooks().isScalarizableAsmOperand(*this, TruncTy)) {
         Builder.CreateStore(Tmp, A);
         continue;
       }
 
       QualType Ty = getContext().getIntTypeForBitwidth(Size, /*Signed*/ false);
       if (Ty.isNull()) {
         const Expr *OutExpr = S.getOutputExpr(i);
-        CGM.Error(
-            OutExpr->getExprLoc(),
-            "impossible constraint in asm: can't store value into a register");
+        CGM.getDiags().Report(OutExpr->getExprLoc(),
+                              diag::err_store_value_to_reg);
         return;
       }
       Dest = MakeAddrLValue(A, Ty);
     }
     EmitStoreThroughLValue(RValue::get(Tmp), Dest);
   }
 }
 

clang/lib/Sema/SemaStmtAsm.cpp

for (unsigned i = 0, e = InputConstraintInfos.size(); i != e; ++i) {
 
     // Decide if the input and output are in the same domain (integer/ptr or
     // floating point.
     enum AsmDomain {
       AD_Int, AD_FP, AD_Other
     } InputDomain, OutputDomain;
 
     if (InTy->isIntegerType() || InTy->isPointerType())
       InputDomain = AD_Int;
     else if (InTy->isRealFloatingType())
       InputDomain = AD_FP;
     else
       InputDomain = AD_Other;
 
     if (OutTy->isIntegerType() || OutTy->isPointerType())
       OutputDomain = AD_Int;
     else if (OutTy->isRealFloatingType())
if (isOperandMentioned(TiedTo, Pieces)) {
       // value, then it's ok because we'll promote the input to the larger type.
       SmallerValueMentioned |= OutSize < InSize;
     }
 
     // If the smaller value wasn't mentioned in the asm string, and if the
     // output was a register, just extend the shorter one to the size of the
     // larger one.
     if (!SmallerValueMentioned && InputDomain != AD_Other &&
-        OutputConstraintInfos[TiedTo].allowsRegister())
+        OutputConstraintInfos[TiedTo].allowsRegister()) {
+      // FIXME: GCC supports the OutSize to be 128 at maximum. Currently codegen
+      // crash when the size larger than the register size. So we limit it here.
+      if (OutTy->isStructureType() &&
+          Context.getIntTypeForBitwidth(OutSize, /*Signed*/ false).isNull()) {
+        targetDiag(OutputExpr->getExprLoc(), diag::err_store_value_to_reg);
+        return NS;
+      }
+
       continue;
+    }
 
     // Either both of the operands were mentioned or the smaller one was
     // mentioned.  One more special case that we'll allow: if the tied input is
     // integer, unmentioned, and is a constant, then we'll allow truncating it
     // down to the size of the destination.
     if (InputDomain == AD_Int && OutputDomain == AD_Int &&
         !isOperandMentioned(InputOpNo, Pieces) &&
         InputExpr->isEvaluatable(Context)) {

clang/test/Sema/asm.c

lab2:;
   asm ("" : [lab] "=r" (x),[lab] "+r" (x) : [lab1] "r" (x));
   // expected-error@+2 {{duplicate use of asm operand name "lab"}}
   // expected-note@+1 {{asm operand name "lab" first referenced here}}
   asm ("" : [lab] "=r" (x1) : [lab] "r" (x));
   // expected-error@+1 {{invalid operand number in inline asm string}}
   asm ("jne %l0":::);
   asm goto ("jne %l0"::::lab);
 }
+
+typedef struct _st_size64 {
+  int a;
+  char b;
+} st_size64;
+
+typedef struct _st_size96 {
+  int a;
+  int b;
+  int c;
+} st_size96;
+
+typedef struct _st_size16 {
+  char a;
+  char b;
+} st_size16;
+
+typedef struct _st_size32 {
+  char a;
+  char b;
+  char c;
+  char d;
+} st_size32;
+
+typedef struct _st_size128 {
+  int a;
+  int b;
+  int c;
+  int d;
+} st_size128;
+
+void test19(long long x)
+{
+  st_size64 a;
+  st_size96 b;
+  st_size16 c;
+  st_size32 d;
+  st_size128 e;
+  asm ("" : "=rm" (a): "0" (1)); // no-error
+  asm ("" : "=rm" (d): "0" (1)); // no-error
+  asm ("" : "=rm" (c): "0" (x)); // no-error
+  // FIXME: This case is actually supported by codegen.
+  asm ("" : "=rm" (x): "0" (a)); // expected-error {{unsupported inline asm: input with type 'st_size64' (aka 'struct _st_size64') matching output with type 'long long'}}
+  // FIXME: This case is actually supported by codegen.
+  asm ("" : "=rm" (a): "0" (d)); // expected-error {{unsupported inline asm: input with type 'st_size32' (aka 'struct _st_size32') matching output with type 'st_size64' (aka 'struct _st_size64')}}
+  asm ("" : "=rm" (b): "0" (1)); // expected-error {{impossible constraint in asm: can't store value into a register}}
+  // FIXME: This case should be supported by codegen, but it fails now.
+  asm ("" : "=rm" (e): "0" (1)); // no-error
+  // FIXME: This case should be supported by codegen, but it fails now.
+  asm ("" : "=rm" (x): "0" (e)); // expected-error {{unsupported inline asm: input with type 'st_size128' (aka 'struct _st_size128') matching output with type 'long long'}}
+}