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...
