On Tue, May 27, 2014 at 4:06 PM, Patrick Palka <patr...@parcs.ath.cx> wrote:
> On Tue, May 27, 2014 at 8:32 AM, Patrick Palka <patr...@parcs.ath.cx> wrote:
>> On Tue, May 27, 2014 at 3:33 AM, Richard Biener
>> <richard.guent...@gmail.com> wrote:
>>> On Mon, May 26, 2014 at 9:01 PM, Patrick Palka <patr...@parcs.ath.cx> wrote:
>>>> Hi,
>>>>
>>>> This patch teaches the C++ frontend to warn on NULL checks against
>>>> inline functions and it teaches the middle-end to fold NULL checks
>>>> against inline functions. These two things are currently done for
>>>> non-inline C++ functions, but inline functions are exceptional in that
>>>> the C++ frontend marks them as weak, and NULL checks against weak
>>>> symbols cannot be folded in general because the symbol may be mapped to
>>>> NULL at link-time.
>>>>
>>>> But in the case of an inline function, the fact that it's a weak symbol
>>>> is an implementation detail. And because it is not permitted to
>>>> explicitly give an inline function the "weak" attribute (see
>>>> handle_weak_attribute), in order to acheive $SUBJECT it suffices to
>>>> assume that all inline functions are non-null, which is what this patch
>>>> does.
>>>>
>>>> Bootstrap and regtest against x86_64-unknown-linux-gnu in progress. Is
>>>> this patch OK assuming no regressions?
>>>
>>> What about always_inline function wrappers as used in FORTIFY_SOURCE?
>>> IIRC the actual referenced function may be declared weak and thus the
>>> address can compare to NULL? Sth like
>>>
>>> extern void __foo (void) __attribute__((weak,asm("foo")));
>>> extern inline __attribute__((always_inline,gnu_inline)) void foo (void)
>>> {
>>> __foo ();
>>> }
>>>
>>> int main()
>>> {
>>> if (foo == 0)
>>> return 0;
>>> abort ();
>>> }
>>>
>>> The issue is that the inline and alias may be hidden to the user and thus
>>> he'll get unwanted and confusing warnings.
>>>
>>> Richard.
>>
>> So as it stands the foo == 0 check in the above example gets folded
>> with or without my patch. The issue here seems to be related to the
>> use of gnu_inline. The address of an inline function marked
>> gnu_inline shouldn't be considered non-NULL because a standalone
>> function does not actually get emitted. Of course, one could inspect
>> aliases but in general it is not correct to assume such functions are
>> non-NULL. Does this sound right?
>>
>> This issue has slight overlap with the issue that my patch tries to
>> fix. I could try to handle it as well.
>
> Oh, disregard the above. I think I see what you mean now, Richard.
> Because __foo is an alias for foo, and __foo is weak, is it safe to
> assume that foo is non-NULL? That is a tricky question...
No (I wasn't making a folding validity remark). I want to make sure
we won't see diagnostics on code like
#include <string.h>
int main()
{
if (malloc != 0)
...
}
just because it is built with -D_FORTIFY_SOURCE. Whether we
may fold that check at all is another question, of course (and again
that should _not_ differ between -D_FORTIFY_SOURCE or -U_FORTIFY_SOURCE,
but it may well also be a glibc issue).
And yes, aliases are tricky beasts ...
Richard.
