[PATCH] D148381: [Clang] Add counted_by attribute

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

I changed more than just the testcase (not a lot, but non-trivial nonetheless). PTAL.



================
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
+
----------------
nickdesaulniers wrote:
> 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.
Done.


================
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
----------------
nickdesaulniers wrote:
> Is the call to `func` is unnecessary for this test or can we just `return __builtin_dynamic_object_size(p->array, 1)`?
Nope. Not sure why I had it. :-/ Removed.


================
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
----------------
nickdesaulniers wrote:
> 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?
This is a bit weird. I don't understand why it would trap either. It might have something to do with the 'alloc_size' attribute? (The code that this patch generates doesn't put that trap in there.) That's probably a bug.

However, when I modify the test to include an actual store:

```
void test3(int index) {
  /* ... */
  p->array[index] = 42;
}
```

I get two checks: one for the 'counted_by' attribute, and this one, probably for the bounds check. Again, it's probably because of the 'alloc_size', because when I remove the call to 'malloc' it generates only the 'counted_by' check...



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381