On Tue, 10 Oct 2023, Jakub Jelinek wrote: > On Tue, Oct 10, 2023 at 10:49:04AM +0000, Richard Biener wrote: > > The following fixes a mistake in count_nonzero_bytes which happily > > skips over stores clobbering the memory we load a value we store > > from and then performs analysis on the memory state before the > > intermediate store. > > > > The patch implements the most simple fix - guarantee that there are > > no intervening stores by tracking the original active virtual operand > > and comparing that to the one of a load we attempt to analyze. > > > > This simple approach breaks two subtests of gcc.dg/Wstringop-overflow-69.c > > which I chose to XFAIL. > > This function is a total mess, but I think punting right after the > gimple_assign_single_p test is too big hammer. > There are various cases and some of them are fine even when vuse is > different, others aren't. > The function at that point tries to handle CONSTRUCTOR, MEM_REF, or decls. > > I don't see why the CONSTRUCTOR case couldn't be fine regardless of the > vuse. Though, am not really sure when a CONSTRUCTOR would appear, the > lhs would need to be an SSA_NAME, so wouldn't for vectors that be a > VECTOR_CST instead, etc.? Ah, perhaps a vector with some non-constant > element in it.
Yeah, but what should that be interpreted to in terms of object-size?! I think the only real case we'd see here is the MEM_REF RHS given we know we have a register type value. I'll note the function totally misses handled_component_p (), it only seems to handle *p and 'decl'. > The MEM_REF case supposedly only if we can guarantee nothing could overwrite > it (so MEM_REF of TREE_STATIC TREE_READONLY could be fine, STRING_CST too, > anything else is wrong - count_nonzero_bytes_addr uses the > get_stridx/get_strinfo APIs, which for something that can be changed > always reflects only the state at the current statement. So, e.g. the > get_stridx (exp, stmt) > 0 case in count_nonzero_bytes_addr is when > the caller must definitely punt if vuse is different. > Then for decls, again, CONST_DECLs, DECL_IN_CONSTANT_POOLs are certainly > fine. Other decls for which ctor_for_folding returns non-error_mark_node > should be fine as well, I think ctor_useable_for_folding_p is supposed > to verify that. STRING_CSTs should be fine as well. > > So maybe pass the vuse down to count_nonzero_bytes_addr and return false > in the idx > 0 case in there if gimple_vuse (stmt) != vuse? I don't know enough of the pass to do better, can you take it from here? One of the points is that we need to know the memory context (aka vuse) of both the original store and the load we are interpreting. For _addr I wasn't sure how we arrive here. As you said, this is a bit of spaghetti and I don't want to untangle this any further. Thanks, Richard.
