[Clang] Add __builtin_counted_by_ref builtin

[bwendling]

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.

[bwendling]

@AaronBallman @rapidsna -- I think I covered all of the comments you made on the other PR (stupid git!). PTAL.

[AaronBallman]

Can you add a test with -ast-print that shows we print out the builtin as it's used in the source, and a test with -ast-dump that shows the AST retains information about the use of the builtin?

[bwendling]

-ast-print testcase added.

[rapidsna]

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?

[Sirraide]

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.

[rapidsna]

I agree.

[bwendling]

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

[rapidsna]

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.

[bwendling]

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?

[rapidsna]

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).

[bwendling]

Done.

[rapidsna]

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

[bwendling]

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.

[rapidsna]

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.

[bwendling]

*__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.

[bwendling]

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

[bwendling]

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.

[rapidsna]

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

[bwendling]

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

[bwendling]

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?

[rapidsna]

Nit: we can also add __builtin_counted_by_ref(ptr->array[0]);

[rapidsna]

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]

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

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