On 3/17/21 3:03 PM, Martin Sebor wrote:
On 3/16/21 2:24 PM, Jason Merrill wrote:
On 3/11/21 1:06 PM, Martin Sebor wrote:
More extensive testing of the patch I just committed in r11-7563 to
avoid the false positive -Warray-bounds on accesses to members of
virtual bases has exposed a couple of problems that cause many false
negatives for even basic bugs like:
typedef struct A { int i; } A;
void* g (void)
{
A *p = (A*)malloc (3);
p->i = 0; // missing warning in C++
return p;
}
as well as a number of others, including more complex ones involving
virtual base classes that can, with some additional work, be reliably
detected. Most of them were successfully diagnosed before the fix
for PR 98266. Unfortunately, the tests that exercise this are all
C so the C++ only regressions due to the divergence went unnoticed.
The root cause is twofold: a) the simplistic check for TYPE_BINFO
in the new inbounds_vbase_memaccess_p() function means we exclude
from checking any member accesses whose leading offset is in bounds,
and b) the check for the leading offset misses cases where just
the ending offset of an access is out of bounds.
The attached patch adds code to restrict the original solution
to just the narrow subset of accesses to members of classes with
virtual bases, and also adds a check for the ending offset.
Is it enough to just fix the ending offset check, and maybe remove
"vbase" from the function name? The shortcut should be valid for
classes without vbases, as well.
You mean check just the ending offset? I did both in case the size
of the member wasn't known or constant but I don't think those cases
come up here. The C++ front end rejects both flexible array members
and variably-modified types in base classes so the size of any such
member should always be constant (right?)
Is the attached update what you're suggesting?
Ah, no, I meant dropping the has_virtual_base check.
Martin
Is the patch okay for trunk?
(The code for has_virtual_base() is adapted from
polymorphic_type_binfo_p() in ipa-utils.h. It seems like a handy
utility that could go in tree.h.)
Martin
PS There is another regression I discovered while working on this.
It's a false positive but unrelated to the r11-7563 fix so I'll
post a patch for separately.
