On Wed, Aug 29, 2018 at 2:12 AM Martin Sebor <mse...@gmail.com> wrote: > > >> Sadly, dstbase is the PARM_DECL for d. That's where things are going > >> "wrong". Not sure why you're getting the PARM_DECL in that case. I'd > >> debug get_addr_base_and_unit_offset to understand what's going on. > >> Essentially you're getting different results of > >> get_addr_base_and_unit_offset in a case where they arguably should be > >> the same. > > > > Probably get_attr_nonstring_decl has the same "mistake" and returns > > the PARM_DECL instead of the SSA name pointer. So we're comparing > > apples and oranges here. > > Returning the SSA_NAME_VAR from get_attr_nonstring_decl() is > intentional but the function need not (perhaps should not) > also set *REF to it. > > > > > Yeah: > > > > /* If EXPR refers to a character array or pointer declared attribute > > nonstring return a decl for that array or pointer and set *REF to > > the referenced enclosing object or pointer. Otherwise returns > > null. */ > > > > tree > > get_attr_nonstring_decl (tree expr, tree *ref) > > { > > tree decl = expr; > > if (TREE_CODE (decl) == SSA_NAME) > > { > > gimple *def = SSA_NAME_DEF_STMT (decl); > > > > if (is_gimple_assign (def)) > > { > > tree_code code = gimple_assign_rhs_code (def); > > if (code == ADDR_EXPR > > || code == COMPONENT_REF > > || code == VAR_DECL) > > decl = gimple_assign_rhs1 (def); > > } > > else if (tree var = SSA_NAME_VAR (decl)) > > decl = var; > > } > > > > if (TREE_CODE (decl) == ADDR_EXPR) > > decl = TREE_OPERAND (decl, 0); > > > > if (ref) > > *ref = decl; > > > > I see a lot of "magic" here again in the attempt to "propagate" > > a nonstring attribute. > > That's the function's purpose: to look for the attribute. Is > there a better way to do this?
Well, the question is what "nonstring" is, semantically. I read it as sth like __restrinct - a pointer with "nonstring" attribute points to a non-string. So I suspect your function either computes "may expr point to a nonstring" or "must expr point to a nonstring" if it gets a pointer argument. If it gets a (string) object it checks whether that object is declared "nonstring" (thus, if you'd built a pointer to expr whether that pointer _must_ point to a nonstring. So I guess the first one is "must". Clearly looking at SSA_NAME_VAR isn't good here, it would be semantically correct only for SSA_NAME_IS_DEFAULT_DEF and SSA_NAME_VAR being a PARM_DECL. I guess it would be nice to clearly separate the pointer vs. object case by documentation in the function - all of the quoted parts above seem to be for the address case so a gcc_assert (POINTER_TYPE_P (TREE_TYPE (decl)) inside the if (TREE_CODE (decl) == SSA_NAME) path should never trigger? > > Note > > > > foo (char *p __attribute__(("nonstring"))) > > { > > p = "bar"; > > strlen (p); // or whatever is necessary to call get_attr_nonstring_decl > > } > > > > is perfectly valid and p as passed to strlen is _not_ nonstring(?). > > I don't know if you're saying that it should get a warning or > shouldn't. Right now it doesn't because the strlen() call is > folded before we check for nonstring. I say it shouldn't because I assign "bar" to p and after that p isn't the original parameter anymore? > I could see an argument for diagnosing it but I suspect you > wouldn't like it because it would mean more warning from > the folder. I could also see an argument against it because, > as you said, it's safe. > > If you take the assignment to p away then a warning is issued, > and that's because p is declared with attribute nonstring. > That's also why get_attr_nonstring_decl looks at SSA_NAME_VAR. > > > I think in your code comparing bases you want to look at the _original_ > > argument to the string function rather than what get_attr_nonstring_decl > > returned as ref. > > I've adjusted get_attr_nonstring_decl() to avoid setting *REF > to SSA_NAME_VAR. That let me remove the GIMPLE_NOP code from > the patch. I've also updated the comment above SSA_NAME_VAR > to clarify its purpose per Jeff's comments. > > Attached is an updated revision with these changes. > > Martin