Fix stack address builtin for negative numbers

[zoecarver]

Summary

This patch checks that an argument passed to either of the builtins __builtin_frame_address or __builtin_return_address is at least 0. This patch resolves the issue where these builtins would cause an infinite loop ( 25497 ).

[zoecarver]

clang/test/Sema/builtin-stackaddress.c

10

For some reason lit was still erroring here.

[efriedma]

We usually prefer to generate error messages for incorrect parameters to builtins in SemaChecking.cpp.

I don't think there's really an infinite loop here. The parameter to __builtin_frame_address is an unsigned integer; values greater than 0x7FFFFFFU aren't special. The reason it takes a very long time is that we try to unroll the implied loop; generating an object file with four billion load instructions takes essentially forever. We could impose a smaller limit, but it would be sort of arbitrary.

[zoecarver]

Thanks for the explanation. Should I then not try to "fix" the issue? Or should I update sema checking?

[efriedma]

In the context of __builtin_frame_address, an arbitrary limit is probably okay. Maybe something like 0xFFFF, which is larger than anyone would realistically use, but doesn't take a crazy amount of time to compile.

[Jim]

I think this checking should be implemented in lib/Sema/SemaChecking.cpp.

[zoecarver]

@Jim Thanks, I'll move it.

[zoecarver]

• move verification to SemaChecking

[efriedma]

LGTM

[zoecarver]

Resolved by c93112dc4f745b0455addb54bfe1c2f79b827c6d

[zoecarver]

Reverted: 698078257285a044110620d7dab2fb4451a3fa29

[zoecarver]

• Error if SemaBuiltinConstantArgRange returns true, not false

[zoecarver]

I was using SemaBuiltinConstantArgRange incorrectly. I was returning an error for !SemaBuiltinConstantArgRange instead of SemaBuiltinConstantArgRange. Also, I only modified the fail tests so I wasn't able to catch the error. Updated and am running both the Sema and CodeGen tests now. Should fix the error.

[zoecarver]

• diff from master (arg phab...)

clang/lib/Sema/SemaChecking.cpp

case Builtin::BIget_kernel_sub_group_count_for_ndrange:
     if (SemaOpenCLBuiltinNDRangeAndBlock(*this, TheCall))
       return ExprError();
     break;
   case Builtin::BI__builtin_os_log_format:
   case Builtin::BI__builtin_os_log_format_buffer_size:
     if (SemaBuiltinOSLogFormat(TheCall))
       return ExprError();
     break;
+  case Builtin::BI__builtin_frame_address:
+  case Builtin::BI__builtin_return_address:
+    if (SemaBuiltinConstantArgRange(TheCall, 0, 0, 0xFFFF))
+      return ExprError();
+    break;
   }
 
   // Since the target specific builtins for each arch overlap, only check those
   // of the arch we are compiling for.
   if (Context.BuiltinInfo.isTSBuiltin(BuiltinID)) {
     switch (Context.getTargetInfo().getTriple().getArch()) {
       case llvm::Triple::arm:
       case llvm::Triple::armeb:

clang/test/Sema/builtin-stackaddress.c

 // RUN: %clang_cc1 -fsyntax-only -verify %s
+
 void* a(unsigned x) {
 return __builtin_return_address(0);
 }
 
 void b(unsigned x) {
 return __builtin_return_address(x); // expected-error{{argument to '__builtin_return_address' must be a constant integer}}
 }
 
 void* c(unsigned x) {
+// expected-error@+1 {{argument value 4294967295 is outside the valid range [0, 65535]}}
+return __builtin_return_address(-1);
+}
+
+void* d(unsigned x) {
+// expected-error@+1 {{argument value 1048575 is outside the valid range [0, 65535]}}
+return __builtin_return_address(0xFFFFF);
+}
+
+void* e(unsigned x) {
 return __builtin_frame_address(0);
 }
 
-void d(unsigned x) {
-return __builtin_frame_address(x); // expected-error{{argument to '__builtin_frame_address' must be a constant integer}}
+void f(unsigned x) {
+// expected-error@+1 {{argument to '__builtin_frame_address' must be a constant integer}}
+return __builtin_frame_address(x);
+}
+
+void* g(unsigned x) {
+// expected-error@+1 {{argument value 4294967295 is outside the valid range [0, 65535]}}
+return __builtin_frame_address(-1);
+}
+
+void* h(unsigned x) {
+// expected-error@+1 {{argument value 1048575 is outside the valid range [0, 65535]}}
+return __builtin_frame_address(0xFFFFF);
 }