This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] avoid calling memset et al. with excessively large sizes (PR 79095)
On 01/18/2017 10:45 AM, Martin Sebor wrote:
On 01/18/2017 01:10 AM, Jakub Jelinek wrote:
On Tue, Jan 17, 2017 at 10:59:43PM -0700, Jeff Law wrote:
I agree that breaking those applications would be bad. It could
be dealt with by adding an option to let them disable the insertion
of the trap. With the warning, programmers would get a heads up
that their (already dubious) code won't work otherwise. I don't
think it's a necessary or wise to have the default mode be the most
permissive (and most dangerous) and expect users to tweak options
to make it safe. Rather, I would argue that it should be the other
way around. Make the default safe and strict and let the advanced
users who know how deal with the risks tweak those options.
I still come back to the assertion that changing this loop to a mem* is
fundamentally the wrong thing to do as it changes something that has
well
defined semantics to something that is invalid.
Thus the transformation into a mem* call is invalid.
The mem* call is as valid as the loop, it will work exactly the same.
The problem here isn't the loop itself or the memset call but the
bounds computed from the inputs.
Sorry. My bad. Only memcpy has the SIZE_MAX / 2 restriction.
[ ... ]
Going to work from the self-contained test...
Here's a test case that's closer to the one from the bug. It also
ends up with the out of bounds memset even at -O1, during PRE.
typedef __SIZE_TYPE__ size_t;
struct S
int *p0, *p1, *p2;
size_t size () const { return p1 - p0; }
void f (size_t n) {
if (n > size ()) // can't happen because
foo (n - size ()); // n is in [1, MIN(size() - 1, 3)]
else if (n < size ())
bar (p0 + n);
}
void foo (size_t n)
{
size_t left = (size_t)(p2 - p1);
if (left >= n)
__builtin_memset (p2, 0, n * sizeof *p2);
}
void bar (int*);
};
void f (S &s)
{
size_t n = s.size ();
if (n > 1 && n < 5)
s.f (n - 1);
}
Looking at the .vrp1 dump for this test we find:
;; basic block 3, loop depth 0, count 0, freq 3664, maybe hot
;; prev block 2, next block 4, flags: (NEW, REACHABLE, VISITED)
;; pred: 2 [36.6%] (TRUE_VALUE,EXECUTABLE)
_18 = ASSERT_EXPR <_13, _13 + 18446744073709551614 <= 2>;
_2 = _18 + 18446744073709551615;
if (_2 > _18)
goto <bb 4>; [50.00%]
else
goto <bb 6>; [50.00%]
_2 > _18 is the same as
(_18 - 1) > _18
Which is only true if _18 is zero. And since _18 has a computed range
of [2..4], that can never happen.
VRP doesn't currently handle this case, though we do have some code to
turn relational comparisons into equality comparisons. If I manually
turn the relational into _18 == 0 at that point, then the next VRP pass
is able to optimize the conditional away which in turn eliminates the
problematical memset call & warning.
So one possible approach would be to treat simplify_cond_using_ranges to
simplify that kind of condition. I don't know if that would fix the
reported testcase as I haven't looked at it directly under the debugger yet.
Jeff