This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Warn on and fold NULL checks against inline functions


On Tue, May 27, 2014 at 4:06 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Tue, May 27, 2014 at 8:32 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> On Tue, May 27, 2014 at 3:33 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Mon, May 26, 2014 at 9:01 PM, Patrick Palka <patrick@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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]