On 11/25/20 2:31 AM, Richard Biener wrote:
On Wed, Nov 25, 2020 at 1:45 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
Offsets in pointer expressions are signed but GCC prefers to
represent them as sizetype instead, and sometimes (though not
always) crashes during GIMPLE verification when they're not.
The sometimes-but-not-always part makes it easy for mistakes
to slip in and go undetected for months, until someone either
trips over it by accident, or deliberately tries to break
things (the test case in the bug relies on declaring memchr
with the third argument of type signed long which is what's
apparently needed to trigger the ICE). The attached patch
corrects a couple of such mistakes.
Martin
PS It would save us the time and effort dealing with these
bugs to either detect (or even correct) the mistakes early,
at the time the POINTER_PLUS_EXPR is built. Adding an assert
to gimple_build_assign()) to verify that it has the expected
type (or converting the operand to sizetype) as in the change
below does that. I'm pretty sure I submitted a patch like it
in the past but it was rejected. If I'm wrong or if there are
no objections to it now I'll be happy to commit it as well.
We already verify this in verify_gimple_assign_binary after
each pass. Iff then this would argue for verifying all built
stmts immediately, assigns with verify_gimple_assign.
But I think this is overkill - your testcase is already catched
by the IL verification.
You're right, having the check wouldn't have prevented this bug.
But I'm not worried about this test case. What I'd like to do
is reduce the risk of similar problems happening in the future
where the check would help. Catching problems earlier by having
functions verify their pre- and postconditions is good practice.
So yes, I think all these build() functions should do that (not
necessarily to the same extent as the full-blown IL verification
but at least the basics).
Btw, are you sure the offset returned by constant_byte_string
is never checked to be positive in callers?
The function sets *PTR_OFFSET to sizetype in all but this one
case (actually, it also sets it to integer_zero_one). Callers
then typically compare it to the length of the string to see
if it's less. If not, the result is discarded because it refers
outside the string. It's tested for equality to zero but I don't
see it being checked to see if it's positive and I'm not sure to
what end. What's your concern?
Anyway, as an experiment, I've changed the function to set
the offset to ssizetype instead of sizetype and reran a subset
of the test suite with the check in gimple_build_assign and it
didn't trigger. So I guess the sloppiness here doesn't matter.
That said, there is a bug in the function that I noticed while
making this change so it wasn't a completely pointless exercise.
The function should call itself recursively but instead it calls
string_constant. I'll resubmit the sizetype change with the fix
for this bug
Martin
The gimple-fold.c hunk and the new testcase are OK.
Richard.
Both patches were tested on x86_64-linux.
diff --git a/gcc/gimple.c b/gcc/gimple.c
index e3e508daf2f..8e88bab9e41 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -489,6 +489,9 @@ gassign *
gimple_build_assign (tree lhs, enum tree_code subcode, tree op1,
tree op2 MEM_STAT_DECL)
{
+ if (subcode == POINTER_PLUS_EXPR)
+ gcc_checking_assert (ptrofftype_p (TREE_TYPE (op2)));
+
return gimple_build_assign_1 (lhs, subcode, op1, op2, NULL_TREE
PASS_MEM_STAT);
}
