[PATCH] D135727: [clang] Correct sanitizer behavior in union FAMs

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D135727#3854252 <https://reviews.llvm.org/D135727#3854252>, @kees wrote:

> In D135727#3853896 <https://reviews.llvm.org/D135727#3853896>, @void wrote:
>
>> @kees @serge-sans-paille: It appears to me that a terminating array of size > 2 *isn't* treated as a FAM in Clang, at least Clang warns about it. The first failure above (`clang/test/Sema/array-bounds-ptr-arith.c`) shows that. It turns out that the same failure occurs in that testcase when the array isn't the last in a structure, so I'll change it.
>
> Okay, what the heck is even going on in this test? The diagnostic appears to think the array changes in size based on the cast??

I think we opened the can or worms. :-)

>   <source>:16:24: warning: the pointer incremented by 80 refers past the end of the array (that contains 64 elements) [-Warray-bounds-pointer-arithmetic]
>           return (void *)es->s_uuid + 80;
>                          ^            ~~
>
> s_uuid is 8, not 64. 64 would be 8 "void *"s.  This seems like a very very broken diagnostic?
>
> These should all trip the diagnostic, but don't:
>
>   es->s_uuid + 8; /* this is past the end */
>   (void *)es->s_uuid + 9; /* this is past the end by 1, but doesn't trip because it thinks it's suddenly 8 times larger */
>
> For -Warray-bounds, GCC isn't fooled by the "void *" casts (but doesn't have -Warray-bounds-arithmetic):
> https://godbolt.org/z/5eqEEv4of
>
> Note that while the diagnostics of both GCC and Clang complain only about >1 terminal arrays, they both return -1 for `__builtin_object_size` regardless of length.
>
> So, we're facing, again, a disconnect between diagnostics, bos, and sanitizer. GCC's sanitizer follows bos rules, where-as Clang's sanitizer followed diagnostics rules. Given that bos is used for run-time analysis, it follows that sanitizer and bos should match.

I created a separate patch to fix the diagnostic (https://reviews.llvm.org/D135920).

>> There's another failure that I'm not too sure about. The `Clang.SemaCXX::array-bounds.cpp` failure is due to a union that's acting like an FAM. I have a question for you. Should `a` and `c` in the union in this code be considered an FAM?
>
> This test looks correct to me:
>
>>   struct {
>>     union {
>>       short a[2]; // expected-note 4 {{declared here}}
>>       char c[4];
>>     };
>>     int ignored;
>>   };
>
> I would **not** expect this to warn or trap because it's not the trailing member of the //structure//.

Yeah, that's what I thought. I'll send out a separate fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135727