On 12/5/19 4:37 PM, Martin Sebor wrote:
If yes, since I don't think I have the time for it for GCC 10
I need to decide whether to drop just this improvement or all
of the buffer overflow checks that depend on it.
Let me know which you prefer.
As I mentioned in my previous message, I think we've got two potential
paths. We could just drop the problem hunk and adjust the tests with
xfails, but I'm not sure how badly that compromises what you're trying
to do. We could also leave the hunk in with a fixme or somesuch.
It would compromise the buffer overflow detection in cases where
the length of a string computed by the pass is then used in the same
function to access another string. There are more of these now as
the strlen has been getting better and better at tracking the string
lenghts.
Rather than using fixed size arrays, code that deals with strings
of unlimited lengths has to allocate space for them dynamically.
Until now, the strlen pass hasn't tried to detect or prevent
overflow into those (and transformed writes to them in ways that
prevented it from being detected later by _FORTIFY_SOURCE). But
the follow-on enhancement to this patch changes that, and so this
limitation becomes more of an impediment.
Maybe I would ask the question differently. In real world scenarios how
often does this happen? How about in tests you're adding? Is there
value in the patch without this hunk?
To reiterate what we discussed privately: the patches in this
series (the dynamic buffer overflow detection) have considerable
value even without the hunk. The detection will work correctly
in most cases. It will fail and cause false negatives in
the specific cases I mentioned uptread, namely:
where the out-of-bounds store is to a character array whose size
or offset depend on the length of a string previously computed
by the strlen pass. The class of these cases is large but, due
to the limitation (it has to be the same SSA_NAME that's used
in both the allocation context and the destination offset),
the subset where it is effective is relatively small.
Essentially I'm trying to make a determination if the patch and its
follow-ups have value without this hack.
But I don't understand why you think using the RHS of an SSA_NAME
assignment is a problem when it's an INTEGER_CST. Is that unsafe
for some reason, or likely to fail somehow?
Because it's just working around a failing in our optimizers and a
fairly trivial one at that. I'd rather fix the real problem rather than
just paper over it.
The robust solution is to implement the full constant propagation
in (or for) the strlen pass. Hopefully, the on-demand VRP will
give us that in GCC 11. If not, I will look into implementing
it myself.
Since the workaround stands in the way of getting the patch
approved I have removed it. Attached is a revised patch, rebased
on the top of trunk and retested on x86_64-linux.
I have committed this in r279248 with Jeff's off-list approval.
Martin
