[PATCH] D41405: Fix an assertion failure regression in isDesignatorAtObjectEnd for __builtin_object_size with incomplete array type in struct

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D41405#960008, @george.burgess.iv wrote:

> LGTM assuming my nit gets addressed.
>
> Thank you!
>
> > Maybe someone who's more familiar with this builtin could point to the cause of this discrepancy
>
> Yeah, the documentation for this builtin is pretty sparse. My guess: clang assumes that the array's size is unknown because it's both an array and at the end of a struct. This exists because code will do clever things like
>
>   struct string {
>     size_t len;
>     char buf[1]; // actual string is accessed from here; `string` just gets overallocated to hold it all.
>   };
>
>
> in FreeBSD-land, they also recommend overallocation with sockaddrs, which have a 14-length trailing element: https://www.freebsd.org/doc/en/books/developers-handbook/sockets-essential-functions.html
>
> ...So, the best compatible heuristic we've been able to settle on here, sadly, is "are we touching the final element in a struct, and is that element an array?" On the bright side, clang failing just means LLVM gets to try to figure it out, so only some hope of getting a useful answer is lost. :)
>
> It's interesting that GCC tries to do better here, since AIUI it has a similar heuristic meant to cope with code like the above.


Thanks!



================
Comment at: test/Sema/builtin-object-size.c:105
+void rd36094951_IAS_builtin_object_size_assertion(IncompleteArrayStruct* p) {
+   __builtin___strlcpy_chk (p->session[0].string, "ab", 2, __builtin_object_size(p->session[0].string, 1));
+}
----------------
george.burgess.iv wrote:
> vsapsai wrote:
> > Do we execute significantly different code paths when the second `__builtin_object_size` argument is 0, 2, 3? I think it is worth checking locally if these values aren't causing an assertion. Not sure about having such tests permanently, I'll leave it to you as you are more familiar with the code.
> In this case, only 1 and 3 should be touching the buggy codepath, and they should execute it identically. If 0 and 2 reach there, we have bigger problems, since they shouldn't really be poking around in the designator of the given LValue.
> 
> Since it's presumably only ~10 seconds of copy-pasting, I'd be happy if we added sanity checks for other modes, as well. :)
Sure, I'll test the other modes as well.


Repository:
  rC Clang

https://reviews.llvm.org/D41405