Produce a better diagnostic for global register variables

[ahatanak]

Summary

clang doesn't print a very user-friendly message when an invalid register is used for a global register variable:

For example, when the following code is compiled,

$ cat f1.c
volatile register long long A asm ("rdi");

void foo1() {

A = 1;

}

clang prints this error message:

$ clang -c f1.c
fatal error: error in backend: Invalid register name global variable

The code fails to compile because "rdi" isn't a valid register for global register variables on x86 (rsp, rbp, esp, and ebp are the only registers that are currently valid), but the diagnostic doesn't give much detail on why it is an error or which line of the source code is not correct because the error is detected in the backend.

This patch makes changes in Sema to catch this kind of error earlier. In addition, it errors out if the size of the register doesn't match the declared variable size.

e.g., volatile register int B asm ("rbp");

[echristo]

My preference for this sort of thing would be an enum of failure reasons, but I guess this is ok for now. One inline request.

-eric

lib/Basic/Targets.cpp

3978–3981 ↗

(On Diff #37645)

This could use a comment, what are you testing here?

[ahatanak]

Added comments to clarify what the code is doing.

[echristo]

LGTM.

Thanks!

-eric

[ahatanak]

Thanks, I'll commit this patch shortly.

If it makes the code cleaner, I can define enums in TargetInfo and change validateGlobalRegisterVariable to return one of them in a follow-up patch.

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def err_asm_invalid_type_in_input : Error<
     "invalid type %0 in asm input for constraint '%1'">;
   def err_asm_tying_incompatible_types : Error<
     "unsupported inline asm: input with type "
     "%diff{$ matching output with type $|}0,1">;
   def err_asm_unexpected_constraint_alternatives : Error<
     "asm constraint has an unexpected number of alternatives: %0 vs %1">;
   def err_asm_incomplete_type : Error<"asm operand has incomplete type %0">;
   def err_asm_unknown_register_name : Error<"unknown register name '%0' in asm">;
+  def err_asm_invalid_global_var_reg : Error<"register '%0' unsuitable for "
+    "global register variables on this target">;
+  def err_asm_register_size_mismatch : Error<"size of register '%0' does not "
+    "match variable size">;
   def err_asm_bad_register_type : Error<"bad type for named register variable">;
   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">;

cfe/trunk/include/clang/Basic/TargetInfo.h

public:
     void setTiedOperand(unsigned N, ConstraintInfo &Output) {
       Output.setHasMatchingInput();
       Flags = Output.Flags;
       TiedOperand = N;
       // Don't copy Name or constraint string.
     }
   };
 
+  /// \brief Validate register name used for global register variables.
+  ///
+  /// This function returns true if the register passed in RegName can be used
+  /// for global register variables on this target. In addition, it returns
+  /// true in HasSizeMismatch if the size of the register doesn't match the
+  /// variable size passed in RegSize.
+  virtual bool validateGlobalRegisterVariable(StringRef RegName,
+                                              unsigned RegSize,
+                                              bool &HasSizeMismatch) const {
+    HasSizeMismatch = false;
+    return true;
+  }
+
   // validateOutputConstraint, validateInputConstraint - Checks that
   // a constraint is valid and provides information about it.
   // FIXME: These should return a real error instead of just true/false.
   bool validateOutputConstraint(ConstraintInfo &Info) const;
   bool validateInputConstraint(MutableArrayRef<ConstraintInfo> OutputConstraints,
                                ConstraintInfo &info) const;
 
   virtual bool validateOutputSize(StringRef /*Constraint*/,

cfe/trunk/lib/Basic/Targets.cpp

public:
   }
   ArrayRef<TargetInfo::AddlRegName> getGCCAddlRegNames() const override {
     return llvm::makeArrayRef(AddlRegNames);
   }
   bool validateCpuSupports(StringRef Name) const override;
   bool validateAsmConstraint(const char *&Name,
                              TargetInfo::ConstraintInfo &info) const override;
 
+  bool validateGlobalRegisterVariable(StringRef RegName,
+                                      unsigned RegSize,
+                                      bool &HasSizeMismatch) const override {
+    // esp and ebp are the only 32-bit registers the x86 backend can currently
+    // handle.
+    if (RegName.equals("esp") || RegName.equals("ebp")) {
+      // Check that the register size is 32-bit.
+      HasSizeMismatch = RegSize != 32;
+      return true;
+    }
+
+    return false;
+  }
+
   bool validateOutputSize(StringRef Constraint, unsigned Size) const override;
 
   bool validateInputSize(StringRef Constraint, unsigned Size) const override;
 
   virtual bool validateOperandSize(StringRef Constraint, unsigned Size) const;
 
   std::string convertConstraint(const char *&Constraint) const override;
   const char *getClobbers() const override {
public:
   }
 
   CallingConv getDefaultCallingConv(CallingConvMethodType MT) const override {
     return CC_C;
   }
 
   // for x32 we need it here explicitly
   bool hasInt128Type() const override { return true; }
+
+  bool validateGlobalRegisterVariable(StringRef RegName,
+                                      unsigned RegSize,
+                                      bool &HasSizeMismatch) const override {
+    // rsp and rbp are the only 64-bit registers the x86 backend can currently
+    // handle.
+    if (RegName.equals("rsp") || RegName.equals("rbp")) {
+      // Check that the register size is 64-bit.
+      HasSizeMismatch = RegSize != 64;
+      return true;
+    }
+
+    // Check if the register is a 32-bit register the backend can handle.
+    return X86TargetInfo::validateGlobalRegisterVariable(RegName, RegSize,
+                                                         HasSizeMismatch);
+  }
 };
 
 // x86-64 Windows target
 class WindowsX86_64TargetInfo : public WindowsTargetInfo<X86_64TargetInfo> {
 public:
   WindowsX86_64TargetInfo(const llvm::Triple &Triple)
       : WindowsTargetInfo<X86_64TargetInfo>(Triple) {
     WCharType = UnsignedShort;

cfe/trunk/lib/Sema/SemaDecl.cpp

if (S->getFnParent() != nullptr) {
         break;
       case SC_Static:
       case SC_Extern:
       case SC_PrivateExtern:
         break;
       }
     } else if (SC == SC_Register) {
       // Global Named register
-      if (!Context.getTargetInfo().isValidGCCRegisterName(Label) &&
-          DeclAttrsMatchCUDAMode(getLangOpts(), NewVD))
+      if (DeclAttrsMatchCUDAMode(getLangOpts(), NewVD)) {
+        const auto &TI = Context.getTargetInfo();
+        bool HasSizeMismatch;
+
+        if (!TI.isValidGCCRegisterName(Label))
           Diag(E->getExprLoc(), diag::err_asm_unknown_register_name) << Label;
+        else if (!TI.validateGlobalRegisterVariable(Label,
+                                                    Context.getTypeSize(R),
+                                                    HasSizeMismatch))
+          Diag(E->getExprLoc(), diag::err_asm_invalid_global_var_reg) << Label;
+        else if (HasSizeMismatch)
+          Diag(E->getExprLoc(), diag::err_asm_register_size_mismatch) << Label;
+      }
+
       if (!R->isIntegralType(Context) && !R->isPointerType()) {
         Diag(D.getLocStart(), diag::err_asm_bad_register_type);
         NewVD->setInvalidDecl(true);
       }
     }
 
     NewVD->addAttr(::new (Context) AsmLabelAttr(SE->getStrTokenLoc(0),
                                                 Context, Label, 0));

cfe/trunk/test/CodeGen/named_reg_global.c

-// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm %s -o - | FileCheck %s
-// RUN: %clang_cc1 -triple arm64-linux-gnu -S -emit-llvm %s -o - | FileCheck %s
-// RUN: %clang_cc1 -triple armv7-linux-gnu -S -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-X86-64
+// RUN: %clang_cc1 -triple arm64-linux-gnu -S -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-ARM
+// RUN: %clang_cc1 -triple armv7-linux-gnu -S -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-ARM
 
 // CHECK-NOT: @sp = common global
+
+#if defined(__x86_64__)
+register unsigned long current_stack_pointer asm("rsp");
+#else
 register unsigned long current_stack_pointer asm("sp");
+#endif
+
 struct p4_Thread {
   struct {
     int len;
   } word;
 };
 // Testing pointer types as well
+#if defined(__x86_64__)
+register struct p4_Thread *p4TH asm("rsp");
+#else
 register struct p4_Thread *p4TH asm("sp");
+#endif
 
 // CHECK: define{{.*}} i[[bits:[0-9]+]] @get_stack_pointer_addr()
 // CHECK: [[ret:%[0-9]+]] = call i[[bits]] @llvm.read_register.i[[bits]](metadata !0)
 // CHECK: ret i[[bits]] [[ret]]
 unsigned long get_stack_pointer_addr() {
   return current_stack_pointer;
 }
 // CHECK: declare{{.*}} i[[bits]] @llvm.read_register.i[[bits]](metadata)

 
 // CHECK: define {{.*}}@fn2
 void fn2(struct p4_Thread *val) {
   p4TH = val;
 }
 // CHECK: %[[regw:[0-9]+]] = ptrtoint %struct.p4_Thread* %{{.*}} to i[[bits]]
 // CHECK: call void @llvm.write_register.i[[bits]](metadata !0, i[[bits]] %[[regw]])
 
-// CHECK: !llvm.named.register.sp = !{!0}
-// CHECK: !0 = !{!"sp"}
+// CHECK-X86-64: !llvm.named.register.rsp = !{!0}
+// CHECK-X86-64: !0 = !{!"rsp"}
+// CHECK-ARM: !llvm.named.register.sp = !{!0}
+// CHECK-ARM: !0 = !{!"sp"}

cfe/trunk/test/OpenMP/atomic_capture_codegen.cpp

struct BitFields4_packed {
   short : 16;
   int a: 1;
   long b : 7;
 } __attribute__ ((__packed__)) bfx4_packed;
 
 typedef float float2 __attribute__((ext_vector_type(2)));
 float2 float2x;
 
-register int rix __asm__("0");
+// Register "0" is currently an invalid register for global register variables.
+// Use "esp" instead of "0".
+// register int rix __asm__("0");
+register int rix __asm__("esp");
 
 int main() {
 // CHECK: [[PREV:%.+]] = atomicrmw add i8* @{{.+}}, i8 1 monotonic
 // CHECK: store i8 [[PREV]], i8* @{{.+}},
 #pragma omp atomic capture
   bv = bx++;
 // CHECK: atomicrmw add i8* @{{.+}}, i8 1 monotonic
 // CHECK: add nsw i32 %{{.+}}, 1

cfe/trunk/test/OpenMP/atomic_read_codegen.c

struct BitFields4_packed {
   short : 16;
   int a: 1;
   long b : 7;
 } __attribute__ ((__packed__)) bfx4_packed;
 
 typedef float float2 __attribute__((ext_vector_type(2)));
 float2 float2x;
 
-register int rix __asm__("0");
+// Register "0" is currently an invalid register for global register variables.
+// Use "esp" instead of "0".
+// register int rix __asm__("0");
+register int rix __asm__("esp");
 
 int main() {
 // CHECK: load atomic i8, i8*
 // CHECK: store i8
 #pragma omp atomic read
   bv = bx;
 // CHECK: load atomic i8, i8*
 // CHECK: store i8

cfe/trunk/test/OpenMP/atomic_update_codegen.cpp

struct BitFields4_packed {
   short : 16;
   int a: 1;
   long b : 7;
 } __attribute__ ((__packed__)) bfx4_packed;
 
 typedef float float2 __attribute__((ext_vector_type(2)));
 float2 float2x;
 
-register int rix __asm__("0");
+// Register "0" is currently an invalid register for global register variables.
+// Use "esp" instead of "0".
+// register int rix __asm__("0");
+register int rix __asm__("esp");
 
 int main() {
 // CHECK-NOT: atomicrmw
 #pragma omp atomic
   ++dv;
 // CHECK: atomicrmw add i8* @{{.+}}, i8 1 monotonic
 #pragma omp atomic
   bx++;

cfe/trunk/test/OpenMP/atomic_write_codegen.c

struct BitFields4_packed {
   short : 16;
   int a: 1;
   long b : 7;
 } __attribute__ ((__packed__)) bfx4_packed;
 
 typedef float float2 __attribute__((ext_vector_type(2)));
 float2 float2x;
 
-register int rix __asm__("0");
+// Register "0" is currently an invalid register for global register variables.
+// Use "esp" instead of "0".
+// register int rix __asm__("0");
+register int rix __asm__("esp");
 
 int main() {
 // CHECK: load i8, i8*
 // CHECK: store atomic i8
 #pragma omp atomic write
   bx = bv;
 // CHECK: load i8, i8*
 // CHECK: store atomic i8

cfe/trunk/test/OpenMP/for_loop_messages.cpp

 // RUN: %clang_cc1 -fsyntax-only -fopenmp -x c++ -std=c++11 -fexceptions -fcxx-exceptions -verify %s
 
 class S {
   int a;
   S() : a(0) {}
 
 public:
   S(int v) : a(v) {}
   S(const S &s) : a(s.a) {}
 };
 
 static int sii;
 #pragma omp threadprivate(sii)
 static int globalii;
 
-register int reg0 __asm__("0");
+// Currently, we cannot use "0" for global register variables.
+// register int reg0 __asm__("0");
+int reg0;
 
 int test_iteration_spaces() {
   const int N = 100;
   float a[N], b[N], c[N];
   int ii, jj, kk;
   float fii;
   double dii;
   register int reg; // expected-warning {{'register' storage class specifier is deprecated}}

cfe/trunk/test/OpenMP/threadprivate_messages.cpp

public:
     static T s;
 #pragma omp threadprivate (s)
 };
 #pragma omp threadprivate (s) // expected-error {{use of undeclared identifier 's'}}
 
 static __thread int t; // expected-note {{'t' defined here}}
 #pragma omp threadprivate (t) // expected-error {{variable 't' cannot be threadprivate because it is thread-local}}
 
-register int reg0 __asm__("0"); // expected-note {{'reg0' defined here}}
+// Register "0" is currently an invalid register for global register variables.
+// Use "esp" instead of "0".
+// register int reg0 __asm__("0");
+register int reg0 __asm__("esp"); // expected-note {{'reg0' defined here}}
 #pragma omp threadprivate (reg0) // expected-error {{variable 'reg0' cannot be threadprivate because it is a global named register variable}}
 
 int o; // expected-note {{candidate found by name lookup is 'o'}}
 #pragma omp threadprivate (o)
 namespace {
 int o; // expected-note {{candidate found by name lookup is '(anonymous namespace)::o'}}
 #pragma omp threadprivate (o)
 #pragma omp threadprivate (o)

cfe/trunk/test/Sema/asm.c

 // RUN: %clang_cc1 %s -Wno-private-extern -triple i386-pc-linux-gnu -verify -fsyntax-only
 
 
-
 void f() {
   int i;
 
   asm ("foo\n" : : "a" (i + 2));
   asm ("foo\n" : : "a" (f())); // expected-error {{invalid type 'void' in asm input}}
 
   asm ("foo\n" : "=a" (f())); // expected-error {{invalid lvalue in asm output}}
   asm ("foo\n" : "=a" (i + 2)); // expected-error {{invalid lvalue in asm output}}
double test15() {
   __asm("0.0":"=,g"(ret)); // no-error
   __asm("0.0":"=g"(ret)); // no-error
   return ret;
 }
 
 // PR19837
 struct foo {
   int a;
-  char b;
 };
-register struct foo bar asm("sp"); // expected-error {{bad type for named register variable}}
-register float baz asm("sp"); // expected-error {{bad type for named register variable}}
+register struct foo bar asm("esp"); // expected-error {{bad type for named register variable}}
+register float baz asm("esp"); // expected-error {{bad type for named register variable}}
+
+register int r0 asm ("edi"); // expected-error {{register 'edi' unsuitable for global register variables on this target}}
+register long long r1 asm ("esp"); // expected-error {{size of register 'esp' does not match variable size}}
+register int r2 asm ("esp");
 
 double f_output_constraint(void) {
   double result;
   __asm("foo1": "=f" (result)); // expected-error {{invalid output constraint '=f' in asm}}
   return result;
 }
 
 void fn1() {

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

cfe/trunk/test/SemaCUDA/asm-constraints-mixed.cu

 // REQUIRES: x86-registered-target
 // REQUIRES: nvptx-registered-target
 // RUN: %clang_cc1 -triple nvptx-unknown-cuda -fsyntax-only -fcuda-is-device -verify %s
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -verify %s
 
 __attribute__((device)) register long global_dev_reg asm("r0");
 __attribute__((device)) register long
-    global_dev_hreg asm("rax"); // device-side error
+    global_dev_hreg asm("rsp"); // device-side error
 
-register long global_host_reg asm("rax");
+register long global_host_reg asm("rsp");
 register long global_host_dreg asm("r0"); // host-side error
 
 __attribute__((device)) void df() {
   register long local_dev_reg asm("r0");
-  register long local_host_reg asm("rax"); // device-side error
+  register long local_host_reg asm("rsp"); // device-side error
   short h;
   // asm with PTX constraints. Some of them are PTX-specific.
   __asm__("dont care" : "=h"(h) : "f"(0.0), "d"(0.0), "h"(0), "r"(0), "l"(0));
 }
 
 void hf() {
   register long local_dev_reg asm("r0"); // host-side error
-  register long local_host_reg asm("rax");
+  register long local_host_reg asm("rsp");
   int a;
   // Asm with x86 constraints and registers that are not supported by PTX.
   __asm__("dont care" : "=a"(a) : "a"(0), "b"(0), "c"(0) : "flags");
 }
 
 // Check errors in named register variables.
 // We should only see errors relevant to current compilation mode.
 #if defined(__CUDA_ARCH__)
 // Device-side compilation:
-// expected-error@8 {{unknown register name 'rax' in asm}}
-// expected-error@15 {{unknown register name 'rax' in asm}}
+// expected-error@8 {{unknown register name 'rsp' in asm}}
+// expected-error@15 {{unknown register name 'rsp' in asm}}
 #else
 // Host-side compilation:
 // expected-error@11 {{unknown register name 'r0' in asm}}
 // expected-error@22 {{unknown register name 'r0' in asm}}
 #endif