[Clang] Add __builtin_counted_by_ref builtin #114495

bwendling · 2024-11-01T01:05:50Z

The __builtin_counted_by_ref builtin is used on a flexible array
pointer and returns a pointer to the "counted_by" attribute's COUNT
argument, which is a field in the same non-anonymous struct as the
flexible array member. This is useful for automatically setting the
count field without needing the programmer's intervention. Otherwise
it's possible to get this anti-pattern:

  ptr = alloc(<ty>, ..., COUNT);
  ptr->FAM[9] = 42; /* <<< Sanitizer will complain */
  ptr->count = COUNT;

To prevent this anti-pattern, the user can create an allocator that
automatically performs the assignment:

  #define alloc(TY, FAM, COUNT) ({ \
      TY __p = alloc(get_size(TY, COUNT));             \
      if (__builtin_counted_by_ref(__p->FAM))          \
          *__builtin_counted_by_ref(__p->FAM) = COUNT; \
      __p;                                             \
  })

The builtin's behavior is heavily dependent upon the "counted_by"
attribute existing. It's main utility is during allocation to avoid
the above anti-pattern. If the flexible array member doesn't have that
attribute, the builtin becomes a no-op. Therefore, if the flexible
array member has a "count" field not referenced by "counted_by", it
must be set explicitly after the allocation as this builtin will
return a "nullptr" and the assignment will most likely be elided.

The __builtin_counted_by_ref builtin is used on a flexible array pointer and returns a pointer to the "counted_by" attribute's COUNT argument, which is a field in the same non-anonymous struct as the flexible array member. This is useful for automatically setting the count field without needing the programmer's intervention. Otherwise it's possible to get this anti-pattern: ptr = alloc(<ty>, ..., COUNT); ptr->FAM[9] = 42; /* <<< Sanitizer will complain */ ptr->count = COUNT; To prevent this anti-pattern, the user can create an allocator that automatically performs the assignment: #define alloc(TY, FAM, COUNT) ({ \ TY __p = alloc(get_size(TY, COUNT)); \ if (__builtin_counted_by_ref(__p->FAM)) \ *__builtin_counted_by_ref(__p->FAM) = COUNT; \ __p; \ }) The builtin's behavior is heavily dependent upon the "counted_by" attribute existing. It's main utility is during allocation to avoid the above anti-pattern. If the flexible array member doesn't have that attribute, the builtin becomes a no-op. Therefore, if the flexible array member has a "count" field not referenced by "counted_by", it must be set explicitly after the allocation as this builtin will return a "nullptr" and the assignment will most likely be elided.

…d ignores pretty much everythings that may 'hide' the underlying MemberExpr.

…t the return type based on the 'count' field's type.

FindCountedByField can be used in more places than CodeGen. Move it into FieldDecl to avoid layering issues.

…on-pathological expressions.

…d / or count field.

…rn type.

…. Go back to 'size_t *' as the default return type.

rapidsna · 2024-11-04T22:42:29Z

clang/test/CodeGen/builtin-counted-by-ref.c

+  *_Generic(
+    __builtin_counted_by_ref(&p->array[0]),
+      void *: &__ignored,
+      default: __builtin_counted_by_ref(&p->array[idx++])) = size;


I see you have a test with a side effect. Though idx++ is a parameter so the side effect would be optimized out in this O2 test anyway even if it was codegen'ed. Could you please use something that won't be optimized out to make sure the test is really checking it's not codegen'ed?

Also, is it intentionally to make &p->array[non-zero] work?

Wait, why are we doing -O2 in a test to begin with? Afaik codegen tests are usually supposed to run w/o optimisation; otherwise, you just end up testing the optimiser, and that’s not what Clang’s regression tests are for, after all.

I removed the -O2 now. So the idx shouldn't be optimized away.

Thanks! Do you have a test when the base of member has a side effect? e.g., __builtin_counted_by_ref(foo().array) The behavior seems to be different than idx++ case, because when the base has a side effect the codegen seems to return (void *)0 instead of returning the count reference. My inclination is to reject code or emitting a different warning message when the base has a side effect because the programmer might want to fix that instead of it being silently ignored or confused about the behavior.

If I do something like this:

*_Generic( __flex_counter((p++)->array), void *: &__ignored_assignment, default: __flex_counter(p->array)) = 42;

The code generation seems to take it in stride (with a warning of course):

define dso_local noalias noundef ptr @alloc_s(i32 noundef %size) local_unnamed_addr #0 { entry: %0 = load ptr, ptr @p, align 8, !tbaa !5 %..counted_by.gep = getelementptr inbounds i8, ptr %0, i64 4 store i8 42, ptr %..counted_by.gep, align 1, !tbaa !9 ret ptr null }

I'm not sure if that's good or bad. In truth, the warning should probably be upgraded to an error, as it will lead to unexpected behavior if the warning is ignored. Thoughts?

Yes, upgrading to an error sounds good to me! Also, the message might need to be updated (since the side effect is not actually ignored).

clang/lib/CodeGen/CGBuiltin.cpp

rapidsna · 2024-11-04T23:08:21Z

clang/test/Sema/builtin-counted-by-ref.c

@@ -16,6 +16,8 @@ void test1(struct fam_struct *ptr, int size, int idx) {

  *__builtin_counted_by_ref(ptr->array) = size;             // ok
  *__builtin_counted_by_ref(&ptr->array[idx]) = size;       // ok
+  *__builtin_counted_by_ref(&ptr->array) = size;            // ok


Is it okay? Or should we make it an ill-form?

It looks like it generates correct code:

{ unsigned long int __ignored_assignment; *_Generic( __flex_counter(&p->array), void *: &__ignored_assignment, default: __flex_counter(&p->array)) = 42; } /* define dso_local noalias noundef ptr @alloc_s(i32 noundef %size) local_unnamed_addr #0 { entry: %0 = load ptr, ptr @p, align 8, !tbaa !5 %..counted_by.gep = getelementptr inbounds i8, ptr %0, i64 4 store i8 42, ptr %..counted_by.gep, align 1, !tbaa !9 ret ptr null } */

But if you prefer it to be ill-formed, then I can do that.

I guess this will just work too? *__builtin_counted_by_ref(ptr->array[idx]) = size;

Do you have a use case? I'd prefer these being ill-formed because the user might find it working unexpectedly. But I don't have a strong opinion on this. I'd leave it up to you/Linux users if you find this would be useful.

*__builtin_counted_by_ref(ptr->array[idx]) = size; doesn't work because the builtin requires a pointer to the FAM. (If you meant &ptr->array[idx], that does work.)

I don't have any use case for &ptr->array or the like. I just don't want to be overly restrictive on what's considered a valid argument to the builtin. I think I'll do a comparison with gcc's version and see what they accept. We can match them.

GCC rejects everything that isn't an array. So not even &ptr->array[0]. I guess we can do that.

Note that, because GCC only checks if the argument is an array, it allows for things like:

int global_array[]; void foo(int val) { *__builtin_counted_by_ref(global_array) = val; }

which isn't great.

I think we can still keep it an error and have GCC to reflect our model on this one, unless Linux relies on this?

Yeah. I don't want to allow for global arrays either, just flexible array members.

bwendling · 2024-11-04T23:13:22Z

Okay. I removed the -O2 from the testcases. That should prevent optimizations from removing the idx++. I removed the frivolous setting if the InBounds. As for the &ptr->array question, if we're able to get the proper information from it I don't see why we can't have it :-). Anything else?

clang/test/CodeGen/attr-counted-by-pr88931.cpp

clang/include/clang/Basic/Builtins.td

…ementation.

clang/test/Sema/builtin-counted-by-ref.c

rapidsna · 2024-11-05T23:34:09Z

I still see CodeGen/attr-counted-by*.c being unnecessarily changed. Should we just revert it back to llvm:main. Other than a couple of minor comments on the tests, LGTM! Thanks for addressing feedback. You might still wait on @AaronBallman though.

Sirraide · 2024-11-06T07:16:01Z

I still see CodeGen/attr-counted-by*.c being unnecessarily changed.

Yeah, the -O2 definitely needs to be added back to those tests.

bwendling · 2024-11-07T12:20:07Z

I still see CodeGen/attr-counted-by*.c being unnecessarily changed.

Yeah, the -O2 definitely needs to be added back to those tests.

Fine, but I'm not touching anymore testcases again. I'm getting quite sick of you two ping-ponging back and forth on already good testcases.

nikic · 2024-11-08T10:15:36Z

This change causes a large compile-time regression: https://llvm-compile-time-tracker.com/compare.php?from=ae9d0623ad65d84022bb4ed8446b6491451ae575&to=7475156d49406785a974b1205d11fe3de9c1553e&stat=instructions:u

From a quick look, I'd guess it's due the extra AST traversals you have added in SemaExpr.

rapidsna · 2024-11-08T17:34:23Z

@nikic thanks for reporting the issue. I filed #115520. We should probably be able to remove the AST traversals. I will take a look at it.

bwendling · 2024-11-11T22:55:24Z

I'll check it out. Sorry about the regression.

The __builtin_counted_by_ref builtin is used on a flexible array pointer and returns a pointer to the "counted_by" attribute's COUNT argument, which is a field in the same non-anonymous struct as the flexible array member. This is useful for automatically setting the count field without needing the programmer's intervention. Otherwise it's possible to get this anti-pattern: ptr = alloc(<ty>, ..., COUNT); ptr->FAM[9] = 42; /* <<< Sanitizer will complain */ ptr->count = COUNT; To prevent this anti-pattern, the user can create an allocator that automatically performs the assignment: #define alloc(TY, FAM, COUNT) ({ \ TY __p = alloc(get_size(TY, COUNT)); \ if (__builtin_counted_by_ref(__p->FAM)) \ *__builtin_counted_by_ref(__p->FAM) = COUNT; \ __p; \ }) The builtin's behavior is heavily dependent upon the "counted_by" attribute existing. It's main utility is during allocation to avoid the above anti-pattern. If the flexible array member doesn't have that attribute, the builtin becomes a no-op. Therefore, if the flexible array member has a "count" field not referenced by "counted_by", it must be set explicitly after the allocation as this builtin will return a "nullptr" and the assignment will most likely be elided. --------- Co-authored-by: Bill Wendling <[email protected]> Co-authored-by: Aaron Ballman <[email protected]>

bwendling and others added 30 commits October 31, 2024 18:02

If the 'counted_by' attribute doesn't exist, then return nullptr.

e3583c7

Remove debugging code.

053bfcd

Make returning a nullptr the default.

4ce6525

clang-format

8017e73

Use a visitor to retrieve the MemberExpr. It's intentionally harsh an…

8808431

…d ignores pretty much everythings that may 'hide' the underlying MemberExpr.

Add testcases.

122d2b8

Combine the code to grab the MemberExpr, if it exists.

2b58283

Move FindCountedByField into a central place.

76f01fe

Use CustomTypeChecking for the builtin. It allows us explicitly to se…

c95e38c

…t the return type based on the 'count' field's type.

[Clang][NFC] Move FindCountedByField into FieldDecl (llvm#104235)

925fbd2

FindCountedByField can be used in more places than CodeGen. Move it into FieldDecl to avoid layering issues.

Fix a build failure.

ec62a2b

Remove empty line.

d3ca48c

Remove merge errors.

85b4474

Use 'size_t *' for the default return type and improve the testcase.

91a7d1b

Don't use a Visitor to retrieve the MemberExpr. Instead accept only n…

2d95e44

…on-pathological expressions.

Improve error messages. Make the side-effects error a warning.

9628b0f

Add _Generic checks.

c278c1f

Remove unrelated change

0f42e5d

Use MemberExpr::Create instead of MemberExpr::CreateImplicit.

751a55d

Regenerate

53272cf

Add -W flag for warning.

ecda8e0

Handle any anonymous structs surrounding the flexible array member an…

612000f

…d / or count field.

Add anonymous struct testcase.

b555af9

Report a fatal error if we can't find the 'count' field.

48707ae

Use 'cast' instead of 'dyn_cast' and default to 'void *' for the retu…

5c778ca

…rn type.

A 'void *' can result in an error message about assigning to a 'void'…

a1a6ead

…. Go back to 'size_t *' as the default return type.

Add some documentation.

6b388b5

Rename to __builtin_counted_by_ref.

791443b

Rename method to reflect new builtin name.

768e7b1

rapidsna reviewed Nov 4, 2024

View reviewed changes

clang/lib/CodeGen/CGBuiltin.cpp Outdated Show resolved Hide resolved

rapidsna reviewed Nov 4, 2024

View reviewed changes

bwendling added 2 commits November 4, 2024 15:09

Remote -O2 from codegen.

3b50e8d

Remove setting the 'inBounds' flag.

d2112e5

Upgrade the side-effects warning into an error.

d595618

rapidsna reviewed Nov 5, 2024

View reviewed changes

clang/test/CodeGen/attr-counted-by-pr88931.cpp Outdated Show resolved Hide resolved

rapidsna reviewed Nov 5, 2024

View reviewed changes

clang/include/clang/Basic/Builtins.td Show resolved Hide resolved

bwendling added 3 commits November 4, 2024 20:38

Restrict __builtin_counted_by_ref to 'ptr->array' to match GCC's impl…

faffcf8

…ementation.

Regenerate.

426782f

Revert the modifications as they're not affected by this feature.

4892df7

rapidsna reviewed Nov 5, 2024

View reviewed changes

clang/test/Sema/builtin-counted-by-ref.c Show resolved Hide resolved

bwendling added 2 commits November 7, 2024 04:18

Add testcase.

e26e5b6

Revert.

2a0c003

bwendling added 3 commits November 7, 2024 13:24

Revert unaffected tests.

26d9e13

Merge branch 'llvm:main' into builtin_get_counted_by

5a66155

Fix Sphinx formatting.

c37a6b3

rapidsna added the clang:bounds-safety Issue/PR relating to the experimental -fbounds-safety feature in Clang label Nov 7, 2024

rapidsna approved these changes Nov 7, 2024

View reviewed changes

bwendling merged commit 7475156 into llvm:main Nov 7, 2024
7 of 9 checks passed

rapidsna mentioned this pull request Nov 8, 2024

compile time regression introduced by Sema checking for __builtin_counted_by_ref #115520

Closed

bwendling deleted the builtin_get_counted_by branch November 12, 2024 00:41

[Clang] Add __builtin_counted_by_ref builtin #114495

[Clang] Add __builtin_counted_by_ref builtin #114495

Uh oh!

Conversation

bwendling commented Nov 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rapidsna Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rapidsna Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwendling Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rapidsna Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwendling commented Nov 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rapidsna commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sirraide commented Nov 6, 2024

Uh oh!

bwendling commented Nov 7, 2024

Uh oh!

Uh oh!

nikic commented Nov 8, 2024

Uh oh!

rapidsna commented Nov 8, 2024

Uh oh!

bwendling commented Nov 11, 2024

Uh oh!

Uh oh!

rapidsna Nov 4, 2024 •

edited

Loading

rapidsna Nov 4, 2024 •

edited

Loading

bwendling Nov 4, 2024 •

edited

Loading

rapidsna Nov 5, 2024 •

edited

Loading

rapidsna commented Nov 5, 2024 •

edited

Loading