[clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[jtmott-intel]

Summary

Motivating example:

struct { int v[10]; } t[10];

__builtin_object_size(
    &t[0].v[11], // access past end of subobject
    1            // request remaining bytes of closest surrounding subobject
);

In GCC, this returns 0. https://godbolt.org/z/7TeGs7

In current clang, however, this returns 356, the number of bytes remaining in the whole variable, as if the type was 0 instead of 1. https://godbolt.org/z/6Kffox

This patch checks for the specific case where we're requesting a subobject's size (type 1) but the subobject is invalid.

[aaron.ballman]

Adding some reviewers and subscribing the mailing lists. FWIW, the changes seem reasonable to me.

[george.burgess.iv]

thanks for working on this!

just one tiny nit and lgtm

clang/lib/AST/ExprConstant.cpp

11410–11411

nit: the new part of this condition makes this comment somewhat outdated. should it say something like "outside of" instead of "before"?

[jtmott-intel]

Updated comments to reflect "outside of" instead of "before".

[jtmott-intel]

Seems I don't have commit access. I'll look into it. For now, could someone push this commit? Thanks.

[pcc]

This causes us to reject the following (reduced from AOSP):

int sprintf(char* __s, const char* __fmt, ...)
    __attribute__((__format__(printf, 2, 3))) ;
int sprintf(char* dest, const char* format)
    __attribute__((overloadable))
    __attribute__((enable_if(((__builtin_object_size(((dest)), (1))) != ((unsigned long) -1) && (__builtin_object_size(((dest)), (1))) < (__builtin_strlen(format))), "format string will always overflow destination buffer")))

    __attribute__((unavailable("format string will always overflow destination buffer")));

void f() {
  unsigned char number_buffer[26] = {0};
  sprintf((char *)number_buffer, "null");
}

It doesn't seem like we ought to be rejecting this case. Can you please take a look?

[manojgupta]

This CL has caused two issues in Chrome OS :
Compilation fail with FORTIFY: https://bugs.chromium.org/p/chromium/issues/detail?id=1168199
Runtime failures: https://bugs.chromium.org/p/chromium/issues/detail?id=1167504

I have requested George to take a look but will it be ok to revert this meanwhile?

[aaron.ballman]

In D92892#2510299, @manojgupta wrote:

This CL has caused two issues in Chrome OS :
Compilation fail with FORTIFY: https://bugs.chromium.org/p/chromium/issues/detail?id=1168199
Runtime failures: https://bugs.chromium.org/p/chromium/issues/detail?id=1167504

I have requested George to take a look but will it be ok to revert this meanwhile?

Yes, it's fine to revert this until we get these issues ironed out.

[george.burgess.iv]

reverted in https://github.com/llvm/llvm-project/commit/b270fd59f0a86fe737853abc43e76b9d29a67eea until we can figure out how to address the issues outlined above. thanks!

[aaron.ballman]

In D92892#2510445, @george.burgess.iv wrote:

reverted in https://github.com/llvm/llvm-project/commit/b270fd59f0a86fe737853abc43e76b9d29a67eea until we can figure out how to address the issues outlined above. thanks!

Thank you for this! I've also pinged @jtmott-intel internally in case he's not seen the post-commit feedback.

clang/lib/AST/ExprConstant.cpp

if (E->isGLValue()) {
       APValue RVal;
       if (!EvaluateAsRValue(Info, E, RVal))
         return false;
       LVal.setFrom(Info.Ctx, RVal);
     } else if (!EvaluatePointer(ignorePointerCastsAndParens(E), LVal, Info,
                                 /*InvalidBaseOK=*/true))
       return false;
   }
 
-  // If we point to before the start of the object, there are no accessible
-  // bytes.
-  if (LVal.getLValueOffset().isNegative()) {
+  // If we point outside of the object, there are no accessible bytes.
+  if (LVal.getLValueOffset().isNegative() ||
+      ((Type & 1) && !LVal.Designator.isValidSubobject())) {
     Size = 0;
     return true;
   }
 
   CharUnits EndOffset;
   if (!determineEndOffset(Info, E->getExprLoc(), Type, LVal, EndOffset))
     return false;
 

clang/test/CodeGen/object-size.c

void test24() {
   // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0, 3);
 }
 
 // CHECK-LABEL: @test25
 void test25() {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 2);
   // Note: Currently fixed at zero because LLVM can't handle type=3 correctly.
   // Hopefully will be lowered properly in the future.
   // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 3);
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 2);
   // Note: Currently fixed at zero because LLVM can't handle type=3 correctly.
   // Hopefully will be lowered properly in the future.
   // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 3);
 }
 
 // CHECK-LABEL: @test26
 void test26() {
   struct { int v[10]; } t[10];
 
   // CHECK: store i32 316
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[11], 0);
-  // CHECK: store i32 312
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[12], 1);
   // CHECK: store i32 308
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[13], 2);
   // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[14], 3);
 }
 
 struct Test27IncompleteTy;
void test29(struct DynStructVar *dv, struct DynStruct0 *d0,
   gi = OBJECT_SIZE_BUILTIN(dv->snd, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(dv->snd, 2);
   // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(dv->snd, 3);
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 2);
   // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 3);
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(d1->snd, 0);
void test31() {
   gi = OBJECT_SIZE_BUILTIN(ds1[9].snd, 1);
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(&ss[9].snd[0], 1);
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(&ds1[9].snd[0], 1);
 
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(&ds0[9].snd[0], 1);
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(&dsv[9].snd[0], 1);
 }
 
 // CHECK-LABEL: @PR30346
 void PR30346() {