On 11/19/2017 10:06 AM, Martin Sebor wrote:
>>> +/* 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. */
>> Generally we'd write NULL rather than null for a generic pointer and in
>> this specific case NULL_TREE is even better.
>
> I agree. The C NULL macro, C++ nullptr, and GCC's NULL_TREE and
> NULL_RTL are helpful both for type safety and readability of code.
> But they all are implementation details that's ot not relevant to the
> specification
> of an API or its users.
>
> The established term that every C and C++ programmer is familiar
> with is "null pointer."
>
> and exist
> mainly for type safety. They are not relevant to the specification
> of an API or its users. I didn't think too hard about it when
> I wrote the comment (it just didn't seem important enough) but
> now
> that I have I believe "null" is better, clearer, and more future-
> proof (in case the return type should ever change to some smart
> pointer class).
>
> (There also comments in this file and elsewhere in GCC that use
> various forms of null.)
Perhaps so, but please capitalize it -- while there may be "null"
references, I believe in all-caps is by far more common within comments
within the GCC sources.
WRT NULL vs NULL_TREE, I'd tend to disagree. The function isn't going
to return NULL, it's going to return NULL_TREE. That's actually already
exposed in the API/ABI by the return value's type. The fact that NULL
and NULL_TREE are interchangeable is an implementation detail and we
shouldn't encourage folks to rely on that by way of the function comment.
>
>>> + if (TREE_CODE (dcl) == SSA_NAME)
>>> + {
>>> + if (SSA_NAME_IS_DEFAULT_DEF (dcl))
>>> + dcl = SSA_NAME_VAR (dcl);
>>> + else
>>> + {
>>> + gimple *def = SSA_NAME_DEF_STMT (dcl);
>>> +
>>> + if (is_gimple_assign (def))
>>> + {
>>> + tree_code code = gimple_assign_rhs_code (def);
>>> + if (code == ADDR_EXPR
>>> + || code == COMPONENT_REF
>>> + || code == VAR_DECL)
>>> + dcl = gimple_assign_rhs1 (def);
>> Note that COMPONENT_REFs can have arguments that are themselves
>> COMPONENT_REFS. So you typically want to use a loop to strip all away.
>
> I have seen it in the early IL but this late I couldn't come
> up with a test case where such a loop would be necessary.
> The COMPONENT_REF always refers to the member array. If you
> can show me an example to test with I'll add the handling if
> possible. There are cases where it isn't, such as in
>
> struct A { char a[4] __attribute__ ((nonstring)); } *pa;
>
> strlen (pa->a + 1);
>
> where pa->a is represented as a MEM_REF (char[4], pa, 1) with
> the member information having been lost in translation. (This
> is the same problem that I had solved for the -Warray-bounds
> warning for out-of bounds offsets by implementing it in
> tree-ssa-forwprop.c: because that's where the member is folded
> into a reference to the base object.)
Set up a nested structure then reference it like a.b.c.d.
>
>>> + /* It's safe to call strndup and strnlen with a non-string argument
>>> + since the functions provide an explicit bound for this
>>> purpose. */
>>> + if (DECL_FUNCTION_CODE (fndecl) == BUILT_IN_STRNDUP)
>>> + return;
>> So I see you handle BUILT_IN_STRNDUP here, but not BUILT_IN_STRNLEN. Is
>> there a reason for the omission?
>
> strnlen is not a GCC built-in so it's handled implicitly by
> the test for DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL
> just before the if statement above. The test I added verifies
> that strnlen is excluded. (Bug 81384 tracks the missing
> intrinsic support for strnlen.)
>
> That said, more testing revealed that Glibc uses functions like
> strncmp and strncpy to compare and copy non-strings, so I've
> added those to the white list and improved the checking a bit.
> I've also changed the hash_map to a pointer to make Jakub happy.
OK. Thanks.
>
>
