On Wed, Nov 25, 2020 at 7:22 PM Martin Sebor <mse...@gmail.com> wrote: > > 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?
My concern was that code might do /* Handle broken code. */ if (tree_int_cst_sgn (offset) < 0) return NULL_TREE; .. expect offset to be within the string .. but I didn't look at much context. If the function uses sizetype everywhere else that concern is moot and thus this hunk is OK as well. Thanks, Richard. > 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); > >> } >