[PATCH] D148381: [Clang] Add counted_by attribute

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Patch LGTM; just want moar tests.

---

Mind adding a test for

  #if !__has_attribute(counted_by)
  #error "has attribute broken"
  #endif



================
Comment at: clang/test/CodeGen/attr-counted-by.c:2
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 3
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -O2 -Wall -fsanitize=array-bounds,object-size,local-bounds -fstrict-flex-arrays=3 -emit-llvm -o - %s | FileCheck %s
+
----------------
Can you add another run line without the `-fsanitize` flags set, and use 2 different `--check-prefixes` for the two RUN lines?  I'd be curious to see the differences in codegen between those set or not.  I assume this attribute should affect codegen even with all of those disabled (maybe trapping instead of libcalling into ubsan runtime).  I think update_cc_test_checks should be able to handle that.


================
Comment at: clang/test/CodeGen/attr-counted-by.c:39-51
+// CHECK-LABEL: define dso_local void @test2(
+// CHECK-SAME: ptr noundef [[P:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[COUNT:%.*]] = getelementptr inbounds [[STRUCT_ANNOTATED:%.*]], ptr [[P]], i64 0, i32 1
+// CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[COUNT]], align 8, !tbaa [[TBAA2]]
+// CHECK-NEXT:    [[TMP1:%.*]] = shl i32 [[TMP0]], 2
+// CHECK-NEXT:    [[TMP2:%.*]] = zext i32 [[TMP1]] to i64
----------------
Is the call to `func` is unnecessary for this test or can we just `return __builtin_dynamic_object_size(p->array, 1)`?


================
Comment at: clang/test/CodeGen/attr-counted-by.c:67-73
+// CHECK-NEXT:    br i1 [[TMP4]], label [[TRAP:%.*]], label [[TMP5:%.*]]
+// CHECK:       5:
+// CHECK-NEXT:    tail call void @func(i64 noundef [[ADD1]]) #[[ATTR5]]
+// CHECK-NEXT:    ret void
+// CHECK:       trap:
+// CHECK-NEXT:    tail call void @llvm.trap() #[[ATTR4]]
+// CHECK-NEXT:    unreachable
----------------
What is the intent of this test? Mind adding a comment?

In particular, it's not clear to my //why// this case should conditionally trap, seeing as the dynamically allocated object's `array` field is yet to be accessed in this example.  What am I missing?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148381/new/

https://reviews.llvm.org/D148381