Additional fix for PR14269: Clang crashes when a bit field is used as inline assembler input / output with memory constraint

[andreybokhanko]

Summary

In his post-review for my fix (http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150608/130464.html) Richard Smith wrote:

What about other weird kinds of lvalues, like the result of __real / __imag, vector indexing, and global register variables? Those have the same problem; CGStmt.cpp blindly calls LValue::getAddress without checking for those cases.

This patch fixes these cases:

like the result of __real / __imag

This already works well -- no errors printed for real (as they can be handled just fine), a error ("invalid lvalue in asm output") is printed for imag.

I can't create a test case that leads either to compile time fail or incorrect code generation.

vector indexing

Fixed

and global register variables

Fixed

[andreybokhanko]

Ping

[andreybokhanko]

Pinging Richard!

[andreybokhanko]

Ping!

[echristo]

This seems to be a bit overconstrained versus what gcc accepts on the testcase:

dzur:~/tmp> gcc -S baz.c |
baz.c: In function ‘test16’: |
baz.c:18:4: error: cannot take address of bit-field ‘field3’ |

: "m" (a.field3)); // expected-error {{reference to a non-addressable value in asm input with a me\|

m |

^                                                                                                  |

baz.c:28:4: error: address of global register variable ‘test16_baz’ requested |

: "m" (test16_baz)); // expected-error {{reference to a non-addressable value in asm input with a \|

m |

^                                                                                                  |

baz.c:16:3: error: memory input 0 is not directly addressable |

__asm__("movl $5, %0"                                                                               |
^                                                                                                   |

baz.c:26:3: error: memory input 0 is not directly addressable |

__asm__("movl $5, %0"                                                                               |
^                                                                                                   |

also, the precision in error messages is nice. :)

-eric

[andreybokhanko]

Error message became more specific (as requested in Eric's comment); check for memory constraint compatibility put into a separate helper function (checkExprMemoryConstraintCompat).

[andreybokhanko]

Hi Eric,

Thanks for looking into the patch!

In D10476#200564, @echristo wrote:

This seems to be a bit overconstrained versus what gcc accepts on the testcase:

There are three cases that GCC accepts and clang (with my fix) doesn't:

1. Output to a bit-field:

typedef struct test16_foo {
  unsigned int field1 : 1;
  unsigned int field2 : 2;
  unsigned int field3 : 3;
} test16_foo;

void test16()
{
  test16_foo a;

  __asm__("movl $5, %0"
          : "=rm" (a.field2)); // expected-error {{reference to a non-addressable value in asm output with a memory constraint '=rm'}}
}

GCC accepts this, but generates incorrect code, that is not even accepted by assembler:

$ gcc test1.c
test1.c: Assembler messages:
test1.c:11: Error: `%al' not allowed with `movl'

2. Vector elements:

typedef __attribute__((vector_size(16))) int test16_bar;

int main()
{
  test16_bar b = {1, 2, 3, 4};

  __asm__("movl $5, %0"
          : "=rm" (b[2]));

  return b[2];
}

The problem here is that LLVM IR represents vectors with a specific vector type; you can't get address of a random element inside vector. Specific instructions should be used to get individual vector elements ("extractelement" and "insertelement"), but then again -- they don't provide addresses of elements. GCC simply treats a vector as an array of elements and computes desired address. In theory, this can be done in LLVM IR as well, but I don't think this is the right approach -- we generally can't make any assumptions on how vectors are represented by a target CPU.

Do you agree?

3. Global register variables:

register int test16_baz asm("rbx");

void test16()
{
  __asm__("movl $5, %0"
          : "=rm" (test16_baz)); // expected-error {{reference to a non-addressable value in asm output with a memory constraint '=rm'}}
}

The constraint here says "register *or* memory". GCC chooses register and compiles the test fine. Clang always chooses memory -- due to the following check at CGStmt.cpp:1874:

// If this is a register output, then make the inline asm return it
// by-value.  If this is a memory result, return the value by-reference.
if (!Info.allowsMemory() && hasScalarEvaluationKind(OutExpr->getType())) {

I have no idea why it checks for "!info.allowsMemory()" and not for "info.allowsRegister", but this is a separate issue, not related to my patch.

When a test is re-written in a way that allows *only* memory, GCC complains as well:

register int test16_baz asm("rbx");

void test16()
{
  __asm__("movl $5, %0"
          : "=m" (test16_baz)); // expected-error {{reference to a non-addressable value in asm output with a memory constraint '=rm'}}
}

$ gcc test3.c
test3.c: In function ?test16?:
test3.c:6:11: error: address of global register variable ?test16_baz? requested
           : "=m" (test16_baz)); // expected-error {{reference to a non-addressable value in asm output with a memory constraint '=rm'}}
           ^
test3.c:5:3: error: invalid lvalue in asm output 0
   __asm__("movl $5, %0"
   ^

In D10476#200564, @echristo wrote:

also, the precision in error messages is nice. :)

Done!

Please re-review.

Andrey

[hfinkel]

The problem here is that LLVM IR represents vectors with a specific vector type; you can't get address of a random element inside vector. Specific instructions should be used to get individual vector elements ("extractelement" and "insertelement"), but then again -- they don't provide addresses of elements. GCC simply treats a vector as an array of elements and computes desired address. In theory, this can be done in LLVM IR as well, but I don't think this is the right approach -- we generally can't make any assumptions on how vectors are represented by a target CPU.

Do you agree?

I agree; we don't want to tie frontend functionality to a specific representation of the vectors.

We could support this, but we'd need to do it by:

1. Creating a local stack variable (alloca)
2. Extracting the requested vector element and storing it in that stack-allocated memory
3. Providing the address of the local stack variable to the inline asm
4. After the inline asm, loading the value from the local stack variable and inserting it back into the vector

I have no opinion on whether or not this is worth implementing.

[andreybokhanko]

In D10476#202740, @hfinkel wrote:

I agree; we don't want to tie frontend functionality to a specific representation of the vectors.

We could support this, but we'd need to do it by:

1. Creating a local stack variable (alloca)
2. Extracting the requested vector element and storing it in that stack-allocated memory
3. Providing the address of the local stack variable to the inline asm
4. After the inline asm, loading the value from the local stack variable and inserting it back into the vector

I have no opinion on whether or not this is worth implementing.

Hal, what you suggested means basically creating a new local variable, copying value of a vector element to it and then providing address of this local variable, not original vector element. I'm not sure that preserves semantic of inline assembly's "m" restriction, as it asks for memory address of original variable, not some copy.

Eric, what do you think?

Andrey

[andreybokhanko]

Ping!

[hfinkel]

In D10476#203538, @andreybokhanko wrote:

In D10476#202740, @hfinkel wrote:

I agree; we don't want to tie frontend functionality to a specific representation of the vectors.

We could support this, but we'd need to do it by:

1. Creating a local stack variable (alloca)
2. Extracting the requested vector element and storing it in that stack-allocated memory
3. Providing the address of the local stack variable to the inline asm
4. After the inline asm, loading the value from the local stack variable and inserting it back into the vector

I have no opinion on whether or not this is worth implementing.

Hal, what you suggested means basically creating a new local variable, copying value of a vector element to it and then providing address of this local variable, not original vector element. I'm not sure that preserves semantic of inline assembly's "m" restriction, as it asks for memory address of original variable, not some copy.

The real question is: Is the difference observable? When I made the suggestion, I did so because I felt the answer was no. But this is not true if you capture the address to use later. Thus, the difference is observable, and I'll vote that we simply not support this case.

Eric, what do you think?

Andrey

[echristo]

In D10476#209500, @hfinkel wrote:

In D10476#203538, @andreybokhanko wrote:

In D10476#202740, @hfinkel wrote:

I agree; we don't want to tie frontend functionality to a specific representation of the vectors.

We could support this, but we'd need to do it by:

1. Creating a local stack variable (alloca)
2. Extracting the requested vector element and storing it in that stack-allocated memory
3. Providing the address of the local stack variable to the inline asm
4. After the inline asm, loading the value from the local stack variable and inserting it back into the vector

I have no opinion on whether or not this is worth implementing.

Hal, what you suggested means basically creating a new local variable, copying value of a vector element to it and then providing address of this local variable, not original vector element. I'm not sure that preserves semantic of inline assembly's "m" restriction, as it asks for memory address of original variable, not some copy.

The real question is: Is the difference observable? When I made the suggestion, I did so because I felt the answer was no. But this is not true if you capture the address to use later. Thus, the difference is observable, and I'll vote that we simply not support this case.

Eric, what do you think?

I agree here.

Let's go with this patch right now and do anything else incrementally.

Thanks!

-eric

include/clang/AST/Expr.h

public:
   const ObjCPropertyRefExpr *getObjCProperty() const;
 
   /// \brief Check if this expression is the ObjC 'self' implicit parameter.
   bool isObjCSelfExpr() const;
 
   /// \brief Returns whether this expression refers to a vector element.
   bool refersToVectorElement() const;
 
+  /// \brief Returns whether this expression refers to a global register
+  /// variable.
+  bool refersToGlobalRegisterVar() const;
+
   /// \brief Returns whether this expression has a placeholder type.
   bool hasPlaceholderType() const {
     return getType()->isPlaceholderType();
   }
 
   /// \brief Returns whether this expression has a specific placeholder type.
   bool hasPlaceholderType(BuiltinType::Kind K) const {
     assert(BuiltinType::isPlaceholderTypeKind(K));

include/clang/Basic/DiagnosticSemaKinds.td

def err_asm_invalid_input_size : Error<
     "invalid input size for constraint '%0'">;
   def err_asm_invalid_output_size : Error<
     "invalid output size for constraint '%0'">;
   def err_invalid_asm_cast_lvalue : Error<
     "invalid use of a cast in a inline asm context requiring an l-value: "
     "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_bitfield_in_memory_constraint
-      : Error <"reference to a bit-field in asm "
+  def err_asm_non_addr_value_in_memory_constraint
+      : Error <"reference to a non-addressable value in asm "
       "%select{input|output}0 with a memory constraint '%1'">;
 
   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 l-value: "
     "accepted due to -fheinous-gnu-extensions, but clang may remove support "
     "for this in the future">;

lib/AST/Expr.cpp

if (const ArraySubscriptExpr *ASE = dyn_cast<ArraySubscriptExpr>(E))
     return ASE->getBase()->getType()->isVectorType();
 
   if (isa<ExtVectorElementExpr>(E))
     return true;
 
   return false;
 }
 
+bool Expr::refersToGlobalRegisterVar() const {
+  const Expr *E = this->IgnoreParenImpCasts();
+
+  if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
+    if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
+      if (VD->getStorageClass() == SC_Register &&
+          VD->hasAttr<AsmLabelAttr>() && !VD->isLocalVarDecl())
+        return true;
+
+  return false;
+}
+
 /// isArrow - Return true if the base expression is a pointer to vector,
 /// return false if the base expression is a vector.
 bool ExtVectorElementExpr::isArrow() const {
   return getBase()->getType()->isPointerType();
 }
 
 unsigned ExtVectorElementExpr::getNumElements() const {
   if (const VectorType *VT = getType()->getAs<VectorType>())

lib/Sema/SemaStmtAsm.cpp

for (unsigned i = 0; i != NumOutputs; i++) {
 
     // Check that the output exprs are valid lvalues.
     Expr *OutputExpr = Exprs[i];
 
     // Referring to parameters is not allowed in naked functions.
     if (CheckNakedParmReference(OutputExpr, *this))
       return StmtError();
 
-    // Bitfield can't be referenced with a pointer.
-    if (Info.allowsMemory() && OutputExpr->refersToBitField())
+    // Bitfields, vector elements and global register variables can't be
+    // referenced with a pointer.
+    if (Info.allowsMemory() && (OutputExpr->refersToBitField() ||
+                                OutputExpr->refersToVectorElement() ||
+                                OutputExpr->refersToGlobalRegisterVar()))
       return StmtError(Diag(OutputExpr->getLocStart(),
-                            diag::err_asm_bitfield_in_memory_constraint)
-                       << 1
-                       << Info.getConstraintStr()
+                            diag::err_asm_non_addr_value_in_memory_constraint)
+                       << 1 << Info.getConstraintStr()
                        << OutputExpr->getSourceRange());
 
     OutputConstraintInfos.push_back(Info);
 
     // If this is dependent, just continue.
     if (OutputExpr->isTypeDependent())
       continue;
 
for (unsigned i = NumOutputs, e = NumOutputs + NumInputs; i != e; i++) {
     Exprs[i] = ER.get();
 
     Expr *InputExpr = Exprs[i];
 
     // Referring to parameters is not allowed in naked functions.
     if (CheckNakedParmReference(InputExpr, *this))
       return StmtError();
 
-    // Bitfield can't be referenced with a pointer.
-    if (Info.allowsMemory() && InputExpr->refersToBitField())
+    // Bitfields, vector elements and global register variables can't be
+    // referenced with a pointer.
+    if (Info.allowsMemory() && (InputExpr->refersToBitField() ||
+                                InputExpr->refersToVectorElement() ||
+                                InputExpr->refersToGlobalRegisterVar()))
       return StmtError(Diag(InputExpr->getLocStart(),
-                            diag::err_asm_bitfield_in_memory_constraint)
-                       << 0
-                       << Info.getConstraintStr()
+                            diag::err_asm_non_addr_value_in_memory_constraint)
+                       << 0 << Info.getConstraintStr()
                        << InputExpr->getSourceRange());
 
     // Only allow void types for memory constraints.
     if (Info.allowsMemory() && !Info.allowsRegister()) {
       if (CheckAsmLValue(InputExpr, *this))
         return StmtError(Diag(InputExpr->getLocStart(),
                               diag::err_asm_invalid_lvalue_in_input)
                          << Info.getConstraintStr()

test/Sema/asm.c

 }
 
 // PR14269
 typedef struct test16_foo {
   unsigned int field1 : 1;
   unsigned int field2 : 2;
   unsigned int field3 : 3;
 } test16_foo;
-test16_foo x;
+typedef __attribute__((vector_size(16))) int test16_bar;
+register int test16_baz asm("rbx");
+
 void test16()
 {
+  test16_foo a;
+  test16_bar b;
+
+  __asm__("movl $5, %0"
+          : "=rm" (a.field2)); // expected-error {{reference to a non-addressable value in asm output with a memory constraint '=rm'}}
+  __asm__("movl $5, %0"
+          :
+          : "m" (a.field3)); // expected-error {{reference to a non-addressable value in asm input with a memory constraint 'm'}}
+  __asm__("movl $5, %0"
+          : "=rm" (b[2])); // expected-error {{reference to a non-addressable value in asm output with a memory constraint '=rm'}}
+  __asm__("movl $5, %0"
+          :
+          : "m" (b[3])); // expected-error {{reference to a non-addressable value in asm input with a memory constraint 'm'}}
   __asm__("movl $5, %0"
-          : "=rm" (x.field2)); // expected-error {{reference to a bit-field in asm output with a memory constraint '=rm'}}
+          : "=rm" (test16_baz)); // expected-error {{reference to a non-addressable value in asm output with a memory constraint '=rm'}}
   __asm__("movl $5, %0"
           :
-          : "m" (x.field3)); // expected-error {{reference to a bit-field in asm input with a memory constraint 'm'}}
+          : "m" (test16_baz)); // expected-error {{reference to a non-addressable value in asm input with a memory constraint 'm'}}
 }